[Qemu-devel] [PATCH v2 0/2] quorum: Simplify quorum_open()

[Qemu-devel] [PATCH v2 0/2] quorum: Simplify quorum_open()

[Max Reitz]

This series employs the new ability of qdict_array_split() to not only
split a QDict into a QList of QDicts (and a remainder QDict), but into a
QList of QObjects of any kind, to simplify quorum_open(). This (in my
opinion) enhances readability, robustness against malformatted options
and allows specifying both reference strings and full option dicts at
the same time.

This series depends on v19 of Benoît's Quorum series and on my "Extract
non-QDicts in qdict_array_split()" series.

v2:
 - Patch 1: Adapted comment about how num_children is determined
   [Benoît]
 - Patch 2: Fixing the monitor output of quorum (v19) requires fixing of
   the test output as well; however, filtering the sector-num is no
   longer required


Max Reitz (2):
  quorum: Simplify quorum_open()
  iotests: Mixed quorum child device specifications

 block/quorum.c             | 66 +++++++++++++++++++++++++++-------------------
 tests/qemu-iotests/081     | 51 +++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/081.out | 15 +++++++++++
 3 files changed, 105 insertions(+), 27 deletions(-)

-- 
1.9.0

[Qemu-devel] [PATCH v2 1/2] quorum: Simplify quorum_open()

[Max Reitz]

Although it may not look like it, this patch simplifies quorum_open().
qdict_array_split() is now able to return QLists with different objects
than only QDicts, therefore it will now do all the work and
quorum_open() does not have to handle reference strings by itself.

This allows mixing full option dicts and reference strings for
specifying the child block devices of quorum; furthermore, it improves
handling of malformed specifications.

Signed-off-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: Benoit Canet <benoit@irqsave.net>
---
 block/quorum.c | 66 ++++++++++++++++++++++++++++++++++------------------------
 1 file changed, 39 insertions(+), 27 deletions(-)

