The blkverify mode of Quorum can only be enabled if the number of
children is exactly two and the value of vote-threshold is also two.

If the user tries to enable it but the other settings are incorrect
then QEMU simply prints an error message to stderr and carries on
disabling the blkverify setting.

This patch makes quorum_open() fail and return an error in this case.

Signed-off-by: Alberto Garcia <berto@igalia.com>
Reported-by: Markus Armbruster <armbru@redhat.com>
---
 block/quorum.c | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/block/quorum.c b/block/quorum.c
index b1b777baef..6188ff6666 100644
--- a/block/quorum.c
+++ b/block/quorum.c
@@ -912,13 +912,12 @@ static int quorum_open(BlockDriverState *bs, QDict *options, int flags,
     s->read_pattern = ret;
 
     if (s->read_pattern == QUORUM_READ_PATTERN_QUORUM) {
-        /* is the driver in blkverify mode */
-        if (qemu_opt_get_bool(opts, QUORUM_OPT_BLKVERIFY, false) &&
-            s->num_children == 2 && s->threshold == 2) {
-            s->is_blkverify = true;
-        } else if (qemu_opt_get_bool(opts, QUORUM_OPT_BLKVERIFY, false)) {
-            fprintf(stderr, "blkverify mode is set by setting blkverify=on "
-                    "and using two files with vote_threshold=2\n");
+        s->is_blkverify = qemu_opt_get_bool(opts, QUORUM_OPT_BLKVERIFY, false);
+        if (s->is_blkverify && (s->num_children != 2 || s->threshold != 2)) {
+            error_setg(&local_err, "blkverify=on can only be set if there are "
+                       "exactly two files and vote-threshold is 2");
+            ret = -EINVAL;
+            goto exit;
         }
 
         s->rewrite_corrupted = qemu_opt_get_bool(opts, QUORUM_OPT_REWRITE,
-- 
2.11.0

Am 17.10.2018 um 16:33 hat Alberto Garcia geschrieben:
> The blkverify mode of Quorum can only be enabled if the number of
> children is exactly two and the value of vote-threshold is also two.
> 
> If the user tries to enable it but the other settings are incorrect
> then QEMU simply prints an error message to stderr and carries on
> disabling the blkverify setting.
> 
> This patch makes quorum_open() fail and return an error in this case.
> 
> Signed-off-by: Alberto Garcia <berto@igalia.com>
> Reported-by: Markus Armbruster <armbru@redhat.com>

While reviewing this, I think I found another problem: Shouldn't
.bdrv_add_child refuse to add a child if s->is_blkverify == true? (And
probably assert in .bdrv_del_child that s->is_blkverify == false after
checking s->num_children <= s->threshold)

Kevin

On Wed 17 Oct 2018 05:31:32 PM CEST, Kevin Wolf wrote:
> Am 17.10.2018 um 16:33 hat Alberto Garcia geschrieben:
>> The blkverify mode of Quorum can only be enabled if the number of
>> children is exactly two and the value of vote-threshold is also two.
>> 
>> If the user tries to enable it but the other settings are incorrect
>> then QEMU simply prints an error message to stderr and carries on
>> disabling the blkverify setting.
>> 
>> This patch makes quorum_open() fail and return an error in this case.
>> 
>> Signed-off-by: Alberto Garcia <berto@igalia.com>
>> Reported-by: Markus Armbruster <armbru@redhat.com>
>
> While reviewing this, I think I found another problem: Shouldn't
> .bdrv_add_child refuse to add a child if s->is_blkverify == true? (And
> probably assert in .bdrv_del_child that s->is_blkverify == false after
> checking s->num_children <= s->threshold)

I think you're right. Do you want me to send this in a later patch or do
you prefer that I make it part of this series?

Berto

Am 17.10.2018 um 17:34 hat Alberto Garcia geschrieben:
> On Wed 17 Oct 2018 05:31:32 PM CEST, Kevin Wolf wrote:
> > Am 17.10.2018 um 16:33 hat Alberto Garcia geschrieben:
> >> The blkverify mode of Quorum can only be enabled if the number of
> >> children is exactly two and the value of vote-threshold is also two.
> >> 
> >> If the user tries to enable it but the other settings are incorrect
> >> then QEMU simply prints an error message to stderr and carries on
> >> disabling the blkverify setting.
> >> 
> >> This patch makes quorum_open() fail and return an error in this case.
> >> 
> >> Signed-off-by: Alberto Garcia <berto@igalia.com>
> >> Reported-by: Markus Armbruster <armbru@redhat.com>
> >
> > While reviewing this, I think I found another problem: Shouldn't
> > .bdrv_add_child refuse to add a child if s->is_blkverify == true? (And
> > probably assert in .bdrv_del_child that s->is_blkverify == false after
> > checking s->num_children <= s->threshold)
> 
> I think you're right. Do you want me to send this in a later patch or do
> you prefer that I make it part of this series?

This series looked good, so no need to respin. A separate patch is fine.

Kevin