[Qemu-devel] [PATCH v6 00/22] blkdebug/blkverify: Allow QMP configuration

[Qemu-devel] [PATCH v6 00/22] blkdebug/blkverify: Allow QMP configuration

[Max Reitz]

Currently, the configuration of blkdebug and blkverify is done through
the "filename" alone. There is now way of manually choosing blkdebug or
blkverify as a driver and using a normal image filename.

In the case of blkdebug, the filename starts with the protocol prefix,
follows up with the name of a configuration file and ends with the name
of the image file.

In the case of blkverify, the filename starts with the protocol prefix,
follows up with the raw reference image filename and ends with the name
of the image file.

This patch allows the configuration of both drivers completely through
QMP and accordingly command-line options. The driver has to be selected
through the driver option (or similar), the image filename may be given
either as the filename itself or through a x.filename option, where "x"
depends on the driver. Further options may be required depending on the
driver.

In case of blkverify, the test image may be specified either through the
filename or as a BlockdevRef reference through the "test" option. The
raw image is referenced as "raw".

In case of blkdebug, one may either set the "config" option to the
filename of a configuration file, or the content of the configuration
file may be given directly (as options). The image filename is either
specified as the filename or referenced through the "image" option.


v6:
 - Addressed Kevin's and Erik's comments (first number is patch number
   in v5, second is patch number in this series):
    - Patch 4/4: Adjusted comment
    - Patch 5/-: Dropped
    - Patch 11/10: Guard against !drv && !file before
      find_image_format()
    - Patch -/11: Added
    - Patch -/12: Added
    - Patch 12/13: Removed unnecessary hunk and squashed next
      patch into this one
    - Patch 13/-: Squashed into previous patch
    - Patch 15/15: Dropped static open_image() in favor of the global
      bdrv_open_image()
    - Patch 16/-: Dropped
    - Patch 17/16: Dropped static open_image() in favor of the global
      bdrv_open_image(); also, disable format auto-detection for the raw
      image
    - Patch 18/17: Drop check for !filename since the filename is always
      passed to this function
    - Patch 19/18: Add "errno" to the list of polluted words in qapi.py
      rather than introduce a new alias
    - Patch 20/19: Use enumeration for the blkdebug trigger events;
      also, we can and must use "errno" now instead of "error"
    - Patch 22/21: Fix test by increasing the image size; add actually
      working tests for blkverify failures; add test for blkverify using
      an already existing block device as the raw image
    - Patch -/22: Added

v5:
 - Addressed Fam's comments:
    - Patch 4: Asserted against prefix == NULL in qdict_flatten_qlist()
    - Patch 4: Support other types than QDicts and QLists in QLists in
      qdict_flatten_qlist()
    - Patch 4: Modified comment above qdict_flatten() to include the
      functionality of qdict_flatten_qlist()
    - Patch 8 (now patch 9): Removed goto fail; in favor of directly
      returning in the if (reference) patch - this fixes a double
      QDECREF(options) as well as two bs dereferences with bs being NULL
    - Patch 21 (now patch 22): Added reason for not using Python to the
      commit message
 - Added patch 5: Cleans up qdict_flatten_qdict() which contains a now
   unused delete variable

v4:
 - Fixed a memleak in qdict_flatten_qlist() in patch 4

v3:
 - The first few patches are probably similar to the ones from the
   previous series; but it's probably best to see this series as a
   completely new one.


Key:
[----] : patches are identical
[####] : number of functional differences between upstream/downstream
patch
[down] : patch is downstream-only
The flags [FC] indicate (F)unctional and (C)ontextual differences,
respectively

001/22:[----] [--] 'blkdebug: Use errp for read_config()'
002/22:[----] [--] 'blkdebug: Don't require sophisticated filename'
003/22:[----] [--] 'qdict: Add qdict_array_split()'
004/22:[0003] [FC] 'qapi: extend qdict_flatten() for QLists'
005/22:[----] [--] 'qemu-option: Add qemu_config_parse_qdict()'
006/22:[----] [--] 'blkdebug: Always call read_config()'
007/22:[----] [--] 'blkdebug: Use command-line in read_config()'
008/22:[----] [--] 'block: Allow reference for bdrv_file_open()'
009/22:[----] [--] 'block: Pass reference to bdrv_file_open()'
010/22:[0008] [FC] 'block: Allow block devices without files'
011/22:[down] 'block: Add bdrv_open_image()'
012/22:[down] 'block: Use bdrv_open_image() in bdrv_open()'
013/22:[0010] [FC] 'block: Allow recursive "file"s'
014/22:[----] [--] 'blockdev: Move "file" to legacy_opts'
015/22:[0026] [FC] 'blkdebug: Allow command-line file configuration'
016/22:[0042] [FC] 'blkverify: Allow command-line configuration'
017/22:[0006] [FC] 'blkverify: Don't require protocol filename'
018/22:[down] 'qapi: Add "errno" to the list of polluted words'
019/22:[0031] [FC] 'qapi: QMP interface for blkdebug and blkverify'
020/22:[----] [--] 'qemu-io: Make filename optional'
021/22:[0083] [FC] 'iotests: Test new blkdebug/blkverify interface'
022/22:[down] 'iotests: Test file format nesting'


Max Reitz (22):
  blkdebug: Use errp for read_config()
  blkdebug: Don't require sophisticated filename
  qdict: Add qdict_array_split()
  qapi: extend qdict_flatten() for QLists
  qemu-option: Add qemu_config_parse_qdict()
  blkdebug: Always call read_config()
  blkdebug: Use command-line in read_config()
  block: Allow reference for bdrv_file_open()
  block: Pass reference to bdrv_file_open()
  block: Allow block devices without files
  block: Add bdrv_open_image()
  block: Use bdrv_open_image() in bdrv_open()
  block: Allow recursive "file"s
  blockdev: Move "file" to legacy_opts
  blkdebug: Allow command-line file configuration
  blkverify: Allow command-line configuration
  blkverify: Don't require protocol filename
  qapi: Add "errno" to the list of polluted words
  qapi: QMP interface for blkdebug and blkverify
  qemu-io: Make filename optional
  iotests: Test new blkdebug/blkverify interface
  iotests: Test file format nesting

 block.c                    | 123 ++++++++++++++++++++---
 block/blkdebug.c           |  59 ++++++-----
 block/blkverify.c          |  29 ++----
 block/cow.c                |   3 +-
 block/qcow.c               |   3 +-
 block/qcow2.c              |   2 +-
 block/qed.c                |   4 +-
 block/sheepdog.c           |   4 +-
 block/vhdx.c               |   2 +-
 block/vmdk.c               |   4 +-
 blockdev.c                 |  19 ++--
 include/block/block.h      |   6 +-
 include/qapi/qmp/qdict.h   |   1 +
 include/qemu/config-file.h |   6 ++
 qapi-schema.json           | 113 ++++++++++++++++++++-
 qemu-io.c                  |  10 +-
 qobject/qdict.c            |  93 ++++++++++++++++--
 scripts/qapi.py            |   2 +-
 tests/qemu-iotests/051.out |   2 +-
 tests/qemu-iotests/071     | 239 +++++++++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/071.out |  90 +++++++++++++++++
 tests/qemu-iotests/072     |  69 +++++++++++++
 tests/qemu-iotests/072.out |  21 ++++
 tests/qemu-iotests/group   |   2 +
 util/qemu-config.c         |  95 ++++++++++++++++++
 25 files changed, 908 insertions(+), 93 deletions(-)
 create mode 100755 tests/qemu-iotests/071
 create mode 100644 tests/qemu-iotests/071.out
 create mode 100755 tests/qemu-iotests/072
 create mode 100644 tests/qemu-iotests/072.out

-- 
1.8.5.1

[Qemu-devel] [PATCH v6 01/22] blkdebug: Use errp for read_config()

[Max Reitz]

Use an Error variable in the read_config() function.

Signed-off-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: Kevin Wolf <kwolf@redhat.com>
---
 block/blkdebug.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/block/blkdebug.c b/block/blkdebug.c
index 957be2c..627e29d 100644
--- a/block/blkdebug.c
+++ b/block/blkdebug.c
@@ -271,7 +271,7 @@ static void remove_rule(BlkdebugRule *rule)
     g_free(rule);
 }
 