diff --git a/block/quorum.c b/block/quorum.c
index 73dd45b..6c28239 100644
--- a/block/quorum.c
+++ b/block/quorum.c
@@ -720,7 +720,6 @@ static int quorum_open(BlockDriverState *bs, QDict *options, int flags,
     QDict *sub = NULL;
     QList *list = NULL;
     const QListEntry *lentry;
-    const QDictEntry *dentry;
     int i;
     int ret = 0;
 
@@ -728,10 +727,15 @@ static int quorum_open(BlockDriverState *bs, QDict *options, int flags,
     qdict_extract_subqdict(options, &sub, "children.");
     qdict_array_split(sub, &list);
 
-    /* count how many different children are present and validate
-     * qdict_size(sub) address the open by reference case
-     */
-    s->num_children = !qlist_size(list) ? qdict_size(sub) : qlist_size(list);
+    if (qdict_size(sub)) {
+        error_setg(&local_err, "Invalid option children.%s",
+                   qdict_first(sub)->key);
+        ret = -EINVAL;
+        goto exit;
+    }
+
+    /* count how many different children are present */
+    s->num_children = qlist_size(list);
     if (s->num_children < 2) {
         error_setg(&local_err,
                    "Number of provided children must be greater than 1");
@@ -767,30 +771,38 @@ static int quorum_open(BlockDriverState *bs, QDict *options, int flags,
     s->bs = g_new0(BlockDriverState *, s->num_children);
     opened = g_new0(bool, s->num_children);
 
-    /* Open by file name or options dict (command line or QMP) */
-    if (s->num_children == qlist_size(list)) {
-        for (i = 0, lentry = qlist_first(list); lentry;
-            lentry = qlist_next(lentry), i++) {
-            QDict *d = qobject_to_qdict(lentry->value);
-            QINCREF(d);
-            ret = bdrv_open(&s->bs[i], NULL, NULL, d, flags, NULL, &local_err);
-            if (ret < 0) {
-                goto close_exit;
-            }
-            opened[i] = true;
+    for (i = 0, lentry = qlist_first(list); lentry;
+         lentry = qlist_next(lentry), i++) {
+        QDict *d;
+        QString *string;
+
+        switch (qobject_type(lentry->value))
+        {
+            /* List of options */
+            case QTYPE_QDICT:
+                d = qobject_to_qdict(lentry->value);
+                QINCREF(d);
+                ret = bdrv_open(&s->bs[i], NULL, NULL, d, flags, NULL,
+                                &local_err);
+                break;
+
+            /* QMP reference */
+            case QTYPE_QSTRING:
+                string = qobject_to_qstring(lentry->value);
+                ret = bdrv_open(&s->bs[i], NULL, qstring_get_str(string), NULL,
+                                flags, NULL, &local_err);
+                break;
+
+            default:
+                error_setg(&local_err, "Specification of child block device %i "
+                           "is invalid", i);
+                ret = -EINVAL;
         }
-    /* Open by QMP references */
-    } else {
-        for (i = 0, dentry = qdict_first(sub); dentry;
-             dentry = qdict_next(sub, dentry), i++) {
-            QString *string = qobject_to_qstring(dentry->value);
-            ret = bdrv_open(&s->bs[i], NULL, qstring_get_str(string), NULL,
-                            flags, NULL, &local_err);
-            if (ret < 0) {
-                goto close_exit;
-            }
-            opened[i] = true;
+
+        if (ret < 0) {
+            goto close_exit;
         }
+        opened[i] = true;
     }
 
     g_free(opened);
-- 
1.9.0

[Qemu-devel] [PATCH v2 2/2] iotests: Mixed quorum child device specifications

[Max Reitz]

Add a test case to test 081 for mixing full option dicts and reference
strings of specifying the quorum child block devices through QMP.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 tests/qemu-iotests/081     | 51 ++++++++++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/081.out | 15 ++++++++++++++
 2 files changed, 66 insertions(+)

diff --git a/tests/qemu-iotests/081 b/tests/qemu-iotests/081
index be34544..f053f11 100755
--- a/tests/qemu-iotests/081
+++ b/tests/qemu-iotests/081
@@ -44,6 +44,18 @@ _supported_fmt raw
 _supported_proto generic
 _supported_os Linux
 
+function do_run_qemu()
+{
+    echo Testing: "$@" | _filter_imgfmt
+    $QEMU -nographic -qmp stdio -serial none "$@"
+    echo
+}
+
+function run_qemu()
+{
+    do_run_qemu "$@" 2>&1 | _filter_testdir | _filter_qmp | _filter_qemu_io
+}
+
 quorum="file.driver=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.2.file.filename=$TEST_DIR/3.raw,file.vote-threshold=2"
@@ -80,6 +92,45 @@ echo "== checking quorum correction =="
 $QEMU_IO -c "open -o $quorum" -c "read -P 0x32 0 $size" | _filter_qemu_io
 
 echo
+echo "== checking mixed reference/option specification =="
+
+run_qemu -drive "file=$TEST_DIR/2.raw,format=$IMGFMT,if=none,id=drive2" <<EOF
+{ "execute": "qmp_capabilities" }
+{ "execute": "blockdev-add",
+    "arguments": {
+        "options": {
+            "driver": "quorum",
+            "id": "drive0-quorum",
+            "vote-threshold": 2,
+            "children": [
+                {
+                    "driver": "raw",
+                    "file": {
+                        "driver": "file",
+                        "filename": "$TEST_DIR/1.raw"
+                    }
+                },
+                "drive2",
+                {
+                    "driver": "raw",
+                    "file": {
+                        "driver": "file",
+                        "filename": "$TEST_DIR/3.raw"
+                    }
+                }
+            ]
+        }
+    }
+}
+{ "execute": "human-monitor-command",
+    "arguments": {
+        "command-line": 'qemu-io drive0-quorum "read -P 0x32 0 $size"'
+    }
+}
+{ "execute": "quit" }
+EOF
+
+echo
 echo "== breaking quorum =="
 
 $QEMU_IO -c "write -P 0x41 0 $size" "$TEST_DIR/1.raw" | _filter_qemu_io
diff --git a/tests/qemu-iotests/081.out b/tests/qemu-iotests/081.out
index b5b55ab..4fe2f95 100644
--- a/tests/qemu-iotests/081.out
+++ b/tests/qemu-iotests/081.out
@@ -25,6 +25,21 @@ wrote 10485760/10485760 bytes at offset 0
 read 10485760/10485760 bytes at offset 0
 10 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
 
+== checking mixed reference/option specification ==
+Testing: -drive file=TEST_DIR/2.IMGFMT,format=IMGFMT,if=none,id=drive2
+QMP_VERSION
+{"return": {}}
+{"return": {}}
+{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "QUORUM_REPORT_BAD", "data": {"node-name": "", "ret": 0, "sectors-count": 20480, "sector-num": 0}}
+read 10485760/10485760 bytes at offset 0
+10 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+{"return": ""}
+{"return": {}}
+{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "SHUTDOWN"}
+{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "DEVICE_TRAY_MOVED", "data": {"device": "ide1-cd0", "tray-open": true}}
+{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "DEVICE_TRAY_MOVED", "data": {"device": "floppy0", "tray-open": true}}
+
+
 == breaking quorum ==
 wrote 10485760/10485760 bytes at offset 0
 10 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
-- 
1.9.0

Re: [Qemu-devel] [PATCH v2 2/2] iotests: Mixed quorum child device specifications

[Benoît Canet]

The Friday 21 Feb 2014 à 22:30:38 (+0100), Max Reitz wrote :
> Add a test case to test 081 for mixing full option dicts and reference
> strings of specifying the quorum child block devices through QMP.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  tests/qemu-iotests/081     | 51 ++++++++++++++++++++++++++++++++++++++++++++++
>  tests/qemu-iotests/081.out | 15 ++++++++++++++
>  2 files changed, 66 insertions(+)
> 
> diff --git a/tests/qemu-iotests/081 b/tests/qemu-iotests/081
> index be34544..f053f11 100755
> --- a/tests/qemu-iotests/081
> +++ b/tests/qemu-iotests/081
> @@ -44,6 +44,18 @@ _supported_fmt raw
>  _supported_proto generic
>  _supported_os Linux
>  
> +function do_run_qemu()
> +{
> +    echo Testing: "$@" | _filter_imgfmt
> +    $QEMU -nographic -qmp stdio -serial none "$@"
> +    echo
> +}
> +
> +function run_qemu()
> +{
> +    do_run_qemu "$@" 2>&1 | _filter_testdir | _filter_qmp | _filter_qemu_io
> +}
> +
>  quorum="file.driver=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.2.file.filename=$TEST_DIR/3.raw,file.vote-threshold=2"
> @@ -80,6 +92,45 @@ echo "== checking quorum correction =="
>  $QEMU_IO -c "open -o $quorum" -c "read -P 0x32 0 $size" | _filter_qemu_io
>  
>  echo
> +echo "== checking mixed reference/option specification =="
> +
> +run_qemu -drive "file=$TEST_DIR/2.raw,format=$IMGFMT,if=none,id=drive2" <<EOF
> +{ "execute": "qmp_capabilities" }
> +{ "execute": "blockdev-add",
> +    "arguments": {
> +        "options": {
> +            "driver": "quorum",
> +            "id": "drive0-quorum",
> +            "vote-threshold": 2,
> +            "children": [
> +                {
> +                    "driver": "raw",
> +                    "file": {
> +                        "driver": "file",
> +                        "filename": "$TEST_DIR/1.raw"
> +                    }
> +                },
> +                "drive2",
> +                {
> +                    "driver": "raw",
> +                    "file": {
> +                        "driver": "file",
> +                        "filename": "$TEST_DIR/3.raw"
> +                    }
> +                }
> +            ]
> +        }
> +    }
> +}
> +{ "execute": "human-monitor-command",
> +    "arguments": {
> +        "command-line": 'qemu-io drive0-quorum "read -P 0x32 0 $size"'
> +    }
> +}
> +{ "execute": "quit" }
> +EOF
> +
> +echo
>  echo "== breaking quorum =="
>  
>  $QEMU_IO -c "write -P 0x41 0 $size" "$TEST_DIR/1.raw" | _filter_qemu_io
> diff --git a/tests/qemu-iotests/081.out b/tests/qemu-iotests/081.out
> index b5b55ab..4fe2f95 100644
> --- a/tests/qemu-iotests/081.out
> +++ b/tests/qemu-iotests/081.out
> @@ -25,6 +25,21 @@ wrote 10485760/10485760 bytes at offset 0
>  read 10485760/10485760 bytes at offset 0
>  10 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
>  
> +== checking mixed reference/option specification ==
> +Testing: -drive file=TEST_DIR/2.IMGFMT,format=IMGFMT,if=none,id=drive2
> +QMP_VERSION
> +{"return": {}}
> +{"return": {}}
> +{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "QUORUM_REPORT_BAD", "data": {"node-name": "", "ret": 0, "sectors-count": 20480, "sector-num": 0}}
> +read 10485760/10485760 bytes at offset 0
> +10 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
> +{"return": ""}
> +{"return": {}}
> +{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "SHUTDOWN"}
> +{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "DEVICE_TRAY_MOVED", "data": {"device": "ide1-cd0", "tray-open": true}}
> +{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "DEVICE_TRAY_MOVED", "data": {"device": "floppy0", "tray-open": true}}
> +
> +
>  == breaking quorum ==
>  wrote 10485760/10485760 bytes at offset 0
>  10 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
> -- 
> 1.9.0
> 

That look good.

Thanks for doing this series.

Reviewed-by: Benoit Canet <benoit@irqsave.net>

Re: [Qemu-devel] [PATCH v2 0/2] quorum: Simplify quorum_open()

[Kevin Wolf]

Am 21.02.2014 um 22:30 hat Max Reitz geschrieben:
> This series employs the new ability of qdict_array_split() to not only
> split a QDict into a QList of QDicts (and a remainder QDict), but into a
> QList of QObjects of any kind, to simplify quorum_open(). This (in my
> opinion) enhances readability, robustness against malformatted options
> and allows specifying both reference strings and full option dicts at
> the same time.
> 
> This series depends on v19 of Benoît's Quorum series and on my "Extract
> non-QDicts in qdict_array_split()" series.

Thanks, applied to the block branch.

Kevin