* [Qemu-devel] [PATCH 0/2] quorum: Simplify quorum_open() @ 2014-02-21 19:44 Max Reitz 2014-02-21 19:44 ` [Qemu-devel] [PATCH 1/2] " Max Reitz 2014-02-21 19:44 ` [Qemu-devel] [PATCH 2/2] iotests: Mixed quorum child device specifications Max Reitz 0 siblings, 2 replies; 5+ messages in thread From: Max Reitz @ 2014-02-21 19:44 UTC (permalink / raw) To: qemu-devel; +Cc: Kevin Wolf, Benoît Canet, Stefan Hajnoczi, 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
* [Qemu-devel] [PATCH 1/2] quorum: Simplify quorum_open() 2014-02-21 19:44 [Qemu-devel] [PATCH 0/2] quorum: Simplify quorum_open() Max Reitz @ 2014-02-21 19:44 ` Max Reitz 2014-02-21 20:04 ` Benoît Canet 2014-02-21 19:44 ` [Qemu-devel] [PATCH 2/2] iotests: Mixed quorum child device specifications Max Reitz 1 sibling, 1 reply; 5+ messages in thread From: Max Reitz @ 2014-02-21 19:44 UTC (permalink / raw) To: qemu-devel; +Cc: Kevin Wolf, Benoît Canet, Stefan Hajnoczi, 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 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [Qemu-devel] [PATCH 1/2] quorum: Simplify quorum_open() 2014-02-21 19:44 ` [Qemu-devel] [PATCH 1/2] " Max Reitz @ 2014-02-21 20:04 ` Benoît Canet 0 siblings, 0 replies; 5+ messages in thread From: Benoît Canet @ 2014-02-21 20:04 UTC (permalink / raw) To: Max Reitz; +Cc: Kevin Wolf, qemu-devel, Stefan Hajnoczi 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 > ^ permalink raw reply [flat|nested] 5+ messages in thread
* [Qemu-devel] [PATCH 2/2] iotests: Mixed quorum child device specifications 2014-02-21 19:44 [Qemu-devel] [PATCH 0/2] quorum: Simplify quorum_open() Max Reitz 2014-02-21 19:44 ` [Qemu-devel] [PATCH 1/2] " Max Reitz @ 2014-02-21 19:44 ` Max Reitz 2014-02-21 20:29 ` Benoît Canet 1 sibling, 1 reply; 5+ messages in thread From: Max Reitz @ 2014-02-21 19:44 UTC (permalink / raw) To: qemu-devel; +Cc: Kevin Wolf, Benoît Canet, Stefan Hajnoczi, 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 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [Qemu-devel] [PATCH 2/2] iotests: Mixed quorum child device specifications 2014-02-21 19:44 ` [Qemu-devel] [PATCH 2/2] iotests: Mixed quorum child device specifications Max Reitz @ 2014-02-21 20:29 ` Benoît Canet 0 siblings, 0 replies; 5+ messages in thread From: Benoît Canet @ 2014-02-21 20:29 UTC (permalink / raw) To: Max Reitz; +Cc: Kevin Wolf, qemu-devel, Stefan Hajnoczi 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 > > ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2014-02-21 20:29 UTC | newest] Thread overview: 5+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2014-02-21 19:44 [Qemu-devel] [PATCH 0/2] quorum: Simplify quorum_open() Max Reitz 2014-02-21 19:44 ` [Qemu-devel] [PATCH 1/2] " Max Reitz 2014-02-21 20:04 ` Benoît Canet 2014-02-21 19:44 ` [Qemu-devel] [PATCH 2/2] iotests: Mixed quorum child device specifications Max Reitz 2014-02-21 20:29 ` Benoît Canet
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).