[Qemu-devel] [PATCH v3 00/21] blkdebug/blkverify: Allow QMP configuration

[Qemu-devel] [PATCH v3 00/21] 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.


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.


Max Reitz (21):
  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: Allow recursive "file"s
  qemu-iotests: Fix output of test 051
  blockdev: Move "file" to legacy_opts
  blkdebug: Allow command-line file configuration
  blkdebug: Make filename optional
  blkverify: Allow command-line configuration
  blkverify: Don't require protocol filename
  blkdebug: Alias "errno" as "error"
  qapi: QMP interface for blkdebug and blkverify
  qemu-io: Make filename optional
  iotests: Test new blkdebug/blkverify interface

 block.c                    |  58 ++++++++++---
 block/blkdebug.c           |  90 ++++++++++++++------
 block/blkverify.c          |  65 ++++++++++-----
 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      |   3 +-
 include/qapi/qmp/qdict.h   |   1 +
 include/qemu/config-file.h |   6 ++
 qapi-schema.json           |  94 ++++++++++++++++++++-
 qemu-io.c                  |  10 ++-
 qobject/qdict.c            |  81 +++++++++++++++++-
 tests/qemu-iotests/051.out |   2 +-
 tests/qemu-iotests/071     | 201 +++++++++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/071.out |  73 ++++++++++++++++
 tests/qemu-iotests/group   |   1 +
 util/qemu-config.c         |  95 +++++++++++++++++++++
 22 files changed, 729 insertions(+), 92 deletions(-)
 create mode 100755 tests/qemu-iotests/071
 create mode 100644 tests/qemu-iotests/071.out

-- 
1.8.5.1

[Qemu-devel] [PATCH v3 01/21] 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>
---
 block/blkdebug.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/block/blkdebug.c b/block/blkdebug.c
index 37cf028..655efd7 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 v3 02/21] 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>
---
 block/blkdebug.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/block/blkdebug.c b/block/blkdebug.c
index 655efd7..7c8592d 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 v3 03/21] 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>
---
 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 v3 04/21] 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 | 45 +++++++++++++++++++++++++++++++++++++++++----
 1 file changed, 41 insertions(+), 4 deletions(-)

diff --git a/qobject/qdict.c b/qobject/qdict.c
index fca1902..d7ce4b3 100644
--- a/qobject/qdict.c
+++ b/qobject/qdict.c
@@ -477,7 +477,40 @@ 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()). Also, prefix can
+     * never be NULL. */
+
+    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 {
+            qdict_flatten_qlist(qobject_to_qlist(value), target, new_key);
+        }
+    }
+}
+
+static void qdict_flatten_qdict(QDict *qdict, QDict *target, const char *prefix)
 {
     QObject *value;
     const QDictEntry *entry, *next;
@@ -500,8 +533,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. */
@@ -531,7 +568,7 @@ static void qdict_do_flatten(QDict *qdict, QDict *target, const char *prefix)
  */
 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 v3 05/21] 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 v3 06/21] 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>
---
 block/blkdebug.c | 36 +++++++++++++++++++-----------------
 1 file changed, 19 insertions(+), 17 deletions(-)

diff --git a/block/blkdebug.c b/block/blkdebug.c
index 7c8592d..f7c898f 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 v3 07/21] 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>
---
 block/blkdebug.c | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

diff --git a/block/blkdebug.c b/block/blkdebug.c
index f7c898f..584b1d6 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 v3 08/21] 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>
---
 block.c               | 27 ++++++++++++++++++++++++---
 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, 41 insertions(+), 17 deletions(-)

diff --git a/block.c b/block.c
index 13f001a..9e197b3 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,26 @@ 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");
+            ret = -EINVAL;
+            goto fail;
+        }
+        QDECREF(options);
+
+        bs = bdrv_find(reference);
+        if (!bs) {
+            error_setg(errp, "Cannot find block device '%s'", reference);
+            ret = -ENODEV;
+            goto fail;
+        }
+        bdrv_ref(bs);
+        *pbs = bs;
+        return 0;
+    }
+
     bs = bdrv_new("");
     bs->options = options;
     options = qdict_clone_shallow(options);
@@ -1124,7 +1145,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 584b1d6..35c2dae 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 b4ae50f..7b80b2f 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);
@@ -1693,7 +1693,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 v3 09/21] 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>
---
 block.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/block.c b/block.c