-static int read_config(BDRVBlkdebugState *s, const char *filename)
+static int read_config(BDRVBlkdebugState *s, const char *filename, Error **errp)
 {
     FILE *f;
     int ret;
@@ -279,11 +279,14 @@ static int read_config(BDRVBlkdebugState *s, const char *filename)
 
     f = fopen(filename, "r");
     if (f == NULL) {
+        error_setg_errno(errp, errno, "Could not read blkdebug config file");
         return -errno;
     }
 
     ret = qemu_config_parse(f, config_groups, filename);
     if (ret < 0) {
+        error_setg(errp, "Could not parse blkdebug config file");
+        ret = -EINVAL;
         goto fail;
     }
 
@@ -370,9 +373,8 @@ static int blkdebug_open(BlockDriverState *bs, QDict *options, int flags,
     /* Read rules from config file */
     config = qemu_opt_get(opts, "config");
     if (config) {
-        ret = read_config(s, config);
-        if (ret < 0) {
-            error_setg_errno(errp, -ret, "Could not read blkdebug config file");
+        ret = read_config(s, config, errp);
+        if (ret) {
             goto fail;
         }
     }
-- 
1.8.5.1

[Qemu-devel] [PATCH v6 02/22] blkdebug: Don't require sophisticated filename

[Max Reitz]

If the filename is not prefixed by "blkdebug:" in
blkdebug_parse_filename(), the blkdebug driver was not selected through
that protocol prefix, but by an explicit command line option
(file.driver=blkdebug or something similar). Contrary to the current
reaction, this is not a problem at all; we just need to store the
filename (in the x-image option) and can go on; the user just has to
manually specify the config option.

Signed-off-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: Kevin Wolf <kwolf@redhat.com>
---
 block/blkdebug.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/block/blkdebug.c b/block/blkdebug.c
index 627e29d..a2301d7 100644
--- a/block/blkdebug.c
+++ b/block/blkdebug.c
@@ -313,7 +313,9 @@ static void blkdebug_parse_filename(const char *filename, QDict *options,
 
     /* Parse the blkdebug: prefix */
     if (!strstart(filename, "blkdebug:", &filename)) {
-        error_setg(errp, "File name string must start with 'blkdebug:'");
+        /* There was no prefix; therefore, all options have to be already
+           present in the QDict (except for the filename) */
+        qdict_put(options, "x-image", qstring_from_str(filename));
         return;
     }
 
-- 
1.8.5.1

[Qemu-devel] [PATCH v6 03/22] qdict: Add qdict_array_split()

[Max Reitz]

This function splits a QDict consisting of entries prefixed by
incrementally enumerated indices into a QList of QDicts.

Signed-off-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: Kevin Wolf <kwolf@redhat.com>
---
 include/qapi/qmp/qdict.h |  1 +
 qobject/qdict.c          | 36 ++++++++++++++++++++++++++++++++++++
 2 files changed, 37 insertions(+)

diff --git a/include/qapi/qmp/qdict.h b/include/qapi/qmp/qdict.h
index 5cefd80..1ddf97b 100644
--- a/include/qapi/qmp/qdict.h
+++ b/include/qapi/qmp/qdict.h
@@ -68,5 +68,6 @@ QDict *qdict_clone_shallow(const QDict *src);
 void qdict_flatten(QDict *qdict);
 
 void qdict_extract_subqdict(QDict *src, QDict **dst, const char *start);
+void qdict_array_split(QDict *src, QList **dst);
 
 #endif /* QDICT_H */
diff --git a/qobject/qdict.c b/qobject/qdict.c
index 17e14f0..fca1902 100644
--- a/qobject/qdict.c
+++ b/qobject/qdict.c
@@ -554,3 +554,39 @@ void qdict_extract_subqdict(QDict *src, QDict **dst, const char *start)
         entry = next;
     }
 }
+
+/**
+ * qdict_array_split(): This function creates a QList of QDicts. Every entry in
+ * the original QDict with a key prefixed "%u.", where %u designates an unsigned
+ * integer starting at 0 and incrementally counting up, will be moved to a new
+ * QDict at index %u in the output QList with the key prefix removed. The
+ * function terminates when there is no entry in the QDict with a prefix
+ * directly (incrementally) following the last one.
+ * Example: {"0.a": 42, "0.b": 23, "1.x": 0, "3.y": 1, "o.o": 7}
+ *      (or {"1.x": 0, "3.y": 1, "0.a": 42, "o.o": 7, "0.b": 23})
+ *       => [{"a": 42, "b": 23}, {"x": 0}]
+ *      and {"3.y": 1, "o.o": 7} (remainder of the old QDict)
+ */
+void qdict_array_split(QDict *src, QList **dst)
+{
+    unsigned i;
+
+    *dst = qlist_new();
+
+    for (i = 0; i < UINT_MAX; i++) {
+        QDict *subqdict;
+        char prefix[32];
+        size_t snprintf_ret;
+
+        snprintf_ret = snprintf(prefix, 32, "%u.", i);
+        assert(snprintf_ret < 32);
+
+        qdict_extract_subqdict(src, &subqdict, prefix);
+        if (!qdict_size(subqdict)) {
+            QDECREF(subqdict);
+            break;
+        }
+
+        qlist_append_obj(*dst, QOBJECT(subqdict));
+    }
+}
-- 
1.8.5.1

[Qemu-devel] [PATCH v6 04/22] qapi: extend qdict_flatten() for QLists

[Max Reitz]

Reversing qdict_array_split(), qdict_flatten() should flatten QLists as
well by interpreting them as QDicts where every entry's key is its
index.

This allows bringing QDicts with QLists from QMP commands to the same
form as they would be given as command-line options, thereby allowing
them to be parsed the same way.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 qobject/qdict.c | 57 +++++++++++++++++++++++++++++++++++++++++++++++++++------
 1 file changed, 51 insertions(+), 6 deletions(-)

diff --git a/qobject/qdict.c b/qobject/qdict.c
index fca1902..7b6b08a 100644
--- a/qobject/qdict.c
+++ b/qobject/qdict.c
@@ -477,7 +477,46 @@ static void qdict_destroy_obj(QObject *obj)
     g_free(qdict);
 }
 
-static void qdict_do_flatten(QDict *qdict, QDict *target, const char *prefix)
+static void qdict_flatten_qdict(QDict *qdict, QDict *target,
+                                const char *prefix);
+
+static void qdict_flatten_qlist(QList *qlist, QDict *target, const char *prefix)
+{
+    QObject *value;
+    const QListEntry *entry;
+    char *new_key;
+    int i;
+
+    /* This function is never called with prefix == NULL, i.e., it is always
+     * called from within qdict_flatten_q(list|dict)(). Therefore, it does not
+     * need to remove list entries during the iteration (the whole list will be
+     * deleted eventually anyway from qdict_flatten_qdict()). */
+    assert(prefix);
+
+    entry = qlist_first(qlist);
+
+    for (i = 0; entry; entry = qlist_next(entry), i++) {
+        value = qlist_entry_obj(entry);
+
+        qobject_incref(value);
+        new_key = g_strdup_printf("%s.%i", prefix, i);
+        qdict_put_obj(target, new_key, value);
+
+        if (qobject_type(value) == QTYPE_QDICT) {
+            qdict_flatten_qdict(qobject_to_qdict(value), target, new_key);
+        } else if (qobject_type(value) == QTYPE_QLIST) {
+            qdict_flatten_qlist(qobject_to_qlist(value), target, new_key);
+        } else {
+            /* All other types are moved to the target unchanged. */
+            qobject_incref(value);
+            qdict_put_obj(target, new_key, value);
+        }
+
+        g_free(new_key);
+    }
+}
+
+static void qdict_flatten_qdict(QDict *qdict, QDict *target, const char *prefix)
 {
     QObject *value;
     const QDictEntry *entry, *next;
@@ -500,8 +539,12 @@ static void qdict_do_flatten(QDict *qdict, QDict *target, const char *prefix)
         if (qobject_type(value) == QTYPE_QDICT) {
             /* Entries of QDicts are processed recursively, the QDict object
              * itself disappears. */
-            qdict_do_flatten(qobject_to_qdict(value), target,
-                             new_key ? new_key : entry->key);
+            qdict_flatten_qdict(qobject_to_qdict(value), target,
+                                new_key ? new_key : entry->key);
+            delete = true;
+        } else if (qobject_type(value) == QTYPE_QLIST) {
+            qdict_flatten_qlist(qobject_to_qlist(value), target,
+                                new_key ? new_key : entry->key);
             delete = true;
         } else if (prefix) {
             /* All other objects are moved to the target unchanged. */
@@ -526,12 +569,14 @@ static void qdict_do_flatten(QDict *qdict, QDict *target, const char *prefix)
 
 /**
  * qdict_flatten(): For each nested QDict with key x, all fields with key y
- * are moved to this QDict and their key is renamed to "x.y". This operation
- * is applied recursively for nested QDicts.
+ * are moved to this QDict and their key is renamed to "x.y". For each nested
+ * QList with key x, the field at index y is moved to this QDict with the key
+ * "x.y" (i.e., the reverse of what qdict_array_split() does).
+ * This operation is applied recursively for nested QDicts and QLists.
  */
 void qdict_flatten(QDict *qdict)
 {
-    qdict_do_flatten(qdict, qdict, NULL);
+    qdict_flatten_qdict(qdict, qdict, NULL);
 }
 
 /* extract all the src QDict entries starting by start into dst */
-- 
1.8.5.1

[Qemu-devel] [PATCH v6 05/22] qemu-option: Add qemu_config_parse_qdict()

[Max Reitz]

This function basically parses command-line options given as a QDict
replacing a config file.

For instance, the QDict {"section.opt1": 42, "section.opt2": 23}
corresponds to the config file:

[section]
opt1 = 42
opt2 = 23

It is possible to specify multiple sections and also multiple sections
of the same type. On the command line, this looks like the following:

inject-error.0.event=reftable_load,\
inject-error.1.event=l2_load,\
set-state.event=l1_update

This would correspond to the following config file:

[inject-error "inject-error.0"]
event = reftable_load

[inject-error "inject-error.1"]
event = l2_load

[set-state]
event = l1_update

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 include/qemu/config-file.h |  6 +++
 util/qemu-config.c         | 95 ++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 101 insertions(+)

diff --git a/include/qemu/config-file.h b/include/qemu/config-file.h
index 508428f..dbd97c4 100644
--- a/include/qemu/config-file.h
+++ b/include/qemu/config-file.h
@@ -4,6 +4,7 @@
 #include <stdio.h>
 #include "qemu/option.h"
 #include "qapi/error.h"
+#include "qapi/qmp/qdict.h"
 
 QemuOptsList *qemu_find_opts(const char *group);
 QemuOptsList *qemu_find_opts_err(const char *group, Error **errp);
@@ -18,6 +19,11 @@ int qemu_config_parse(FILE *fp, QemuOptsList **lists, const char *fname);
 
 int qemu_read_config_file(const char *filename);
 
+/* Parse QDict options as a replacement for a config file (allowing multiple
+   enumerated (0..(n-1)) configuration "sections") */
+void qemu_config_parse_qdict(QDict *options, QemuOptsList **lists,
+                             Error **errp);
+
 /* Read default QEMU config files
  */
 int qemu_read_default_config_files(bool userconfig);
diff --git a/util/qemu-config.c b/util/qemu-config.c
index 04da942..12178d4 100644
--- a/util/qemu-config.c
+++ b/util/qemu-config.c
@@ -356,3 +356,98 @@ int qemu_read_config_file(const char *filename)
         return -EINVAL;
     }
 }
+
+static void config_parse_qdict_section(QDict *options, QemuOptsList *opts,
+                                       Error **errp)
+{
+    QemuOpts *subopts;
+    QDict *subqdict;
+    QList *list = NULL;
+    Error *local_err = NULL;
+    size_t orig_size, enum_size;
+    char *prefix;
+
+    prefix = g_strdup_printf("%s.", opts->name);
+    qdict_extract_subqdict(options, &subqdict, prefix);
+    g_free(prefix);
+    orig_size = qdict_size(subqdict);
+    if (!orig_size) {
+        goto out;
+    }
+
+    subopts = qemu_opts_create_nofail(opts);
+    qemu_opts_absorb_qdict(subopts, subqdict, &local_err);
+    if (error_is_set(&local_err)) {
+        error_propagate(errp, local_err);
+        goto out;
+    }
+
+    enum_size = qdict_size(subqdict);
+    if (enum_size < orig_size && enum_size) {
+        error_setg(errp, "Unknown option '%s' for [%s]",
+                   qdict_first(subqdict)->key, opts->name);
+        goto out;
+    }
+
+    if (enum_size) {
+        /* Multiple, enumerated sections */
+        QListEntry *list_entry;
+        unsigned i = 0;
+
+        /* Not required anymore */
+        qemu_opts_del(subopts);
+
+        qdict_array_split(subqdict, &list);
+        if (qdict_size(subqdict)) {
+            error_setg(errp, "Unused option '%s' for [%s]",
+                       qdict_first(subqdict)->key, opts->name);
+            goto out;
+        }
+
+        QLIST_FOREACH_ENTRY(list, list_entry) {
+            QDict *section = qobject_to_qdict(qlist_entry_obj(list_entry));
+            char *opt_name;
+
+            opt_name = g_strdup_printf("%s.%u", opts->name, i++);
+            subopts = qemu_opts_create(opts, opt_name, 1, &local_err);
+            g_free(opt_name);
+            if (error_is_set(&local_err)) {
+                error_propagate(errp, local_err);
+                goto out;
+            }
+
+            qemu_opts_absorb_qdict(subopts, section, &local_err);
+            if (error_is_set(&local_err)) {
+                error_propagate(errp, local_err);
+                qemu_opts_del(subopts);
+                goto out;
+            }
+
+            if (qdict_size(section)) {
+                error_setg(errp, "[%s] section doesn't support the option '%s'",
+                           opts->name, qdict_first(section)->key);
+                qemu_opts_del(subopts);
+                goto out;
+            }
+        }
+    }
+
+out:
+    QDECREF(subqdict);
+    QDECREF(list);
+}
+
+void qemu_config_parse_qdict(QDict *options, QemuOptsList **lists,
+                             Error **errp)
+{
+    int i;
+    Error *local_err = NULL;
+
+    for (i = 0; lists[i]; i++) {
+        config_parse_qdict_section(options, lists[i], &local_err);
+        if (error_is_set(&local_err)) {
+            error_propagate(errp, local_err);
+            return;
+        }
+    }
+}
-- 
1.8.5.1

[Qemu-devel] [PATCH v6 06/22] blkdebug: Always call read_config()

[Max Reitz]

Move the check whether there actually is a config file into the
read_config() function.

Signed-off-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: Kevin Wolf <kwolf@redhat.com>
---
 block/blkdebug.c | 36 +++++++++++++++++++-----------------
 1 file changed, 19 insertions(+), 17 deletions(-)

diff --git a/block/blkdebug.c b/block/blkdebug.c
index a2301d7..5647467 100644
--- a/block/blkdebug.c
+++ b/block/blkdebug.c
@@ -273,21 +273,23 @@ static void remove_rule(BlkdebugRule *rule)
 
 static int read_config(BDRVBlkdebugState *s, const char *filename, Error **errp)
 {
-    FILE *f;
+    FILE *f = NULL;
     int ret;
     struct add_rule_data d;
 
-    f = fopen(filename, "r");
-    if (f == NULL) {
-        error_setg_errno(errp, errno, "Could not read blkdebug config file");
-        return -errno;
-    }
+    if (filename) {
+        f = fopen(filename, "r");
+        if (f == NULL) {
+            error_setg_errno(errp, errno, "Could not read blkdebug config file");
+            return -errno;
+        }
 
-    ret = qemu_config_parse(f, config_groups, filename);
-    if (ret < 0) {
-        error_setg(errp, "Could not parse blkdebug config file");
-        ret = -EINVAL;
-        goto fail;
+        ret = qemu_config_parse(f, config_groups, filename);
+        if (ret < 0) {
+            error_setg(errp, "Could not parse blkdebug config file");
+            ret = -EINVAL;
+            goto fail;
+        }
     }
 
     d.s = s;
@@ -301,7 +303,9 @@ static int read_config(BDRVBlkdebugState *s, const char *filename, Error **errp)
 fail:
     qemu_opts_reset(&inject_error_opts);
     qemu_opts_reset(&set_state_opts);
-    fclose(f);
+    if (f) {
+        fclose(f);
+    }
     return ret;
 }
 
@@ -374,11 +378,9 @@ static int blkdebug_open(BlockDriverState *bs, QDict *options, int flags,
 
     /* Read rules from config file */
     config = qemu_opt_get(opts, "config");
-    if (config) {
-        ret = read_config(s, config, errp);
-        if (ret) {
-            goto fail;
-        }
+    ret = read_config(s, config, errp);
+    if (ret) {
+        goto fail;
     }
 
     /* Set initial state */
-- 
1.8.5.1

[Qemu-devel] [PATCH v6 07/22] blkdebug: Use command-line in read_config()

[Max Reitz]

Use qemu_config_parse_qdict() to parse the command-line options in
addition to the config file.

Signed-off-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: Kevin Wolf <kwolf@redhat.com>
---
 block/blkdebug.c | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

diff --git a/block/blkdebug.c b/block/blkdebug.c
index 5647467..08ea88c 100644
--- a/block/blkdebug.c
+++ b/block/blkdebug.c
@@ -271,11 +271,13 @@ static void remove_rule(BlkdebugRule *rule)
     g_free(rule);
 }
 
-static int read_config(BDRVBlkdebugState *s, const char *filename, Error **errp)
+static int read_config(BDRVBlkdebugState *s, const char *filename,
+                       QDict *options, Error **errp)
 {
     FILE *f = NULL;
     int ret;
     struct add_rule_data d;
+    Error *local_err = NULL;
 
     if (filename) {
         f = fopen(filename, "r");
@@ -292,6 +294,13 @@ static int read_config(BDRVBlkdebugState *s, const char *filename, Error **errp)
         }
     }
 
+    qemu_config_parse_qdict(options, config_groups, &local_err);
+    if (error_is_set(&local_err)) {
+        error_propagate(errp, local_err);
+        ret = -EINVAL;
+        goto fail;
+    }
+
     d.s = s;
     d.action = ACTION_INJECT_ERROR;
     qemu_opts_foreach(&inject_error_opts, add_rule, &d, 0);
@@ -376,9 +385,9 @@ static int blkdebug_open(BlockDriverState *bs, QDict *options, int flags,
         goto fail;
     }
 
-    /* Read rules from config file */
+    /* Read rules from config file or command line options */
     config = qemu_opt_get(opts, "config");
-    ret = read_config(s, config, errp);
+    ret = read_config(s, config, options, errp);
     if (ret) {
         goto fail;
     }
-- 
1.8.5.1

[Qemu-devel] [PATCH v6 08/22] block: Allow reference for bdrv_file_open()

[Max Reitz]

Allow specifying a reference to an existing block device (by name) for
bdrv_file_open() instead of a filename and/or options.

Signed-off-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: Kevin Wolf <kwolf@redhat.com>
---
 block.c               | 25 ++++++++++++++++++++++---
 block/blkdebug.c      |  2 +-
 block/blkverify.c     |  2 +-
 block/cow.c           |  3 ++-
 block/qcow.c          |  3 ++-
 block/qcow2.c         |  2 +-
 block/qed.c           |  4 ++--
 block/sheepdog.c      |  4 ++--
 block/vhdx.c          |  2 +-
 block/vmdk.c          |  4 ++--
 include/block/block.h |  3 ++-
 qemu-io.c             |  2 +-
 12 files changed, 39 insertions(+), 17 deletions(-)

diff --git a/block.c b/block.c
index 64e7d22..1e53b3d 100644
--- a/block.c
+++ b/block.c
@@ -858,9 +858,10 @@ free_and_fail:
  * dictionary, it needs to use QINCREF() before calling bdrv_file_open.
  */
 int bdrv_file_open(BlockDriverState **pbs, const char *filename,
-                   QDict *options, int flags, Error **errp)
+                   const char *reference, QDict *options, int flags,
+                   Error **errp)
 {
-    BlockDriverState *bs;
+    BlockDriverState *bs = NULL;
     BlockDriver *drv;
     const char *drvname;
     bool allow_protocol_prefix = false;
@@ -872,6 +873,24 @@ int bdrv_file_open(BlockDriverState **pbs, const char *filename,
         options = qdict_new();
     }
 
+    if (reference) {
+        if (filename || qdict_size(options)) {
+            error_setg(errp, "Cannot reference an existing block device with "
+                       "additional options or a new filename");
+            return -EINVAL;
+        }
+        QDECREF(options);
+
+        bs = bdrv_find(reference);
+        if (!bs) {
+            error_setg(errp, "Cannot find block device '%s'", reference);
+            return -ENODEV;
+        }
+        bdrv_ref(bs);
+        *pbs = bs;
+        return 0;
+    }
+
     bs = bdrv_new("");
     bs->options = options;
     options = qdict_clone_shallow(options);
@@ -1124,7 +1143,7 @@ int bdrv_open(BlockDriverState *bs, const char *filename, QDict *options,
 
     qdict_extract_subqdict(options, &file_options, "file.");
 
-    ret = bdrv_file_open(&file, filename, file_options,
+    ret = bdrv_file_open(&file, filename, NULL, file_options,
                          bdrv_open_flags(bs, flags | BDRV_O_UNMAP), &local_err);
     if (ret < 0) {
         goto fail;
diff --git a/block/blkdebug.c b/block/blkdebug.c
index 08ea88c..c73a6cf 100644
--- a/block/blkdebug.c
+++ b/block/blkdebug.c
@@ -403,7 +403,7 @@ static int blkdebug_open(BlockDriverState *bs, QDict *options, int flags,
         goto fail;
     }
 
-    ret = bdrv_file_open(&bs->file, filename, NULL, flags, &local_err);
+    ret = bdrv_file_open(&bs->file, filename, NULL, NULL, flags, &local_err);
     if (ret < 0) {
         error_propagate(errp, local_err);
         goto fail;
diff --git a/block/blkverify.c b/block/blkverify.c
index 3c63528..c6eb287 100644
--- a/block/blkverify.c
+++ b/block/blkverify.c
@@ -141,7 +141,7 @@ static int blkverify_open(BlockDriverState *bs, QDict *options, int flags,
         goto fail;
     }
 
-    ret = bdrv_file_open(&bs->file, raw, NULL, flags, &local_err);
+    ret = bdrv_file_open(&bs->file, raw, NULL, NULL, flags, &local_err);
     if (ret < 0) {
         error_propagate(errp, local_err);
         goto fail;
diff --git a/block/cow.c b/block/cow.c
index dc15e46..7fc0b12 100644
--- a/block/cow.c
+++ b/block/cow.c
@@ -351,7 +351,8 @@ static int cow_create(const char *filename, QEMUOptionParameter *options,
         return ret;
     }
 
-    ret = bdrv_file_open(&cow_bs, filename, NULL, BDRV_O_RDWR, &local_err);
+    ret = bdrv_file_open(&cow_bs, filename, NULL, NULL, BDRV_O_RDWR,
+                         &local_err);
     if (ret < 0) {
         qerror_report_err(local_err);
         error_free(local_err);
diff --git a/block/qcow.c b/block/qcow.c
index c470e05..948b0c5 100644
--- a/block/qcow.c
+++ b/block/qcow.c
@@ -691,7 +691,8 @@ static int qcow_create(const char *filename, QEMUOptionParameter *options,
         return ret;
     }
 
-    ret = bdrv_file_open(&qcow_bs, filename, NULL, BDRV_O_RDWR, &local_err);
+    ret = bdrv_file_open(&qcow_bs, filename, NULL, NULL, BDRV_O_RDWR,
+                         &local_err);
     if (ret < 0) {
         qerror_report_err(local_err);
         error_free(local_err);
diff --git a/block/qcow2.c b/block/qcow2.c
index f29aa88..62e3f6a 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -1483,7 +1483,7 @@ static int qcow2_create2(const char *filename, int64_t total_size,
         return ret;
     }
 
-    ret = bdrv_file_open(&bs, filename, NULL, BDRV_O_RDWR, &local_err);
+    ret = bdrv_file_open(&bs, filename, NULL, NULL, BDRV_O_RDWR, &local_err);
     if (ret < 0) {
         error_propagate(errp, local_err);
         return ret;
diff --git a/block/qed.c b/block/qed.c
index 450a1fa..0dd5c58 100644
--- a/block/qed.c
+++ b/block/qed.c
@@ -563,8 +563,8 @@ static int qed_create(const char *filename, uint32_t cluster_size,
         return ret;
     }
 
-    ret = bdrv_file_open(&bs, filename, NULL, BDRV_O_RDWR | BDRV_O_CACHE_WB,
-                         &local_err);
+    ret = bdrv_file_open(&bs, filename, NULL, NULL,
+                         BDRV_O_RDWR | BDRV_O_CACHE_WB, &local_err);
     if (ret < 0) {
         qerror_report_err(local_err);
         error_free(local_err);
diff --git a/block/sheepdog.c b/block/sheepdog.c
index d1c812d..8d6e940 100644
--- a/block/sheepdog.c
+++ b/block/sheepdog.c
@@ -1534,7 +1534,7 @@ static int sd_prealloc(const char *filename)
     Error *local_err = NULL;
     int ret;
 
-    ret = bdrv_file_open(&bs, filename, NULL, BDRV_O_RDWR, &local_err);
+    ret = bdrv_file_open(&bs, filename, NULL, NULL, BDRV_O_RDWR, &local_err);
     if (ret < 0) {
         qerror_report_err(local_err);
         error_free(local_err);
@@ -1695,7 +1695,7 @@ static int sd_create(const char *filename, QEMUOptionParameter *options,
             goto out;
         }
 
-        ret = bdrv_file_open(&bs, backing_file, NULL, 0, &local_err);
+        ret = bdrv_file_open(&bs, backing_file, NULL, NULL, 0, &local_err);
         if (ret < 0) {
             qerror_report_err(local_err);
             error_free(local_err);
diff --git a/block/vhdx.c b/block/vhdx.c
index 67bbe10..4809056 100644
--- a/block/vhdx.c
+++ b/block/vhdx.c
@@ -1798,7 +1798,7 @@ static int vhdx_create(const char *filename, QEMUOptionParameter *options,
         goto exit;
     }
 
-    ret = bdrv_file_open(&bs, filename, NULL, BDRV_O_RDWR, &local_err);
+    ret = bdrv_file_open(&bs, filename, NULL, NULL, BDRV_O_RDWR, &local_err);
     if (ret < 0) {
         error_propagate(errp, local_err);
         goto exit;
diff --git a/block/vmdk.c b/block/vmdk.c
index 0734bc2..a095f1e 100644
--- a/block/vmdk.c
+++ b/block/vmdk.c
@@ -764,8 +764,8 @@ static int vmdk_parse_extents(const char *desc, BlockDriverState *bs,
 
         path_combine(extent_path, sizeof(extent_path),
                 desc_file_path, fname);
-        ret = bdrv_file_open(&extent_file, extent_path, NULL, bs->open_flags,
-                             errp);
+        ret = bdrv_file_open(&extent_file, extent_path, NULL, NULL,
+                             bs->open_flags, errp);
         if (ret) {
             return ret;
         }
diff --git a/include/block/block.h b/include/block/block.h
index 36efaea..e2b2a15 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -184,7 +184,8 @@ void bdrv_append(BlockDriverState *bs_new, BlockDriverState *bs_top);
 int bdrv_parse_cache_flags(const char *mode, int *flags);
 int bdrv_parse_discard_flags(const char *mode, int *flags);
 int bdrv_file_open(BlockDriverState **pbs, const char *filename,
-                   QDict *options, int flags, Error **errp);
+                   const char *reference, QDict *options, int flags,
+                   Error **errp);
 int bdrv_open_backing_file(BlockDriverState *bs, QDict *options, Error **errp);
 int bdrv_open(BlockDriverState *bs, const char *filename, QDict *options,
               int flags, BlockDriver *drv, Error **errp);
diff --git a/qemu-io.c b/qemu-io.c
index 3b3340a..f180813 100644
--- a/qemu-io.c
+++ b/qemu-io.c
@@ -56,7 +56,7 @@ static int openfile(char *name, int flags, int growable, QDict *opts)
     }
 
     if (growable) {
-        if (bdrv_file_open(&qemuio_bs, name, opts, flags, &local_err)) {
+        if (bdrv_file_open(&qemuio_bs, name, NULL, opts, flags, &local_err)) {
             fprintf(stderr, "%s: can't open device %s: %s\n", progname, name,
                     error_get_pretty(local_err));
             error_free(local_err);
-- 
1.8.5.1

[Qemu-devel] [PATCH v6 09/22] block: Pass reference to bdrv_file_open()

[Max Reitz]

With that now being possible, bdrv_open() should try to extract a block
device reference from the options and pass it to bdrv_file_open().

Signed-off-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: Kevin Wolf <kwolf@redhat.com>
---
 block.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/block.c b/block.c
index 1e53b3d..bef4f82 100644
--- a/block.c
+++ b/block.c
@@ -1056,6 +1056,7 @@ int bdrv_open(BlockDriverState *bs, const char *filename, QDict *options,
     char tmp_filename[PATH_MAX + 1];
     BlockDriverState *file = NULL;
     QDict *file_options = NULL;
+    const char *file_reference;
     const char *drvname;
     Error *local_err = NULL;
 
@@ -1142,9 +1143,11 @@ int bdrv_open(BlockDriverState *bs, const char *filename, QDict *options,
     }
 
     qdict_extract_subqdict(options, &file_options, "file.");
+    file_reference = qdict_get_try_str(options, "file");
 
-    ret = bdrv_file_open(&file, filename, NULL, file_options,
+    ret = bdrv_file_open(&file, filename, file_reference, file_options,
                          bdrv_open_flags(bs, flags | BDRV_O_UNMAP), &local_err);
+    qdict_del(options, "file");
     if (ret < 0) {
         goto fail;
     }
-- 
1.8.5.1

[Qemu-devel] [PATCH v6 10/22] block: Allow block devices without files

[Max Reitz]

blkdebug and blkverify will, in order to retain compatibility, not
support the field "file" implicitly through bdrv_open(). In order to be
able to use those drivers without giving a filename anyway, it is
necessary to be able to have block devices without files implicitly
opened by bdrv_open(). This is the case, if there was neither a file
name, a reference to an existing block device to use as a file nor
options specific to the file.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 block.c | 23 ++++++++++++++++-------
 1 file changed, 16 insertions(+), 7 deletions(-)

diff --git a/block.c b/block.c
index bef4f82..7464fb2 100644
--- a/block.c
+++ b/block.c
@@ -1145,11 +1145,14 @@ int bdrv_open(BlockDriverState *bs, const char *filename, QDict *options,
     qdict_extract_subqdict(options, &file_options, "file.");
     file_reference = qdict_get_try_str(options, "file");
 
-    ret = bdrv_file_open(&file, filename, file_reference, file_options,
-                         bdrv_open_flags(bs, flags | BDRV_O_UNMAP), &local_err);
-    qdict_del(options, "file");
-    if (ret < 0) {
-        goto fail;
+    if (filename || file_reference || qdict_size(file_options)) {
+        ret = bdrv_file_open(&file, filename, file_reference, file_options,
+                             bdrv_open_flags(bs, flags | BDRV_O_UNMAP),
+                             &local_err);
+        qdict_del(options, "file");
+        if (ret < 0) {
+            goto fail;
+        }
     }
 
     /* Find the right image format driver */
@@ -1165,7 +1168,13 @@ int bdrv_open(BlockDriverState *bs, const char *filename, QDict *options,
     }
 
     if (!drv) {
-        ret = find_image_format(file, filename, &drv, &local_err);
+        if (file) {
+            ret = find_image_format(file, filename, &drv, &local_err);
+        } else {
+            error_setg(errp, "Must specify either driver or file");
+            ret = -EINVAL;
+            goto unlink_and_fail;
+        }
     }
 
     if (!drv) {
@@ -1178,7 +1187,7 @@ int bdrv_open(BlockDriverState *bs, const char *filename, QDict *options,
         goto unlink_and_fail;
     }
 
-    if (bs->file != file) {
+    if (file && (bs->file != file)) {
         bdrv_unref(file);
         file = NULL;
     }
-- 
1.8.5.1

[Qemu-devel] [PATCH v6 11/22] block: Add bdrv_open_image()

[Max Reitz]

Add a common function for opening images to be used for block drivers
specified through BlockdevRefs in an option QDict. The difference from
bdrv_file_open() is that this function may invoke bdrv_open() instead,
allowing auto-detection of the driver to be used; and second, it
automatically extracts the BlockdevRef from the option QDict.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 block.c               | 73 +++++++++++++++++++++++++++++++++++++++++++++++++++
 include/block/block.h |  3 +++
 2 files changed, 76 insertions(+)

diff --git a/block.c b/block.c
index 7464fb2..76b6c25 100644
--- a/block.c
+++ b/block.c
@@ -1041,6 +1041,79 @@ int bdrv_open_backing_file(BlockDriverState *bs, QDict *options, Error **errp)
 }
 
 /*
+ * Opens a disk image whose options are given as BlockdevRef in another block
+ * device's options.
+ *
+ * If force_raw is true, bdrv_file_open() will be used, thereby preventing any
+ * image format auto-detection. If it is false and a filename is given,
+ * bdrv_open() will be used for auto-detection.
+ *
+ * If allow_none is true, no image will be opened if filename is false and no
+ * BlockdevRef is given. *pbs will remain unchanged and 0 will be returned.
+ *
+ * bdrev_key specifies the key for the image's BlockdevRef in the options QDict.
+ * That QDict has to be flattened; therefore, if the BlockdevRef is a QDict
+ * itself, all options starting with "${bdref_key}." are considered part of the
+ * BlockdevRef.
+ *
+ * The BlockdevRef will be removed from the options QDict.
+ */
+int bdrv_open_image(BlockDriverState **pbs, const char *filename,
+                    QDict *options, const char *bdref_key, int flags,
+                    bool force_raw, bool allow_none, Error **errp)
+{
+    QDict *image_options;
+    int ret;
+    char *bdref_key_dot;
+    const char *reference;
+
+    bdref_key_dot = g_strdup_printf("%s.", bdref_key);
+    qdict_extract_subqdict(options, &image_options, bdref_key_dot);
+    g_free(bdref_key_dot);
+
+    reference = qdict_get_try_str(options, bdref_key);
+    if (!filename && !reference && !qdict_size(image_options)) {
+        if (allow_none) {
+            ret = 0;
+        } else {
+            error_setg(errp, "A block device must be specified for \"%s\"",
+                       bdref_key);
+            ret = -EINVAL;
+        }
+        goto done;
+    }
+
+    if (filename && !force_raw) {
+        /* If a filename is given and the block driver should be detected
+           automatically (instead of using none), use bdrv_open() in order to do
+           that auto-detection. */
+        BlockDriverState *bs;
+
+        if (reference) {
+            error_setg(errp, "Cannot reference an existing block device while "
+                       "giving a filename");
+            ret = -EINVAL;
+            goto done;
+        }
+
+        bs = bdrv_new("");
+        ret = bdrv_open(bs, filename, image_options, flags, NULL, errp);
+        if (ret < 0) {
+            bdrv_unref(bs);
+        } else {
+            *pbs = bs;
+        }
+    } else {
+        ret = bdrv_file_open(pbs, filename, reference, image_options, flags,
+                             errp);
+    }
+
+done:
+    qdict_del(options, bdref_key);
+    return ret;
+}
+
+/*
  * Opens a disk image (raw, qcow2, vmdk, ...)
  *
  * options is a QDict of options to pass to the block drivers, or NULL for an
diff --git a/include/block/block.h b/include/block/block.h
index e2b2a15..a47f3d4 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -186,6 +186,9 @@ int bdrv_parse_discard_flags(const char *mode, int *flags);
 int bdrv_file_open(BlockDriverState **pbs, const char *filename,
                    const char *reference, QDict *options, int flags,
                    Error **errp);
+int bdrv_open_image(BlockDriverState **pbs, const char *filename,
+                    QDict *options, const char *bdref_key, int flags,
+                    bool force_raw, bool allow_none, Error **errp);
 int bdrv_open_backing_file(BlockDriverState *bs, QDict *options, Error **errp);
 int bdrv_open(BlockDriverState *bs, const char *filename, QDict *options,
               int flags, BlockDriver *drv, Error **errp);
-- 
1.8.5.1

[Qemu-devel] [PATCH v6 12/22] block: Use bdrv_open_image() in bdrv_open()

[Max Reitz]

Using bdrv_open_image() instead of bdrv_file_open() directly in
bdrv_open() is easier.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 block.c | 18 +++++-------------
 1 file changed, 5 insertions(+), 13 deletions(-)

diff --git a/block.c b/block.c
index 76b6c25..9e4e85f 100644
--- a/block.c
+++ b/block.c
@@ -1128,8 +1128,6 @@ int bdrv_open(BlockDriverState *bs, const char *filename, QDict *options,
     /* TODO: extra byte is a hack to ensure MAX_PATH space on Windows. */
     char tmp_filename[PATH_MAX + 1];
     BlockDriverState *file = NULL;
-    QDict *file_options = NULL;
-    const char *file_reference;
     const char *drvname;
     Error *local_err = NULL;
 
@@ -1215,17 +1213,11 @@ int bdrv_open(BlockDriverState *bs, const char *filename, QDict *options,
         flags |= BDRV_O_ALLOW_RDWR;
     }
 
-    qdict_extract_subqdict(options, &file_options, "file.");
-    file_reference = qdict_get_try_str(options, "file");
-
-    if (filename || file_reference || qdict_size(file_options)) {
-        ret = bdrv_file_open(&file, filename, file_reference, file_options,
-                             bdrv_open_flags(bs, flags | BDRV_O_UNMAP),
-                             &local_err);
-        qdict_del(options, "file");
-        if (ret < 0) {
-            goto fail;
-        }
+    ret = bdrv_open_image(&file, filename, options, "file",
+                          bdrv_open_flags(bs, flags | BDRV_O_UNMAP), true, true,
+                          &local_err);
+    if (ret < 0) {
+        goto fail;
     }
 
     /* Find the right image format driver */
-- 
1.8.5.1

[Qemu-devel] [PATCH v6 13/22] block: Allow recursive "file"s

[Max Reitz]

It should be possible to use a format as a driver for a file which in
turn requires another file, i.e., nesting file formats.

Allowing nested file formats results in e.g. qcow2 BlockDriverStates
never being directly passed to bdrv_open_common() from bdrv_file_open(),
but instead being handed through bdrv_open(). This changes the error
message when trying to give a filename to qcow2, i.e. trying to use it
as a driver for the protocol level. Therefore, change the reference
output of I/O test 051 accordingly.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 block.c                    | 9 +++++++--
 tests/qemu-iotests/051.out | 2 +-
 2 files changed, 8 insertions(+), 3 deletions(-)

diff --git a/block.c b/block.c
index 9e4e85f..6af5f6e 100644
--- a/block.c
+++ b/block.c
@@ -948,14 +948,19 @@ int bdrv_file_open(BlockDriverState **pbs, const char *filename,
         goto fail;
     }
 
-    ret = bdrv_open_common(bs, NULL, options, flags, drv, &local_err);
+    if (!drv->bdrv_file_open) {
+        ret = bdrv_open(bs, filename, options, flags, drv, &local_err);
+        options = NULL;
+    } else {
+        ret = bdrv_open_common(bs, NULL, options, flags, drv, &local_err);
+    }
     if (ret < 0) {
         error_propagate(errp, local_err);
         goto fail;
     }
 
     /* Check if any unknown options were used */
-    if (qdict_size(options) != 0) {
+    if (options && (qdict_size(options) != 0)) {
         const QDictEntry *entry = qdict_first(options);
         error_setg(errp, "Block protocol '%s' doesn't support the option '%s'",
                    drv->format_name, entry->key);
diff --git a/tests/qemu-iotests/051.out b/tests/qemu-iotests/051.out
index 49e95a2..121ca66 100644
--- a/tests/qemu-iotests/051.out
+++ b/tests/qemu-iotests/051.out
@@ -223,7 +223,7 @@ QEMU X.Y.Z monitor - type 'help' for more information
 (qemu) q^[[K^[[Dqu^[[K^[[D^[[Dqui^[[K^[[D^[[D^[[Dquit^[[K
 
 Testing: -drive file=TEST_DIR/t.qcow2,file.driver=qcow2
-QEMU_PROG: -drive file=TEST_DIR/t.qcow2,file.driver=qcow2: could not open disk image TEST_DIR/t.qcow2: Can't use 'qcow2' as a block driver for the protocol level
+QEMU_PROG: -drive file=TEST_DIR/t.qcow2,file.driver=qcow2: could not open disk image TEST_DIR/t.qcow2: Block format 'qcow2' used by device '' doesn't support the option 'filename'
 
 
 === Parsing protocol from file name ===
-- 
1.8.5.1

[Qemu-devel] [PATCH v6 14/22] blockdev: Move "file" to legacy_opts

[Max Reitz]

Specifying the image filename through the "file" option is a legacy
option and should not be supported by blockdev-add (in that case, giving
a string for "file" references an existing block device).

Signed-off-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: Kevin Wolf <kwolf@redhat.com>
---
 blockdev.c | 19 ++++++++++---------
 1 file changed, 10 insertions(+), 9 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index 6a85961..d78961d 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -307,12 +307,11 @@ static bool check_throttle_config(ThrottleConfig *cfg, Error **errp)
 typedef enum { MEDIA_DISK, MEDIA_CDROM } DriveMediaType;
 
 /* Takes the ownership of bs_opts */
-static DriveInfo *blockdev_init(QDict *bs_opts,
+static DriveInfo *blockdev_init(const char *file, QDict *bs_opts,
                                 BlockInterfaceType type,
                                 Error **errp)
 {
     const char *buf;
-    const char *file = NULL;
     const char *serial;
     int ro = 0;
     int bdrv_flags = 0;
@@ -354,7 +353,6 @@ static DriveInfo *blockdev_init(QDict *bs_opts,
     ro = qemu_opt_get_bool(opts, "read-only", 0);
     copy_on_read = qemu_opt_get_bool(opts, "copy-on-read", false);
 
-    file = qemu_opt_get(opts, "file");
     serial = qemu_opt_get(opts, "serial");
 
     if ((buf = qemu_opt_get(opts, "discard")) != NULL) {
@@ -599,6 +597,10 @@ QemuOptsList qemu_legacy_drive_opts = {
             .name = "addr",
             .type = QEMU_OPT_STRING,
             .help = "pci address (virtio only)",
+        },{
+            .name = "file",
+            .type = QEMU_OPT_STRING,
+            .help = "file name",
         },
 
         /* Options that are passed on, but have special semantics with -drive */
@@ -629,6 +631,7 @@ DriveInfo *drive_init(QemuOpts *all_opts, BlockInterfaceType block_default_type)
     const char *devaddr;
     bool read_only = false;
     bool copy_on_read;
+    const char *filename;
     Error *local_err = NULL;
 
     /* Change legacy command line options into QMP ones */
@@ -865,8 +868,10 @@ DriveInfo *drive_init(QemuOpts *all_opts, BlockInterfaceType block_default_type)
         }
     }
 
+    filename = qemu_opt_get(legacy_opts, "file");
+
     /* Actual block device init: Functionality shared with blockdev-add */
-    dinfo = blockdev_init(bs_opts, type, &local_err);
+    dinfo = blockdev_init(filename, bs_opts, type, &local_err);
     if (dinfo == NULL) {
         if (error_is_set(&local_err)) {
             qerror_report_err(local_err);
@@ -2203,7 +2208,7 @@ void qmp_blockdev_add(BlockdevOptions *options, Error **errp)
 
     qdict_flatten(qdict);
 
-    blockdev_init(qdict, IF_NONE, &local_err);
+    blockdev_init(NULL, qdict, IF_NONE, &local_err);
     if (error_is_set(&local_err)) {
         error_propagate(errp, local_err);
         goto fail;
@@ -2244,10 +2249,6 @@ QemuOptsList qemu_common_drive_opts = {
             .type = QEMU_OPT_BOOL,
             .help = "enable/disable snapshot mode",
         },{
-            .name = "file",
-            .type = QEMU_OPT_STRING,
-            .help = "disk image",
-        },{
             .name = "discard",
             .type = QEMU_OPT_STRING,
             .help = "discard operation (ignore/off, unmap/on)",
-- 
1.8.5.1

[Qemu-devel] [PATCH v6 15/22] blkdebug: Allow command-line file configuration

[Max Reitz]

Introduce the "image" option as an alternative to specifying the image
through the filename.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 block/blkdebug.c | 12 +++---------
 1 file changed, 3 insertions(+), 9 deletions(-)

diff --git a/block/blkdebug.c b/block/blkdebug.c
index c73a6cf..3a46ecd 100644
--- a/block/blkdebug.c
+++ b/block/blkdebug.c
@@ -374,7 +374,7 @@ static int blkdebug_open(BlockDriverState *bs, QDict *options, int flags,
     BDRVBlkdebugState *s = bs->opaque;
     QemuOpts *opts;
     Error *local_err = NULL;
-    const char *filename, *config;
+    const char *config;
     int ret;
 
     opts = qemu_opts_create_nofail(&runtime_opts);
@@ -396,14 +396,8 @@ static int blkdebug_open(BlockDriverState *bs, QDict *options, int flags,
     s->state = 1;
 
     /* Open the backing file */
-    filename = qemu_opt_get(opts, "x-image");
-    if (filename == NULL) {
-        error_setg(errp, "Could not retrieve image file name");
-        ret = -EINVAL;
-        goto fail;
-    }
-
-    ret = bdrv_file_open(&bs->file, filename, NULL, NULL, flags, &local_err);
+    ret = bdrv_open_image(&bs->file, qemu_opt_get(opts, "x-image"), options, "image",
+                          flags, true, false, &local_err);
     if (ret < 0) {
         error_propagate(errp, local_err);
         goto fail;
-- 
1.8.5.1

[Qemu-devel] [PATCH v6 16/22] blkverify: Allow command-line configuration

[Max Reitz]

Introduce the "test" and "raw" options for specifying images.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 block/blkverify.c | 25 +++++--------------------
 1 file changed, 5 insertions(+), 20 deletions(-)

diff --git a/block/blkverify.c b/block/blkverify.c
index c6eb287..19a5179 100644
--- a/block/blkverify.c
+++ b/block/blkverify.c
@@ -122,7 +122,6 @@ static int blkverify_open(BlockDriverState *bs, QDict *options, int flags,
     BDRVBlkverifyState *s = bs->opaque;
     QemuOpts *opts;
     Error *local_err = NULL;
-    const char *filename, *raw;
     int ret;
 
     opts = qemu_opts_create_nofail(&runtime_opts);
@@ -133,33 +132,19 @@ static int blkverify_open(BlockDriverState *bs, QDict *options, int flags,
         goto fail;
     }
 
-    /* Parse the raw image filename */
-    raw = qemu_opt_get(opts, "x-raw");
-    if (raw == NULL) {
-        error_setg(errp, "Could not retrieve raw image filename");
-        ret = -EINVAL;
-        goto fail;
-    }
-
-    ret = bdrv_file_open(&bs->file, raw, NULL, NULL, flags, &local_err);
+    /* Open the raw file */
+    ret = bdrv_open_image(&bs->file, qemu_opt_get(opts, "x-raw"), options,
+                          "raw", flags, true, false, &local_err);
     if (ret < 0) {
         error_propagate(errp, local_err);
         goto fail;
     }
 
     /* Open the test file */
-    filename = qemu_opt_get(opts, "x-image");
-    if (filename == NULL) {
-        error_setg(errp, "Could not retrieve test image filename");
-        ret = -EINVAL;
-        goto fail;
-    }
-
-    s->test_file = bdrv_new("");
-    ret = bdrv_open(s->test_file, filename, NULL, flags, NULL, &local_err);
+    ret = bdrv_open_image(&s->test_file, qemu_opt_get(opts, "x-image"), options,
+                          "test", flags, false, false, &local_err);
     if (ret < 0) {
         error_propagate(errp, local_err);
-        bdrv_unref(s->test_file);
         s->test_file = NULL;
         goto fail;
     }
-- 
1.8.5.1

[Qemu-devel] [PATCH v6 17/22] blkverify: Don't require protocol filename

[Max Reitz]

If the filename is not prefixed by "blkverify:" in
blkverify_parse_filename(), the blkverify driver was not selected
through that protocol prefix, but by an explicit command line (or QMP)
option (like driver=blkverify).

If blkverify_parse_filename() has been called, a filename has been
given. If it is not prefixed, it is probably really just a plain
filename. This is no problem, since we can use it as the test image
filename and rely on the user to specify the raw image filename through
the new corresponding option.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 block/blkverify.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/block/blkverify.c b/block/blkverify.c
index 19a5179..0c6393b 100644
--- a/block/blkverify.c
+++ b/block/blkverify.c
@@ -78,7 +78,9 @@ static void blkverify_parse_filename(const char *filename, QDict *options,
 
     /* Parse the blkverify: prefix */
     if (!strstart(filename, "blkverify:", &filename)) {
-        error_setg(errp, "File name string must start with 'blkverify:'");
+        /* There was no prefix; therefore, all options have to be already
+           present in the QDict (except for the filename) */
+        qdict_put(options, "x-image", qstring_from_str(filename));
         return;
     }
 
-- 
1.8.5.1

[Qemu-devel] [PATCH v6 18/22] qapi: Add "errno" to the list of polluted words

[Max Reitz]

Using "errno" directly as an identifier results in various syntax
errors; therefore it should be added to the list of polluted words.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 scripts/qapi.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/scripts/qapi.py b/scripts/qapi.py
index 750e9fb..9b3de4c 100644
--- a/scripts/qapi.py
+++ b/scripts/qapi.py
@@ -247,7 +247,7 @@ def c_var(name, protect=True):
                      'and', 'and_eq', 'bitand', 'bitor', 'compl', 'not',
                      'not_eq', 'or', 'or_eq', 'xor', 'xor_eq'])
     # namespace pollution:
-    polluted_words = set(['unix'])
+    polluted_words = set(['unix', 'errno'])
     if protect and (name in c89_words | c99_words | c11_words | gcc_words | cpp_words | polluted_words):
         return "q_" + name
     return name.replace('-', '_').lstrip("*")
-- 
1.8.5.1

[Qemu-devel] [PATCH v6 19/22] qapi: QMP interface for blkdebug and blkverify

[Max Reitz]

Add structures to support blkdebug and blkverify in blockdev-add.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 qapi-schema.json | 113 +++++++++++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 109 insertions(+), 4 deletions(-)

diff --git a/qapi-schema.json b/qapi-schema.json
index c3c939c..feb128c 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -4166,6 +4166,113 @@
             '*pass-discard-other': 'bool' } }
 
 ##
+# @BlkdebugEvent
+#
+# Trigger events supported by blkdebug.
+##
+{ 'enum': 'BlkdebugEvent',
+  'data': [ 'l1_update', 'l1_grow.alloc_table', 'l1_grow.write_table',
+            'l1_grow.activate_table', 'l2_load', 'l2_update',
+            'l2_update_compressed', 'l2_alloc.cow_read', 'l2_alloc.write',
+            'read_aio', 'read_backing_aio', 'read_compressed', 'write_aio',
+            'write_compressed', 'vmstate_load', 'vmstate_save', 'cow_read',
+            'cow_write', 'reftable_load', 'reftable_grow', 'reftable_update',
+            'refblock_load', 'refblock_update', 'refblock_update_part',
+            'refblock_alloc', 'refblock_alloc.hookup', 'refblock_alloc.write',
+            'refblock_alloc.write_blocks', 'refblock_alloc.write_table',
+            'refblock_alloc.switch_table', 'cluster_alloc',
+            'cluster_alloc_bytes', 'cluster_free', 'flush_to_os',
+            'flush_to_disk' ] }
+
+##
+# @BlkdebugInjectErrorOptions
+#
+# Describes a single error injection for blkdebug.
+#
+# @event:       trigger event
+#
+# @state:       #optional the state identifier blkdebug needs to be in to
+#               actually trigger the event; defaults to "any"
+#
+# @errno:       #optional error identifier (errno) to be returned; defaults to
+#               EIO
+#
+# @sector:      #optional specifies the sector index which has to be affected
+#               in order to actually trigger the event; defaults to "any
+#               sector"
+#
+# @once:        #optional disables further events after this one has been
+#               triggered; defaults to false
+#
+# @immediately: #optional fail immediately; defaults to false
+#
+# Since: 2.0
+##
+{ 'type': 'BlkdebugInjectErrorOptions',
+  'data': { 'event': 'BlkdebugEvent',
+            '*state': 'int',
+            '*errno': 'int',
+            '*sector': 'int',
+            '*once': 'bool',
+            '*immediately': 'bool' } }
+
+##
+# @BlkdebugSetStateOptions
+#
+# Describes a single state-change event for blkdebug.
+#
+# @event:       trigger event
+#
+# @state:       #optional the current state identifier blkdebug needs to be in;
+#               defaults to "any"
+#
+# @new_state:   the state identifier blkdebug is supposed to assume if
+#               this event is triggered
+#
+# Since: 2.0
+##
+{ 'type': 'BlkdebugSetStateOptions',
+  'data': { 'event': 'BlkdebugEvent',
+            '*state': 'int',
+            'new_state': 'int' } }
+
+##
+# @BlockdevOptionsBlkdebug
+#
+# Driver specific block device options for blkdebug.
+#
+# @image:           underlying raw block device (or image file)
+#
+# @config:          #optional filename of the configuration file
+#
+# @inject-error:    #optional array of error injection descriptions
+#
+# @set-state:       #optional array of state-change descriptions
+#
+# Since: 2.0
+##
+{ 'type': 'BlockdevOptionsBlkdebug',
+  'data': { 'image': 'BlockdevRef',
+            '*config': 'str',
+            '*inject-error': ['BlkdebugInjectErrorOptions'],
+            '*set-state': ['BlkdebugSetStateOptions'] } }
+
+##
+# @BlockdevOptionsBlkverify
+#
+# Driver specific block device options for blkverify.
+#
+# @test:    block device to be tested
+#
+# @raw:     raw image used for verification
+#
+# Since: 2.0
+##
+{ 'type': 'BlockdevOptionsBlkverify',
+  'data': { 'test': 'BlockdevRef',
+            'raw': 'BlockdevRef' } }
+
+##
 # @BlockdevOptions
 #
 # Options for creating a block device.
@@ -4189,10 +4296,8 @@
 # TODO sheepdog: Wait for structured options
 # TODO ssh: Should take InetSocketAddress for 'host'?
       'vvfat':      'BlockdevOptionsVVFAT',
-
-# TODO blkdebug: Wait for structured options
-# TODO blkverify: Wait for structured options
-
+      'blkdebug':   'BlockdevOptionsBlkdebug',
+      'blkverify':  'BlockdevOptionsBlkverify',
       'bochs':      'BlockdevOptionsGenericFormat',
       'cloop':      'BlockdevOptionsGenericFormat',
       'cow':        'BlockdevOptionsGenericCOWFormat',
-- 
1.8.5.1

[Qemu-devel] [PATCH v6 20/22] qemu-io: Make filename optional

[Max Reitz]

Giving a filename is actually not essential, since it can be specified
through the options as well - on the contrary: Sometimes a filename must
not be given.

Signed-off-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: Kevin Wolf <kwolf@redhat.com>
---
 qemu-io.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/qemu-io.c b/qemu-io.c
index f180813..a0e0e22 100644
--- a/qemu-io.c
+++ b/qemu-io.c
@@ -160,11 +160,13 @@ static int open_f(BlockDriverState *bs, int argc, char **argv)
         flags |= BDRV_O_RDWR;
     }
 
-    if (optind != argc - 1) {
+    if (optind == argc - 1) {
+        return openfile(argv[optind], flags, growable, opts);
+    } else if (optind == argc) {
+        return openfile(NULL, flags, growable, opts);
+    } else {
         return qemuio_command_usage(&open_cmd);
     }
-
-    return openfile(argv[optind], flags, growable, opts);
 }
 
 static int quit_f(BlockDriverState *bs, int argc, char **argv)
-- 
1.8.5.1

[Qemu-devel] [PATCH v6 21/22] iotests: Test new blkdebug/blkverify interface

[Max Reitz]

Add a test for the new blkdebug/blkverify interface.

This test is not written in Python, although it uses QMP. This is
because it invokes the qemu-io HMP command, which outputs errors to
stderr instead of returning them through QMP. Filtering and testing that
output is easier in a shell script than with the Python infrastructure.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 tests/qemu-iotests/071     | 239 +++++++++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/071.out |  90 +++++++++++++++++
 tests/qemu-iotests/group   |   1 +
 3 files changed, 330 insertions(+)
 create mode 100755 tests/qemu-iotests/071
 create mode 100644 tests/qemu-iotests/071.out

diff --git a/tests/qemu-iotests/071 b/tests/qemu-iotests/071
new file mode 100755
index 0000000..2a22546
--- /dev/null
+++ b/tests/qemu-iotests/071
@@ -0,0 +1,239 @@
+#!/bin/bash
+#
+# Test case for the QMP blkdebug and blkverify interfaces
+#
+# Copyright (C) 2013 Red Hat, Inc.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+#
+
+# creator
+owner=mreitz@redhat.com
+
+seq="$(basename $0)"
+echo "QA output created by $seq"
+
+here="$PWD"
+tmp=/tmp/$$
+status=1	# failure is the default!
+
+_cleanup()
+{
+	_cleanup_test_img
+}
+trap "_cleanup; exit \$status" 0 1 2 3 15
+
+# get standard environment, filters and checks
+. ./common.rc
+. ./common.filter
+
+_supported_fmt generic
+_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
+}
+
+IMG_SIZE=64M
+
+echo
+echo "=== Testing blkverify through filename ==="
+echo
+
+TEST_IMG="$TEST_IMG.base" IMGOPTS="" IMGFMT="raw" _make_test_img $IMG_SIZE |\
+    _filter_imgfmt
+_make_test_img $IMG_SIZE
+$QEMU_IO -c "open -o file.driver=blkverify,file.raw.filename=$TEST_IMG.base $TEST_IMG" \
+         -c 'read 0 512' -c 'write -P 42 0x38000 512' -c 'read -P 42 0x38000 512' | _filter_qemu_io
+
+$QEMU_IO -c 'write -P 42 0 512' "$TEST_IMG" | _filter_qemu_io
+
+$QEMU_IO -c "open -o file.driver=blkverify,file.raw.filename=$TEST_IMG.base $TEST_IMG" \
+         -c 'read -P 42 0 512' | _filter_qemu_io
+
+echo
+echo "=== Testing blkverify through file blockref ==="
+echo
+
+TEST_IMG="$TEST_IMG.base" IMGOPTS="" IMGFMT="raw" _make_test_img $IMG_SIZE |\
+    _filter_imgfmt
+_make_test_img $IMG_SIZE
+$QEMU_IO -c "open -o file.driver=blkverify,file.raw.filename=$TEST_IMG.base,file.test.driver=$IMGFMT,file.test.file.filename=$TEST_IMG" \
+         -c 'read 0 512' -c 'write -P 42 0x38000 512' -c 'read -P 42 0x38000 512' | _filter_qemu_io
+
+$QEMU_IO -c 'write -P 42 0 512' "$TEST_IMG" | _filter_qemu_io
+
+$QEMU_IO -c "open -o file.driver=blkverify,file.raw.filename=$TEST_IMG.base $TEST_IMG" \
+         -c 'read -P 42 0 512' | _filter_qemu_io
+
+echo
+echo "=== Testing blkdebug through filename ==="
+echo
+
+$QEMU_IO -c "open -o file.driver=blkdebug,file.inject-error.event=l2_load $TEST_IMG" \
+         -c 'read -P 42 0x38000 512'
+
+echo
+echo "=== Testing blkdebug through file blockref ==="
+echo
+
+$QEMU_IO -c "open -o driver=$IMGFMT,file.driver=blkdebug,file.inject-error.event=l2_load,file.image.filename=$TEST_IMG" \
+         -c 'read -P 42 0x38000 512'
+
+echo
+echo "=== Testing blkdebug on existing block device ==="
+echo
+
+run_qemu -drive "file=$TEST_IMG,format=raw,if=none,id=drive0" <<EOF
+{ "execute": "qmp_capabilities" }
+{ "execute": "blockdev-add",
+    "arguments": {
+        "options": {
+            "driver": "$IMGFMT",
+            "id": "drive0-debug",
+            "file": {
+                "driver": "blkdebug",
+                "image": "drive0",
+                "inject-error": [{
+                    "event": "l2_load"
+                }]
+            }
+        }
+    }
+}
+{ "execute": "human-monitor-command",
+    "arguments": {
+        "command-line": 'qemu-io drive0-debug "read 0 512"'
+    }
+}
+{ "execute": "quit" }
+EOF
+
+echo
+echo "=== Testing blkverify on existing block device ==="
+echo
+
+run_qemu -drive "file=$TEST_IMG,format=$IMGFMT,if=none,id=drive0" <<EOF
+{ "execute": "qmp_capabilities" }
+{ "execute": "blockdev-add",
+    "arguments": {
+        "options": {
+            "driver": "blkverify",
+            "id": "drive0-verify",
+            "test": "drive0",
+            "raw": {
+                "driver": "raw",
+                "file": {
+                    "driver": "file",
+                    "filename": "$TEST_IMG.base"
+                }
+            }
+        }
+    }
+}
+{ "execute": "human-monitor-command",
+    "arguments": {
+        "command-line": 'qemu-io drive0-verify "read 0 512"'
+    }
+}
+{ "execute": "quit" }
+EOF
+
+echo
+echo "=== Testing blkverify on existing raw block device ==="
+echo
+
+run_qemu -drive "file=$TEST_IMG.base,if=none,id=drive0" <<EOF
+{ "execute": "qmp_capabilities" }
+{ "execute": "blockdev-add",
+    "arguments": {
+        "options": {
+            "driver": "blkverify",
+            "id": "drive0-verify",
+            "test": {
+                "driver": "$IMGFMT",
+                "file": {
+                    "driver": "file",
+                    "filename": "$TEST_IMG"
+                }
+            },
+            "raw": "drive0"
+        }
+    }
+}
+{ "execute": "human-monitor-command",
+    "arguments": {
+        "command-line": 'qemu-io drive0-verify "read 0 512"'
+    }
+}
+{ "execute": "quit" }
+EOF
+
+echo
+echo "=== Testing blkdebug's set-state through QMP ==="
+echo
+
+run_qemu -drive "file=$TEST_IMG,format=raw,if=none,id=drive0" <<EOF
+{ "execute": "qmp_capabilities" }
+{ "execute": "blockdev-add",
+    "arguments": {
+        "options": {
+            "driver": "$IMGFMT",
+            "id": "drive0-debug",
+            "file": {
+                "driver": "blkdebug",
+                "image": "drive0",
+                "inject-error": [{
+                    "event": "read_aio",
+                    "state": 42
+                }],
+                "set-state": [{
+                    "event": "write_aio",
+                    "new_state": 42
+                }]
+            }
+        }
+    }
+}
+{ "execute": "human-monitor-command",
+    "arguments": {
+        "command-line": 'qemu-io drive0-debug "read 0 512"'
+    }
+}
+{ "execute": "human-monitor-command",
+    "arguments": {
+        "command-line": 'qemu-io drive0-debug "write 0 512"'
+    }
+}
+{ "execute": "human-monitor-command",
+    "arguments": {
+        "command-line": 'qemu-io drive0-debug "read 0 512"'
+    }
+}
+{ "execute": "quit" }
+EOF
+
+# success, all done
+echo "*** done"
+rm -f $seq.full
+status=0
diff --git a/tests/qemu-iotests/071.out b/tests/qemu-iotests/071.out
new file mode 100644
index 0000000..5f840a9
--- /dev/null
+++ b/tests/qemu-iotests/071.out
@@ -0,0 +1,90 @@
+QA output created by 071
+
+=== Testing blkverify through filename ===
+
+Formatting 'TEST_DIR/t.IMGFMT.base', fmt=IMGFMT size=67108864 
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864 
+read 512/512 bytes at offset 0
+512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+wrote 512/512 bytes at offset 229376
+512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+read 512/512 bytes at offset 229376
+512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+wrote 512/512 bytes at offset 0
+512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+blkverify: read sector_num=0 nb_sectors=4 contents mismatch in sector 0
+
+=== Testing blkverify through file blockref ===
+
+Formatting 'TEST_DIR/t.IMGFMT.base', fmt=IMGFMT size=67108864 
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864 
+read 512/512 bytes at offset 0
+512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+wrote 512/512 bytes at offset 229376
+512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+read 512/512 bytes at offset 229376
+512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+wrote 512/512 bytes at offset 0
+512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+blkverify: read sector_num=0 nb_sectors=4 contents mismatch in sector 0
+
+=== Testing blkdebug through filename ===
+
+read failed: Input/output error
+
+=== Testing blkdebug through file blockref ===
+
+read failed: Input/output error
+
+=== Testing blkdebug on existing block device ===
+
+Testing: -drive file=TEST_DIR/t.IMGFMT,format=raw,if=none,id=drive0
+QMP_VERSION
+{"return": {}}
+{"return": {}}
+read failed: Input/output error
+{"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}}
+
+
+=== Testing blkverify on existing block device ===
+
+Testing: -drive file=TEST_DIR/t.IMGFMT,format=IMGFMT,if=none,id=drive0
+QMP_VERSION
+{"return": {}}
+{"return": {}}
+blkverify: read sector_num=0 nb_sectors=1 contents mismatch in sector 0
+
+
+=== Testing blkverify on existing raw block device ===
+
+Testing: -drive file=TEST_DIR/t.IMGFMT.base,if=none,id=drive0
+QMP_VERSION
+{"return": {}}
+{"return": {}}
+blkverify: read sector_num=0 nb_sectors=1 contents mismatch in sector 0
+
+
+=== Testing blkdebug's set-state through QMP ===
+
+Testing: -drive file=TEST_DIR/t.IMGFMT,format=raw,if=none,id=drive0
+QMP_VERSION
+{"return": {}}
+{"return": {}}
+read 512/512 bytes at offset 0
+512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+{"return": ""}
+wrote 512/512 bytes at offset 0
+512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+{"return": ""}
+read failed: Input/output error
+{"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}}
+
+*** done
diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
index cc750c9..1194339 100644
--- a/tests/qemu-iotests/group
+++ b/tests/qemu-iotests/group
@@ -77,5 +77,6 @@
 068 rw auto
 069 rw auto
 070 rw auto
+071 rw auto
 073 rw auto
 074 rw auto
-- 
1.8.5.1

[Qemu-devel] [PATCH v6 22/22] iotests: Test file format nesting

[Max Reitz]

Add a test for nested image formats.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 tests/qemu-iotests/072     | 69 ++++++++++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/072.out | 21 ++++++++++++++
 tests/qemu-iotests/group   |  1 +
 3 files changed, 91 insertions(+)
 create mode 100755 tests/qemu-iotests/072
 create mode 100644 tests/qemu-iotests/072.out

diff --git a/tests/qemu-iotests/072 b/tests/qemu-iotests/072
new file mode 100755
index 0000000..a3876c2
--- /dev/null
+++ b/tests/qemu-iotests/072
@@ -0,0 +1,69 @@
+#!/bin/bash
+#
+# Test case for nested image formats
+#
+# Copyright (C) 2013 Red Hat, Inc.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+#
+
+# creator
+owner=mreitz@redhat.com
+
+seq="$(basename $0)"
+echo "QA output created by $seq"
+
+here="$PWD"
+tmp=/tmp/$$
+status=1	# failure is the default!
+
+_cleanup()
+{
+	_cleanup_test_img
+}
+trap "_cleanup; exit \$status" 0 1 2 3 15
+
+# get standard environment, filters and checks
+. ./common.rc
+. ./common.filter
+
+_supported_fmt vpc vmdk vhdx vdi qed qcow2 qcow cow
+_supported_proto generic
+_supported_os Linux
+
+IMG_SIZE=64M
+
+echo
+echo "=== Testing nested image formats ==="
+echo
+
+TEST_IMG="$TEST_IMG.base" _make_test_img $IMG_SIZE
+
+$QEMU_IO -c 'write -P 42 0 512' -c 'write -P 23 512 512' \
+         -c 'write -P 66 1024 512' "$TEST_IMG.base" | _filter_qemu_io
+
+$QEMU_IMG convert -f raw -O $IMGFMT "$TEST_IMG.base" "$TEST_IMG"
+
+$QEMU_IO -c "open -o driver=$IMGFMT,file.driver=$IMGFMT,file.file.filename=$TEST_IMG" \
+         -c 'read -P 42 0 512' -c 'read -P 23 512 512' \
+         -c 'read -P 66 1024 512' | _filter_qemu_io
+
+# When not giving any format, qemu should open only one "layer". Therefore, this
+# should not work for any image formats with a header.
+$QEMU_IO -c 'read -P 42 0 512' "$TEST_IMG" | _filter_qemu_io
+
+# success, all done
+echo "*** done"
+rm -f $seq.full
+status=0
diff --git a/tests/qemu-iotests/072.out b/tests/qemu-iotests/072.out
new file mode 100644
index 0000000..efe577c
--- /dev/null
+++ b/tests/qemu-iotests/072.out
@@ -0,0 +1,21 @@
+QA output created by 072
+
+=== Testing nested image formats ===
+
+Formatting 'TEST_DIR/t.IMGFMT.base', fmt=IMGFMT size=67108864 
+wrote 512/512 bytes at offset 0
+512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+wrote 512/512 bytes at offset 512
+512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+wrote 512/512 bytes at offset 1024
+512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+read 512/512 bytes at offset 0
+512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+read 512/512 bytes at offset 512
+512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+read 512/512 bytes at offset 1024
+512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+Pattern verification failed at offset 0, 512 bytes
+read 512/512 bytes at offset 0
+512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+*** done
diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
index 1194339..5860c40 100644
--- a/tests/qemu-iotests/group
+++ b/tests/qemu-iotests/group
@@ -78,5 +78,6 @@
 069 rw auto
 070 rw auto
 071 rw auto
+072 rw auto
 073 rw auto
 074 rw auto
-- 
1.8.5.1

Re: [Qemu-devel] [PATCH v6 01/22] blkdebug: Use errp for read_config()

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 450 bytes --]

On 12/19/2013 12:47 PM, Max Reitz wrote:
> Use an Error variable in the read_config() function.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> Reviewed-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block/blkdebug.c | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]

Re: [Qemu-devel] [PATCH v6 02/22] blkdebug: Don't require sophisticated filename

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1583 bytes --]

On 12/19/2013 12:47 PM, Max Reitz wrote:
> If the filename is not prefixed by "blkdebug:" in
> blkdebug_parse_filename(), the blkdebug driver was not selected through
> that protocol prefix, but by an explicit command line option
> (file.driver=blkdebug or something similar). Contrary to the current
> reaction, this is not a problem at all; we just need to store the
> filename (in the x-image option) and can go on; the user just has to
> manually specify the config option.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> Reviewed-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block/blkdebug.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)

Reviewed-by: Eric Blake <eblake@redhat.com>

> 
> diff --git a/block/blkdebug.c b/block/blkdebug.c
> index 627e29d..a2301d7 100644
> --- a/block/blkdebug.c
> +++ b/block/blkdebug.c
> @@ -313,7 +313,9 @@ static void blkdebug_parse_filename(const char *filename, QDict *options,
>  
>      /* Parse the blkdebug: prefix */
>      if (!strstart(filename, "blkdebug:", &filename)) {
> -        error_setg(errp, "File name string must start with 'blkdebug:'");
> +        /* There was no prefix; therefore, all options have to be already
> +           present in the QDict (except for the filename) */
> +        qdict_put(options, "x-image", qstring_from_str(filename));

Am I correct that x-image is internal use only, and that we aren't
exposing an x- interface to the public user?

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]

Re: [Qemu-devel] [PATCH v6 02/22] blkdebug: Don't require sophisticated filename

[Max Reitz]

On 19.12.2013 21:56, Eric Blake wrote:
> On 12/19/2013 12:47 PM, Max Reitz wrote:
>> If the filename is not prefixed by "blkdebug:" in
>> blkdebug_parse_filename(), the blkdebug driver was not selected through
>> that protocol prefix, but by an explicit command line option
>> (file.driver=blkdebug or something similar). Contrary to the current
>> reaction, this is not a problem at all; we just need to store the
>> filename (in the x-image option) and can go on; the user just has to
>> manually specify the config option.
>>
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>> Reviewed-by: Kevin Wolf <kwolf@redhat.com>
>> ---
>>   block/blkdebug.c | 4 +++-
>>   1 file changed, 3 insertions(+), 1 deletion(-)
> Reviewed-by: Eric Blake <eblake@redhat.com>
>
>> diff --git a/block/blkdebug.c b/block/blkdebug.c
>> index 627e29d..a2301d7 100644
>> --- a/block/blkdebug.c
>> +++ b/block/blkdebug.c
>> @@ -313,7 +313,9 @@ static void blkdebug_parse_filename(const char *filename, QDict *options,
>>   
>>       /* Parse the blkdebug: prefix */
>>       if (!strstart(filename, "blkdebug:", &filename)) {
>> -        error_setg(errp, "File name string must start with 'blkdebug:'");
>> +        /* There was no prefix; therefore, all options have to be already
>> +           present in the QDict (except for the filename) */
>> +        qdict_put(options, "x-image", qstring_from_str(filename));
> Am I correct that x-image is internal use only, and that we aren't
> exposing an x- interface to the public user?

Yes, this is correct.

Max

Re: [Qemu-devel] [PATCH v6 03/22] qdict: Add qdict_array_split()

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1641 bytes --]

On 12/19/2013 12:47 PM, Max Reitz wrote:
> This function splits a QDict consisting of entries prefixed by
> incrementally enumerated indices into a QList of QDicts.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> Reviewed-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  include/qapi/qmp/qdict.h |  1 +
>  qobject/qdict.c          | 36 ++++++++++++++++++++++++++++++++++++
>  2 files changed, 37 insertions(+)

> +
> +/**
> + * qdict_array_split(): This function creates a QList of QDicts. Every entry in
> + * the original QDict with a key prefixed "%u.", where %u designates an unsigned
> + * integer starting at 0 and incrementally counting up, will be moved to a new
> + * QDict at index %u in the output QList with the key prefix removed. The
> + * function terminates when there is no entry in the QDict with a prefix
> + * directly (incrementally) following the last one.
> + * Example: {"0.a": 42, "0.b": 23, "1.x": 0, "3.y": 1, "o.o": 7}
> + *      (or {"1.x": 0, "3.y": 1, "0.a": 42, "o.o": 7, "0.b": 23})
> + *       => [{"a": 42, "b": 23}, {"x": 0}]
> + *      and {"3.y": 1, "o.o": 7} (remainder of the old QDict)

It took me a read through the function to realize the "and ...
(remainder)" comment refers to the input QDict, which is modified
in-place as a side effect.  Maybe change the first sentence to:

This function moves array-like elements of a QDict into a new QList of
QDicts.

But that's minor, and doesn't stop me from adding:

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]

Re: [Qemu-devel] [PATCH v6 04/22] qapi: extend qdict_flatten() for QLists

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 739 bytes --]

On 12/19/2013 12:47 PM, Max Reitz wrote:
> Reversing qdict_array_split(), qdict_flatten() should flatten QLists as
> well by interpreting them as QDicts where every entry's key is its
> index.
> 
> This allows bringing QDicts with QLists from QMP commands to the same
> form as they would be given as command-line options, thereby allowing
> them to be parsed the same way.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  qobject/qdict.c | 57 +++++++++++++++++++++++++++++++++++++++++++++++++++------
>  1 file changed, 51 insertions(+), 6 deletions(-)
> 

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]

Re: [Qemu-devel] [PATCH v6 05/22] qemu-option: Add qemu_config_parse_qdict()

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 998 bytes --]

On 12/19/2013 12:47 PM, Max Reitz wrote:
> This function basically parses command-line options given as a QDict
> replacing a config file.
> 
> For instance, the QDict {"section.opt1": 42, "section.opt2": 23}
> corresponds to the config file:
> 
> [section]
> opt1 = 42
> opt2 = 23

Thanks for the examples; that helped in looking at the code.

> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  include/qemu/config-file.h |  6 +++
>  util/qemu-config.c         | 95 ++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 101 insertions(+)

> +
> +static void config_parse_qdict_section(QDict *options, QemuOptsList *opts,
> +                                       Error **errp)
> +{

> +
> +    subopts = qemu_opts_create_nofail(opts);

You need to rebase this on top of Peter's patches that removed this
function in favor of &error_abort.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]

Re: [Qemu-devel] [PATCH v6 06/22] blkdebug: Always call read_config()

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 516 bytes --]

On 12/19/2013 12:47 PM, Max Reitz wrote:
> Move the check whether there actually is a config file into the
> read_config() function.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> Reviewed-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block/blkdebug.c | 36 +++++++++++++++++++-----------------
>  1 file changed, 19 insertions(+), 17 deletions(-)

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]

Re: [Qemu-devel] [PATCH v6 03/22] qdict: Add qdict_array_split()

[Stefan Hajnoczi]

On Thu, Dec 19, 2013 at 08:47:04PM +0100, Max Reitz wrote:
> This function splits a QDict consisting of entries prefixed by
> incrementally enumerated indices into a QList of QDicts.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> Reviewed-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  include/qapi/qmp/qdict.h |  1 +
>  qobject/qdict.c          | 36 ++++++++++++++++++++++++++++++++++++
>  2 files changed, 37 insertions(+)

Please add a test case to tests/check-qdict.c.

Re: [Qemu-devel] [PATCH v6 04/22] qapi: extend qdict_flatten() for QLists

[Stefan Hajnoczi]

On Thu, Dec 19, 2013 at 08:47:05PM +0100, Max Reitz wrote:
> Reversing qdict_array_split(), qdict_flatten() should flatten QLists as
> well by interpreting them as QDicts where every entry's key is its
> index.
> 
> This allows bringing QDicts with QLists from QMP commands to the same
> form as they would be given as command-line options, thereby allowing
> them to be parsed the same way.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  qobject/qdict.c | 57 +++++++++++++++++++++++++++++++++++++++++++++++++++------
>  1 file changed, 51 insertions(+), 6 deletions(-)

Please add a tests/check-qdict.c test case.

> diff --git a/qobject/qdict.c b/qobject/qdict.c
> index fca1902..7b6b08a 100644
> --- a/qobject/qdict.c
> +++ b/qobject/qdict.c
> @@ -477,7 +477,46 @@ static void qdict_destroy_obj(QObject *obj)
>      g_free(qdict);
>  }
>  
> -static void qdict_do_flatten(QDict *qdict, QDict *target, const char *prefix)
> +static void qdict_flatten_qdict(QDict *qdict, QDict *target,
> +                                const char *prefix);
> +
> +static void qdict_flatten_qlist(QList *qlist, QDict *target, const char *prefix)
> +{
> +    QObject *value;
> +    const QListEntry *entry;
> +    char *new_key;
> +    int i;
> +
> +    /* This function is never called with prefix == NULL, i.e., it is always
> +     * called from within qdict_flatten_q(list|dict)(). Therefore, it does not
> +     * need to remove list entries during the iteration (the whole list will be
> +     * deleted eventually anyway from qdict_flatten_qdict()). */
> +    assert(prefix);
> +
> +    entry = qlist_first(qlist);
> +
> +    for (i = 0; entry; entry = qlist_next(entry), i++) {
> +        value = qlist_entry_obj(entry);
> +
> +        qobject_incref(value);
> +        new_key = g_strdup_printf("%s.%i", prefix, i);
> +        qdict_put_obj(target, new_key, value);

It seems this operation is clobbered by what follows and should be
deleted.  Is the incref also superfluous or...

> +
> +        if (qobject_type(value) == QTYPE_QDICT) {
> +            qdict_flatten_qdict(qobject_to_qdict(value), target, new_key);
> +        } else if (qobject_type(value) == QTYPE_QLIST) {
> +            qdict_flatten_qlist(qobject_to_qlist(value), target, new_key);
> +        } else {
> +            /* All other types are moved to the target unchanged. */
> +            qobject_incref(value);

...should this one be deleted?

> +            qdict_put_obj(target, new_key, value);
> +        }
> +
> +        g_free(new_key);
> +    }

Re: [Qemu-devel] [PATCH v6 04/22] qapi: extend qdict_flatten() for QLists

[Max Reitz]

On 20.12.2013 10:46, Stefan Hajnoczi wrote:
> On Thu, Dec 19, 2013 at 08:47:05PM +0100, Max Reitz wrote:
>> Reversing qdict_array_split(), qdict_flatten() should flatten QLists as
>> well by interpreting them as QDicts where every entry's key is its
>> index.
>>
>> This allows bringing QDicts with QLists from QMP commands to the same
>> form as they would be given as command-line options, thereby allowing
>> them to be parsed the same way.
>>
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>> ---
>>   qobject/qdict.c | 57 +++++++++++++++++++++++++++++++++++++++++++++++++++------
>>   1 file changed, 51 insertions(+), 6 deletions(-)
> Please add a tests/check-qdict.c test case.
>
>> diff --git a/qobject/qdict.c b/qobject/qdict.c
>> index fca1902..7b6b08a 100644
>> --- a/qobject/qdict.c
>> +++ b/qobject/qdict.c
>> @@ -477,7 +477,46 @@ static void qdict_destroy_obj(QObject *obj)
>>       g_free(qdict);
>>   }
>>   
>> -static void qdict_do_flatten(QDict *qdict, QDict *target, const char *prefix)
>> +static void qdict_flatten_qdict(QDict *qdict, QDict *target,
>> +                                const char *prefix);
>> +
>> +static void qdict_flatten_qlist(QList *qlist, QDict *target, const char *prefix)
>> +{
>> +    QObject *value;
>> +    const QListEntry *entry;
>> +    char *new_key;
>> +    int i;
>> +
>> +    /* This function is never called with prefix == NULL, i.e., it is always
>> +     * called from within qdict_flatten_q(list|dict)(). Therefore, it does not
>> +     * need to remove list entries during the iteration (the whole list will be
>> +     * deleted eventually anyway from qdict_flatten_qdict()). */
>> +    assert(prefix);
>> +
>> +    entry = qlist_first(qlist);
>> +
>> +    for (i = 0; entry; entry = qlist_next(entry), i++) {
>> +        value = qlist_entry_obj(entry);
>> +
>> +        qobject_incref(value);
>> +        new_key = g_strdup_printf("%s.%i", prefix, i);
>> +        qdict_put_obj(target, new_key, value);
> It seems this operation is clobbered by what follows and should be
> deleted.  Is the incref also superfluous or...
>
>> +
>> +        if (qobject_type(value) == QTYPE_QDICT) {
>> +            qdict_flatten_qdict(qobject_to_qdict(value), target, new_key);
>> +        } else if (qobject_type(value) == QTYPE_QLIST) {
>> +            qdict_flatten_qlist(qobject_to_qlist(value), target, new_key);
>> +        } else {
>> +            /* All other types are moved to the target unchanged. */
>> +            qobject_incref(value);
> ...should this one be deleted?

Oh great, I've been fixing working code, then. *g*

I added the else branch in v5, I guess, it was correct not to have it 
(however, the condition in the else if was missing in v4). I'll drop it 
again, thanks.

>> +            qdict_put_obj(target, new_key, value);
>> +        }
>> +
>> +        g_free(new_key);
>> +    }

Re: [Qemu-devel] [PATCH v6 04/22] qapi: extend qdict_flatten() for QLists

[Max Reitz]

On 20.12.2013 18:19, Max Reitz wrote:
> On 20.12.2013 10:46, Stefan Hajnoczi wrote:
>> On Thu, Dec 19, 2013 at 08:47:05PM +0100, Max Reitz wrote:
>>> Reversing qdict_array_split(), qdict_flatten() should flatten QLists as
>>> well by interpreting them as QDicts where every entry's key is its
>>> index.
>>>
>>> This allows bringing QDicts with QLists from QMP commands to the same
>>> form as they would be given as command-line options, thereby allowing
>>> them to be parsed the same way.
>>>
>>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>>> ---
>>>   qobject/qdict.c | 57 
>>> +++++++++++++++++++++++++++++++++++++++++++++++++++------
>>>   1 file changed, 51 insertions(+), 6 deletions(-)
>> Please add a tests/check-qdict.c test case.
>>
>>> diff --git a/qobject/qdict.c b/qobject/qdict.c
>>> index fca1902..7b6b08a 100644
>>> --- a/qobject/qdict.c
>>> +++ b/qobject/qdict.c
>>> @@ -477,7 +477,46 @@ static void qdict_destroy_obj(QObject *obj)
>>>       g_free(qdict);
>>>   }
>>>   -static void qdict_do_flatten(QDict *qdict, QDict *target, const 
>>> char *prefix)
>>> +static void qdict_flatten_qdict(QDict *qdict, QDict *target,
>>> +                                const char *prefix);
>>> +
>>> +static void qdict_flatten_qlist(QList *qlist, QDict *target, const 
>>> char *prefix)
>>> +{
>>> +    QObject *value;
>>> +    const QListEntry *entry;
>>> +    char *new_key;
>>> +    int i;
>>> +
>>> +    /* This function is never called with prefix == NULL, i.e., it 
>>> is always
>>> +     * called from within qdict_flatten_q(list|dict)(). Therefore, 
>>> it does not
>>> +     * need to remove list entries during the iteration (the whole 
>>> list will be
>>> +     * deleted eventually anyway from qdict_flatten_qdict()). */
>>> +    assert(prefix);
>>> +
>>> +    entry = qlist_first(qlist);
>>> +
>>> +    for (i = 0; entry; entry = qlist_next(entry), i++) {
>>> +        value = qlist_entry_obj(entry);
>>> +
>>> +        qobject_incref(value);
>>> +        new_key = g_strdup_printf("%s.%i", prefix, i);
>>> +        qdict_put_obj(target, new_key, value);
>> It seems this operation is clobbered by what follows and should be
>> deleted.  Is the incref also superfluous or...
>>
>>> +
>>> +        if (qobject_type(value) == QTYPE_QDICT) {
>>> +            qdict_flatten_qdict(qobject_to_qdict(value), target, 
>>> new_key);
>>> +        } else if (qobject_type(value) == QTYPE_QLIST) {
>>> +            qdict_flatten_qlist(qobject_to_qlist(value), target, 
>>> new_key);
>>> +        } else {
>>> +            /* All other types are moved to the target unchanged. */
>>> +            qobject_incref(value);
>> ...should this one be deleted?
>
> Oh great, I've been fixing working code, then. *g*
>
> I added the else branch in v5, I guess, it was correct not to have it 
> (however, the condition in the else if was missing in v4). I'll drop 
> it again, thanks.

On second thought, that would be even more wrong. I'll leave the else 
branch and drop the qobject_incref() and qdict_put_obj() above the 
condition.

Max

>
>>> +            qdict_put_obj(target, new_key, value);
>>> +        }
>>> +
>>> +        g_free(new_key);
>>> +    }
>

Re: [Qemu-devel] [PATCH v6 04/22] qapi: extend qdict_flatten() for QLists

[Stefan Hajnoczi]

On Fri, Dec 20, 2013 at 06:23:26PM +0100, Max Reitz wrote:
> On 20.12.2013 18:19, Max Reitz wrote:
> >On 20.12.2013 10:46, Stefan Hajnoczi wrote:
> >>>diff --git a/qobject/qdict.c b/qobject/qdict.c
> >>>index fca1902..7b6b08a 100644
> >>>--- a/qobject/qdict.c
> >>>+++ b/qobject/qdict.c
> >>>@@ -477,7 +477,46 @@ static void qdict_destroy_obj(QObject *obj)
> >>>      g_free(qdict);
> >>>  }
> >>>  -static void qdict_do_flatten(QDict *qdict, QDict *target,
> >>>const char *prefix)
> >>>+static void qdict_flatten_qdict(QDict *qdict, QDict *target,
> >>>+                                const char *prefix);
> >>>+
> >>>+static void qdict_flatten_qlist(QList *qlist, QDict *target,
> >>>const char *prefix)
> >>>+{
> >>>+    QObject *value;
> >>>+    const QListEntry *entry;
> >>>+    char *new_key;
> >>>+    int i;
> >>>+
> >>>+    /* This function is never called with prefix == NULL,
> >>>i.e., it is always
> >>>+     * called from within qdict_flatten_q(list|dict)().
> >>>Therefore, it does not
> >>>+     * need to remove list entries during the iteration (the
> >>>whole list will be
> >>>+     * deleted eventually anyway from qdict_flatten_qdict()). */
> >>>+    assert(prefix);
> >>>+
> >>>+    entry = qlist_first(qlist);
> >>>+
> >>>+    for (i = 0; entry; entry = qlist_next(entry), i++) {
> >>>+        value = qlist_entry_obj(entry);
> >>>+
> >>>+        qobject_incref(value);
> >>>+        new_key = g_strdup_printf("%s.%i", prefix, i);
> >>>+        qdict_put_obj(target, new_key, value);
> >>It seems this operation is clobbered by what follows and should be
> >>deleted.  Is the incref also superfluous or...
> >>
> >>>+
> >>>+        if (qobject_type(value) == QTYPE_QDICT) {
> >>>+            qdict_flatten_qdict(qobject_to_qdict(value),
> >>>target, new_key);
> >>>+        } else if (qobject_type(value) == QTYPE_QLIST) {
> >>>+            qdict_flatten_qlist(qobject_to_qlist(value),
> >>>target, new_key);
> >>>+        } else {
> >>>+            /* All other types are moved to the target unchanged. */
> >>>+            qobject_incref(value);
> >>...should this one be deleted?
> >
> >Oh great, I've been fixing working code, then. *g*
> >
> >I added the else branch in v5, I guess, it was correct not to have
> >it (however, the condition in the else if was missing in v4). I'll
> >drop it again, thanks.
> 
> On second thought, that would be even more wrong. I'll leave the
> else branch and drop the qobject_incref() and qdict_put_obj() above
> the condition.

I agree, that sounds like the right solution.