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

[Qemu-devel] [PATCH 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 v18 of Benoît's Quorum series and on my "Extract
non-QDicts in qdict_array_split()" series.


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

 block/quorum.c             | 62 ++++++++++++++++++++++++++++------------------
 tests/qemu-iotests/081     | 52 ++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/081.out | 15 +++++++++++
 3 files changed, 105 insertions(+), 24 deletions(-)

-- 
1.9.0

[Qemu-devel] [PATCH 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>
---
 block/quorum.c | 62 +++++++++++++++++++++++++++++++++++-----------------------
 1 file changed, 38 insertions(+), 24 deletions(-)

diff --git a/block/quorum.c b/block/quorum.c
index 18721ba..8f5833d 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,17 @@ static int quorum_open(BlockDriverState *bs, QDict *options, int flags,
     qdict_extract_subqdict(options, &sub, "children.");
     qdict_array_split(sub, &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 and validate
      * qdict_size(sub) address the open by reference case
      */
-    s->num_children = !qlist_size(list) ? qdict_size(sub) : qlist_size(list);
+    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 +773,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 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     | 52 ++++++++++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/081.out | 15 +++++++++++++
 2 files changed, 67 insertions(+)

diff --git a/tests/qemu-iotests/081 b/tests/qemu-iotests/081
index be34544..421c7cc 100755
--- a/tests/qemu-iotests/081
+++ b/tests/qemu-iotests/081
@@ -44,6 +44,19 @@ _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 | \
+        sed -e 's/"sector-num": [0-9]*/"sector-num": X/'
+}
+
 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 +93,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..3bf882b 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": "%s", "ret": false, "sectors-count": false, "sector-num": X}}
+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 1/2] quorum: Simplify quorum_open()

[Benoît Canet]

The Friday 21 Feb 2014 à 20:44:07 (+0100), Max Reitz wrote :
> 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>
> ---
>  block/quorum.c | 62 +++++++++++++++++++++++++++++++++++-----------------------
>  1 file changed, 38 insertions(+), 24 deletions(-)
> 
> diff --git a/block/quorum.c b/block/quorum.c
> index 18721ba..8f5833d 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,17 @@ static int quorum_open(BlockDriverState *bs, QDict *options, int flags,
>      qdict_extract_subqdict(options, &sub, "children.");
>      qdict_array_split(sub, &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 and validate
>       * qdict_size(sub) address the open by reference case
>       */
I think the second part of the comment about qlist_size(sub) is an extra now.

Aside from that:

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

> -    s->num_children = !qlist_size(list) ? qdict_size(sub) : qlist_size(list);
> +    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 +773,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
> 

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

[Benoît Canet]

The Friday 21 Feb 2014 à 20:44:08 (+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     | 52 ++++++++++++++++++++++++++++++++++++++++++++++
>  tests/qemu-iotests/081.out | 15 +++++++++++++
>  2 files changed, 67 insertions(+)
> 
> diff --git a/tests/qemu-iotests/081 b/tests/qemu-iotests/081
> index be34544..421c7cc 100755
> --- a/tests/qemu-iotests/081
> +++ b/tests/qemu-iotests/081
> @@ -44,6 +44,19 @@ _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 | \
> +        sed -e 's/"sector-num": [0-9]*/"sector-num": X/'
> +}
> +
>  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 +93,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..3bf882b 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": "%s", "ret": false, "sectors-count": false, "sector-num": X}}
I think I wrote something wrong with the QMP events since you got
"node-name": "%s".

Will check that.

Best regards

Benoît

> +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
> 
>