index 9e197b3..4a2c251 100644
--- a/block.c
+++ b/block.c
@@ -1058,6 +1058,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;
 
@@ -1144,9 +1145,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 v3 10/21] 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 | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/block.c b/block.c
index 4a2c251..33ef995 100644
--- a/block.c
+++ b/block.c
@@ -1147,11 +1147,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 */
@@ -1180,7 +1183,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 v3 11/21] 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.

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

diff --git a/block.c b/block.c
index 33ef995..bebf080 100644
--- a/block.c
+++ b/block.c
@@ -950,14 +950,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);
@@ -972,10 +977,12 @@ int bdrv_file_open(BlockDriverState **pbs, const char *filename,
 
 fail:
     QDECREF(options);
-    if (!bs->drv) {
-        QDECREF(bs->options);
+    if (bs) {
+        if (!bs->drv) {
+            QDECREF(bs->options);
+        }
+        bdrv_unref(bs);
     }
-    bdrv_unref(bs);
     return ret;
 }
 
-- 
1.8.5.1

[Qemu-devel] [PATCH v3 12/21] qemu-iotests: Fix output of test 051

[Max Reitz]

Allowing nested file formats now 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.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 tests/qemu-iotests/051.out | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

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 v3 13/21] 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>
---
 blockdev.c | 19 ++++++++++---------
 1 file changed, 10 insertions(+), 9 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index 44755e1..166bff8 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 v3 14/21] 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 | 34 +++++++++++++++++++++++++---------
 1 file changed, 25 insertions(+), 9 deletions(-)

diff --git a/block/blkdebug.c b/block/blkdebug.c
index 35c2dae..b032fff 100644
--- a/block/blkdebug.c
+++ b/block/blkdebug.c
@@ -368,13 +368,35 @@ static QemuOptsList runtime_opts = {
     },
 };
 
