[Qemu-devel] [PATCH 0/3] Misc Quorum fixes

[Qemu-devel] [PATCH 0/3] Misc Quorum fixes

[Alberto Garcia]

Hi,

Here's a couple of small fixes for the Quorum driver plus new test
cases. See the individual patches for details.

Regards,

Berto

Alberto Garcia (3):
  quorum: Remove quorum_err()
  quorum: Return an error if the blkverify mode has invalid settings
  iotest: Test the blkverify mode of the Quorum driver

 block/quorum.c             | 37 +++++++++++--------------------------
 tests/qemu-iotests/081     | 30 ++++++++++++++++++++++++++++++
 tests/qemu-iotests/081.out | 16 ++++++++++++++++
 3 files changed, 57 insertions(+), 26 deletions(-)

-- 
2.11.0

[Qemu-devel] [PATCH 1/3] quorum: Remove quorum_err()

[Alberto Garcia]

This is a static function with only one caller, so there's no need to
keep it. Inlining the code in quorum_compare() makes it much simpler.

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

diff --git a/block/quorum.c b/block/quorum.c
index eb526cc0f1..b1b777baef 100644
--- a/block/quorum.c
+++ b/block/quorum.c
@@ -437,23 +437,7 @@ static bool quorum_iovec_compare(QEMUIOVector *a, QEMUIOVector *b)
     return true;
 }
 
-static void GCC_FMT_ATTR(2, 3) quorum_err(QuorumAIOCB *acb,
-                                          const char *fmt, ...)
-{
-    va_list ap;
-
-    va_start(ap, fmt);
-    fprintf(stderr, "quorum: offset=%" PRIu64 " bytes=%" PRIu64 " ",
-            acb->offset, acb->bytes);
-    vfprintf(stderr, fmt, ap);
-    fprintf(stderr, "\n");
-    va_end(ap);
-    exit(1);
-}
-
-static bool quorum_compare(QuorumAIOCB *acb,
-                           QEMUIOVector *a,
-                           QEMUIOVector *b)
+static bool quorum_compare(QuorumAIOCB *acb, QEMUIOVector *a, QEMUIOVector *b)
 {
     BDRVQuorumState *s = acb->bs->opaque;
     ssize_t offset;
@@ -462,8 +446,10 @@ static bool quorum_compare(QuorumAIOCB *acb,
     if (s->is_blkverify) {
         offset = qemu_iovec_compare(a, b);
         if (offset != -1) {
-            quorum_err(acb, "contents mismatch at offset %" PRIu64,
-                       acb->offset + offset);
+            fprintf(stderr, "quorum: offset=%" PRIu64 " bytes=%" PRIu64
+                    " contents mismatch at offset %" PRIu64 "\n",
+                    acb->offset, acb->bytes, acb->offset + offset);
+            exit(1);
         }
         return true;
     }
-- 
2.11.0

[Qemu-devel] [PATCH 2/3] quorum: Return an error if the blkverify mode has invalid settings

[Alberto Garcia]

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

[Qemu-devel] [PATCH 3/3] iotest: Test the blkverify mode of the Quorum driver

[Alberto Garcia]

Signed-off-by: Alberto Garcia <berto@igalia.com>
---
 tests/qemu-iotests/081     | 30 ++++++++++++++++++++++++++++++
 tests/qemu-iotests/081.out | 16 ++++++++++++++++
 2 files changed, 46 insertions(+)

diff --git a/tests/qemu-iotests/081 b/tests/qemu-iotests/081
index da3fb0984b..0ea010afbf 100755
--- a/tests/qemu-iotests/081
+++ b/tests/qemu-iotests/081
@@ -168,6 +168,36 @@ echo "== checking that quorum is broken =="
 
 $QEMU_IO -c "open -o $quorum" -c "read -P 0x32 0 $size" | _filter_qemu_io
 
+echo
+echo "== checking the blkverify mode with broken content =="
+
+quorum="driver=raw,file.driver=quorum,file.vote-threshold=2,file.blkverify=on"
+quorum="$quorum,file.children.0.file.filename=$TEST_DIR/1.raw"
+quorum="$quorum,file.children.1.file.filename=$TEST_DIR/2.raw"
+quorum="$quorum,file.children.0.driver=raw"
+quorum="$quorum,file.children.1.driver=raw"
+
+$QEMU_IO -c "open -o $quorum" -c "read -P 0x32 0 $size" | _filter_qemu_io
+
+echo
+echo "== writing the same data to both files =="
+
+$QEMU_IO -c "write -P 0x32 0 $size" "$TEST_DIR/1.raw" | _filter_qemu_io
+$QEMU_IO -c "write -P 0x32 0 $size" "$TEST_DIR/2.raw" | _filter_qemu_io
+
+echo
+echo "== checking the blkverify mode with valid content =="
+
+$QEMU_IO -c "open -o $quorum" -c "read -P 0x32 0 $size" | _filter_qemu_io
+
+echo
+echo "== checking the blkverify mode with invalid settings =="
+
+quorum="$quorum,file.children.2.file.filename=$TEST_DIR/3.raw"
+quorum="$quorum,file.children.2.driver=raw"
+
+$QEMU_IO -c "open -o $quorum" | _filter_qemu_io
+
 # success, all done
 echo "*** done"
 rm -f $seq.full
diff --git a/tests/qemu-iotests/081.out b/tests/qemu-iotests/081.out
index 2533c31c78..2f12c890e9 100644
--- a/tests/qemu-iotests/081.out
+++ b/tests/qemu-iotests/081.out
@@ -55,4 +55,20 @@ wrote 10485760/10485760 bytes at offset 0
 
 == checking that quorum is broken ==
 read failed: Input/output error
+
+== checking the blkverify mode with broken content ==
+quorum: offset=0 bytes=10485760 contents mismatch at offset 0
+
+== writing the same data to both files ==
+wrote 10485760/10485760 bytes at offset 0
+10 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+wrote 10485760/10485760 bytes at offset 0
+10 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+
+== checking the blkverify mode with valid content ==
+read 10485760/10485760 bytes at offset 0
+10 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+
+== checking the blkverify mode with invalid settings ==
+can't open: blkverify=on can only be set if there are exactly two files and vote-threshold is 2
 *** done
-- 
2.11.0

Re: [Qemu-devel] [PATCH 2/3] quorum: Return an error if the blkverify mode has invalid settings

[Kevin Wolf]

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

Re: [Qemu-devel] [PATCH 0/3] Misc Quorum fixes

[Kevin Wolf]

Am 17.10.2018 um 16:33 hat Alberto Garcia geschrieben:
> Hi,
> 
> Here's a couple of small fixes for the Quorum driver plus new test
> cases. See the individual patches for details.

Thanks, applied to the block branch.

Kevin

Re: [Qemu-devel] [PATCH 2/3] quorum: Return an error if the blkverify mode has invalid settings

[Alberto Garcia]

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

Re: [Qemu-devel] [PATCH 2/3] quorum: Return an error if the blkverify mode has invalid settings

[Kevin Wolf]

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