+static int open_image(BlockDriverState **pbs, const char *fname, QDict *options,
+                      const char *bdref_key, int flags, Error **errp)
+{
+    QDict *image_options;
+    int ret;
+    char *bdref_key_dot;
+
+    bdref_key_dot = g_strdup_printf("%s.", bdref_key);
+    qdict_extract_subqdict(options, &image_options, bdref_key_dot);
+    g_free(bdref_key_dot);
+
+    /* Never use bdrv_open() here; if just a filename is given without further
+       options, bdrv_open() will try to open it with the block driver we are
+       about to test. bdrv_file_open() never does this. */
+    ret = bdrv_file_open(pbs, fname, qdict_get_try_str(options, bdref_key),
+                         image_options, flags, errp);
+
+    qdict_del(options, bdref_key);
+
+    return ret;
+}
+
 static int blkdebug_open(BlockDriverState *bs, QDict *options, int flags,
                          Error **errp)
 {
     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 +418,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 = open_image(&bs->file, qemu_opt_get(opts, "x-image"), options, "image",
+                     flags, &local_err);
     if (ret < 0) {
         error_propagate(errp, local_err);
         goto fail;
-- 
1.8.5.1

[Qemu-devel] [PATCH v3 15/21] blkdebug: Make filename optional

[Max Reitz]

There is no problem in giving no filename; in this case, it has to be
specified through the "image" option.

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

diff --git a/block/blkdebug.c b/block/blkdebug.c
index b032fff..6b59216 100644
--- a/block/blkdebug.c
+++ b/block/blkdebug.c
@@ -327,8 +327,10 @@ static void blkdebug_parse_filename(const char *filename, QDict *options,
     /* Parse the blkdebug: prefix */
     if (!strstart(filename, "blkdebug:", &filename)) {
         /* 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));
+           present in the QDict; if a filename is given, pass it */
+        if (filename) {
+            qdict_put(options, "x-image", qstring_from_str(filename));
+        }
         return;
     }
 
-- 
1.8.5.1

[Qemu-devel] [PATCH v3 16/21] 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 | 59 ++++++++++++++++++++++++++++++++++++-------------------
 1 file changed, 39 insertions(+), 20 deletions(-)

diff --git a/block/blkverify.c b/block/blkverify.c
index c6eb287..8bf81b5 100644
--- a/block/blkverify.c
+++ b/block/blkverify.c
@@ -116,13 +116,46 @@ static QemuOptsList runtime_opts = {
     },
 };
 
+static int open_image(BlockDriverState **pbs, const char *fname, QDict *options,
+                      const char *bdref_key, int flags, Error **errp)
+{
+    QDict *image_options;
+    int ret;
+    char *bdref_key_dot;
+
+    bdref_key_dot = g_strdup_printf("%s.", bdref_key);
+    qdict_extract_subqdict(options, &image_options, bdref_key_dot);
+    g_free(bdref_key_dot);
+
+    if (fname) {
+        /* If a filename is given, use bdrv_open() in order to use the correct
+           block driver (instead of just opening the raw image). */
+
+        if (qdict_get_try_str(options, bdref_key)) {
+            error_setg(errp, "Cannot reference an existing block device while "
+                       "giving a filename");
+            ret = -EINVAL;
+            goto fail;
+        }
+
+        *pbs = bdrv_new("");
+        ret = bdrv_open(*pbs, fname, image_options, flags, NULL, errp);
+    } else {
+        ret = bdrv_file_open(pbs, NULL, qdict_get_try_str(options, bdref_key),
+                             image_options, flags, errp);
+    }
+
+fail:
+    qdict_del(options, bdref_key);
+    return ret;
+}
+
 static int blkverify_open(BlockDriverState *bs, QDict *options, int flags,
                           Error **errp)
 {
     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 +166,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 = open_image(&bs->file, qemu_opt_get(opts, "x-raw"), options, "raw",
+                     flags, &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 = open_image(&s->test_file, qemu_opt_get(opts, "x-image"), options,
+                     "test", flags, &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 v3 17/21] 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). Contrary to the current reaction, this
is no longer a problem; both images may be specified through the "test"
and "raw" options, respectively.

If a filename has been given, pass it as the test image filename.

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

diff --git a/block/blkverify.c b/block/blkverify.c
index 8bf81b5..a903680 100644
--- a/block/blkverify.c
+++ b/block/blkverify.c
@@ -78,7 +78,11 @@ 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; if a filename is given, pass it */
+        if (filename) {
+            qdict_put(options, "x-image", qstring_from_str(filename));
+        }
         return;
     }
 
-- 
1.8.5.1

[Qemu-devel] [PATCH v3 18/21] blkdebug: Alias "errno" as "error"

[Max Reitz]

Introduce an alias "error" for "errno", since using the latter for QMP
is sure to result in various syntax errors due to the name being used
directly as an identifier.

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

diff --git a/block/blkdebug.c b/block/blkdebug.c
index 6b59216..87993f9 100644
--- a/block/blkdebug.c
+++ b/block/blkdebug.c
@@ -100,6 +100,10 @@ static QemuOptsList inject_error_opts = {
             .type = QEMU_OPT_NUMBER,
         },
         {
+            .name = "error",
+            .type = QEMU_OPT_NUMBER,
+        },
+        {
             .name = "sector",
             .type = QEMU_OPT_NUMBER,
         },
@@ -232,7 +236,8 @@ static int add_rule(QemuOpts *opts, void *opaque)
     /* Parse action-specific options */
     switch (d->action) {
     case ACTION_INJECT_ERROR:
-        rule->options.inject.error = qemu_opt_get_number(opts, "errno", EIO);
+        rule->options.inject.error = qemu_opt_get_number(opts,
+                qemu_opt_get(opts, "errno") ? "errno" : "error", EIO);
         rule->options.inject.once  = qemu_opt_get_bool(opts, "once", 0);
         rule->options.inject.immediately =
             qemu_opt_get_bool(opts, "immediately", 0);
-- 
1.8.5.1

[Qemu-devel] [PATCH v3 19/21] 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 | 94 +++++++++++++++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 90 insertions(+), 4 deletions(-)

diff --git a/qapi-schema.json b/qapi-schema.json
index d6f8615..b4f86c7 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -4166,6 +4166,94 @@
             '*pass-discard-other': 'bool' } }
 
 ##
+# @BlkdebugInjectErrorOptions
+#
+# Describes a single error injection for blkdebug.
+#
+# @event:       trigger event name
+#
+# @state:       #optional the state identifier blkdebug needs to be in to
+#               actually trigger the event; defaults to "any"
+#
+# @error:       #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': 'str',
+            '*state': 'int',
+            '*error': 'int',
+            '*sector': 'int',
+            '*once': 'bool',
+            '*immediately': 'bool' } }
+
+##
+# @BlkdebugSetStateOptions
+#
+# Describes a single state-change event for blkdebug.
+#
+# @event:       trigger event name
+#
+# @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': 'str',
+            '*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 +4277,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 v3 20/21] 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>
---
 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 v3 21/21] iotests: Test new blkdebug/blkverify interface

[Max Reitz]

Add a test for the new blkdebug/blkverify interface.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 tests/qemu-iotests/071     | 201 +++++++++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/071.out |  73 ++++++++++++++++
 tests/qemu-iotests/group   |   1 +
 3 files changed, 275 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..4be525e
--- /dev/null
+++ b/tests/qemu-iotests/071
@@ -0,0 +1,201 @@
+#!/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=128K
+
+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
+
+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
+
+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 file.driver=$IMGFMT,file.file.driver=blkdebug,file.file.inject-error.event=l2_load,file.file.filename=$TEST_IMG" \
+         -c 'read -P 42 0x38000 512'
+
+echo
+echo "=== Testing blkdebug on existing block device ==="
+echo
+
+$QEMU_IO -c 'write -P 42 0 512' "$TEST_IMG" | _filter_qemu_io
+
+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 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..d6205cd
--- /dev/null
+++ b/tests/qemu-iotests/071.out
@@ -0,0 +1,73 @@
+QA output created by 071
+
+=== Testing blkverify through filename ===
+
+Formatting 'TEST_DIR/t.IMGFMT.base', fmt=IMGFMT size=131072 
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=131072 
+read 512/512 bytes at offset 0
+512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+write failed: Input/output error
+read failed: Input/output error
+
+=== Testing blkverify through file blockref ===
+
+Formatting 'TEST_DIR/t.IMGFMT.base', fmt=IMGFMT size=131072 
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=131072 
+read 512/512 bytes at offset 0
+512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+write failed: Input/output error
+read failed: Input/output error
+
+=== 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 ===
+
+wrote 512/512 bytes at offset 0
+512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+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 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 v4 04/21] 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>
---
v4:
 - fixed a memory leak due to a missing g_free(new_key) in
   qdict_flatten_qlist()
---
 qobject/qdict.c | 47 +++++++++++++++++++++++++++++++++++++++++++----
 1 file changed, 43 insertions(+), 4 deletions(-)

diff --git a/qobject/qdict.c b/qobject/qdict.c
index fca1902..a90b7f4 100644
--- a/qobject/qdict.c
+++ b/qobject/qdict.c
@@ -477,7 +477,42 @@ 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()). Also, prefix can
+     * never be NULL. */
+
+    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 {
+            qdict_flatten_qlist(qobject_to_qlist(value), target, new_key);
+        }
+
+        g_free(new_key);
+    }
+}
+
+static void qdict_flatten_qdict(QDict *qdict, QDict *target, const char *prefix)
 {
     QObject *value;
     const QDictEntry *entry, *next;
@@ -500,8 +535,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. */
@@ -531,7 +570,7 @@ static void qdict_do_flatten(QDict *qdict, QDict *target, const char *prefix)
  */
 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

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

[Fam Zheng]

On 2013年12月12日 02:10, 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>
> ---

The logic looks fine, just a few questions about assertions below.

>   qobject/qdict.c | 45 +++++++++++++++++++++++++++++++++++++++++----
>   1 file changed, 41 insertions(+), 4 deletions(-)
>
> diff --git a/qobject/qdict.c b/qobject/qdict.c
> index fca1902..d7ce4b3 100644
> --- a/qobject/qdict.c
> +++ b/qobject/qdict.c
> @@ -477,7 +477,40 @@ 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()). Also, prefix can
> +     * never be NULL. */

Then maybe assert this?

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

And maybe assert the type are not something other than dict and list?


> +            qdict_flatten_qlist(qobject_to_qlist(value), target, new_key);
> +        }
> +    }
> +}
> +
> +static void qdict_flatten_qdict(QDict *qdict, QDict *target, const char *prefix)
>   {
>       QObject *value;
>       const QDictEntry *entry, *next;
> @@ -500,8 +533,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. */
> @@ -531,7 +568,7 @@ static void qdict_do_flatten(QDict *qdict, QDict *target, const char *prefix)
>    */
>   void qdict_flatten(QDict *qdict)
>   {
> -    qdict_do_flatten(qdict, qdict, NULL);
> +    qdict_flatten_qdict(qdict, qdict, NULL);

If there was an assumption that the top level is a qdict, also assert 
that here?

Thanks,

Fam

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

[Fam Zheng]

On 2013年12月12日 02:10, 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 | 45 +++++++++++++++++++++++++++++++++++++++++----
>   1 file changed, 41 insertions(+), 4 deletions(-)
>
> diff --git a/qobject/qdict.c b/qobject/qdict.c
> index fca1902..d7ce4b3 100644
> --- a/qobject/qdict.c
> +++ b/qobject/qdict.c
> @@ -477,7 +477,40 @@ 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()). Also, prefix can
> +     * never be NULL. */
> +
> +    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 {
> +            qdict_flatten_qlist(qobject_to_qlist(value), target, new_key);
> +        }
> +    }
> +}
> +
> +static void qdict_flatten_qdict(QDict *qdict, QDict *target, const char *prefix)

Sorry for replying twice on the same patch. But it would be nice if you 
could add a few comments on this function with some examples, as you did 
with qdict_array_split, I appreciated that. :)

Fam

Re: [Qemu-devel] [PATCH v3 08/21] block: Allow reference for bdrv_file_open()

[Fam Zheng]

On 2013年12月12日 02:11, Max Reitz wrote:
> 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>
> ---
>   block.c               | 27 ++++++++++++++++++++++++---
>   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, 41 insertions(+), 17 deletions(-)
>
> diff --git a/block.c b/block.c
> index 13f001a..9e197b3 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,26 @@ 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");
> +            ret = -EINVAL;
> +            goto fail;
> +        }
> +        QDECREF(options);

QDECREF called...

> +
> +        bs = bdrv_find(reference);
> +        if (!bs) {
> +            error_setg(errp, "Cannot find block device '%s'", reference);
> +            ret = -ENODEV;
> +            goto fail;

Jump to fail for QDECREF again. Duplicated?

Fam

Re: [Qemu-devel] [PATCH v3 21/21] iotests: Test new blkdebug/blkverify interface

[Fam Zheng]

On 2013年12月12日 02:11, Max Reitz wrote:
> Add a test for the new blkdebug/blkverify interface.
>
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>   tests/qemu-iotests/071     | 201 +++++++++++++++++++++++++++++++++++++++++++++
>   tests/qemu-iotests/071.out |  73 ++++++++++++++++
>   tests/qemu-iotests/group   |   1 +
>   3 files changed, 275 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..4be525e
> --- /dev/null
> +++ b/tests/qemu-iotests/071
> @@ -0,0 +1,201 @@
> +#!/bin/bash

Is is intended to use hand coded json instead of iotests.py for QMP test 
here?

Fam

Re: [Qemu-devel] [PATCH v3 00/21] blkdebug/blkverify: Allow QMP configuration

[Fam Zheng]

On 2013年12月12日 02:10, Max Reitz wrote:
> 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.
>
>
> 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.
>

Reviewed the patches and also tried the test case. Looks good to me, I 
have only a few questions in some patches.

Thanks,
Fam

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

[Max Reitz]

On 12.12.2013 07:17, Fam Zheng wrote:
> On 2013年12月12日 02:10, 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>
>> ---
>
> The logic looks fine, just a few questions about assertions below.
>
>> qobject/qdict.c | 45 +++++++++++++++++++++++++++++++++++++++++----
>> 1 file changed, 41 insertions(+), 4 deletions(-)
>>
>> diff --git a/qobject/qdict.c b/qobject/qdict.c
>> index fca1902..d7ce4b3 100644
>> --- a/qobject/qdict.c
>> +++ b/qobject/qdict.c
>> @@ -477,7 +477,40 @@ 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()). Also, 
>> prefix can
>> + * never be NULL. */
>
> Then maybe assert this?

Yes, I'll do that.

>
>> +
>> + 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 {
>
> And maybe assert the type are not something other than dict and list?

I think I rather forgot to support other types. Those should be moved to 
the target QDict, just as qdict_flatten_qdict() does it.

>
>> + qdict_flatten_qlist(qobject_to_qlist(value), target, new_key);
>> + }
>> + }
>> +}
>> +
>> +static void qdict_flatten_qdict(QDict *qdict, QDict *target, const 
>> char *prefix)
>> {
>> QObject *value;
>> const QDictEntry *entry, *next;
>> @@ -500,8 +533,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. */
>> @@ -531,7 +568,7 @@ static void qdict_do_flatten(QDict *qdict, QDict 
>> *target, const char *prefix)
>> */
>> void qdict_flatten(QDict *qdict)
>> {
>> - qdict_do_flatten(qdict, qdict, NULL);
>> + qdict_flatten_qdict(qdict, qdict, NULL);
>
> If there was an assumption that the top level is a qdict, also assert 
> that here?

Well, the type is already QDict *, therefore I think we are safe to 
assume it is indeed one – if that is what you meant. ;-)

The question to me would be whether we should be supporting QLists here, 
too. Since this would make qdict_flatten_qlist() more complex, I think 
we should not do that until it is required.

Thank you,

Max

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

[Max Reitz]

On 12.12.2013 07:21, Fam Zheng wrote:
> On 2013年12月12日 02:10, 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 | 45 +++++++++++++++++++++++++++++++++++++++++----
>> 1 file changed, 41 insertions(+), 4 deletions(-)
>>
>> diff --git a/qobject/qdict.c b/qobject/qdict.c
>> index fca1902..d7ce4b3 100644
>> --- a/qobject/qdict.c
>> +++ b/qobject/qdict.c
>> @@ -477,7 +477,40 @@ 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()). Also, 
>> prefix can
>> + * never be NULL. */
>> +
>> + 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 {
>> + qdict_flatten_qlist(qobject_to_qlist(value), target, new_key);
>> + }
>> + }
>> +}
>> +
>> +static void qdict_flatten_qdict(QDict *qdict, QDict *target, const 
>> char *prefix)
>
> Sorry for replying twice on the same patch. But it would be nice if 
> you could add a few comments on this function with some examples, as 
> you did with qdict_array_split, I appreciated that. :)

Okay.

Max

Re: [Qemu-devel] [PATCH v3 08/21] block: Allow reference for bdrv_file_open()

[Max Reitz]

On 12.12.2013 09:01, Fam Zheng wrote:
> On 2013年12月12日 02:11, Max Reitz wrote:
>> 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>
>> ---
>> block.c | 27 ++++++++++++++++++++++++---
>> 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, 41 insertions(+), 17 deletions(-)
>>
>> diff --git a/block.c b/block.c
>> index 13f001a..9e197b3 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,26 @@ 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");
>> + ret = -EINVAL;
>> + goto fail;
>> + }
>> + QDECREF(options);
>
> QDECREF called...
>
>> +
>> + bs = bdrv_find(reference);
>> + if (!bs) {
>> + error_setg(errp, "Cannot find block device '%s'", reference);
>> + ret = -ENODEV;
>> + goto fail;
>
> Jump to fail for QDECREF again. Duplicated?

Oh, right, thanks.

Max

Re: [Qemu-devel] [PATCH v3 21/21] iotests: Test new blkdebug/blkverify interface

[Max Reitz]

On 12.12.2013 11:41, Fam Zheng wrote:
> On 2013年12月12日 02:11, Max Reitz wrote:
>> Add a test for the new blkdebug/blkverify interface.
>>
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>> ---
>>   tests/qemu-iotests/071     | 201 
>> +++++++++++++++++++++++++++++++++++++++++++++
>>   tests/qemu-iotests/071.out |  73 ++++++++++++++++
>>   tests/qemu-iotests/group   |   1 +
>>   3 files changed, 275 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..4be525e
>> --- /dev/null
>> +++ b/tests/qemu-iotests/071
>> @@ -0,0 +1,201 @@
>> +#!/bin/bash
>
> Is is intended to use hand coded json instead of iotests.py for QMP 
> test here?

I first tried to use Python, but I hit problems with 
human-monitor-command and qemu-io. qemu-io prints errors to stderr, 
therefore they are not returned via QMP. iotests.py discards stderr, 
therefore there was no way to actually check for the qemu-io return status.

I could have added a function to iotests.py, but then I would have to 
manually compare and filter the qemu-io output with a reference. All 
these functions are already there for shell script tests.

I guess I should add this explanation to the commit message, though…

Max