[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.


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

backport diff against v3:
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:[0015] [FC] 'qapi: extend qdict_flatten() for QLists'
005/22:[down] 'qdict: Remove delete from qdict_flatten_qdict()'
006/22:[----] [--] 'qemu-option: Add qemu_config_parse_qdict()'
007/22:[----] [--] 'blkdebug: Always call read_config()'
008/22:[----] [--] 'blkdebug: Use command-line in read_config()'
009/22:[0006] [FC] 'block: Allow reference for bdrv_file_open()'
010/22:[----] [--] 'block: Pass reference to bdrv_file_open()'
011/22:[----] [--] 'block: Allow block devices without files'
012/22:[----] [--] 'block: Allow recursive "file"s'
013/22:[----] [--] 'qemu-iotests: Fix output of test 051'
014/22:[----] [--] 'blockdev: Move "file" to legacy_opts'
015/22:[----] [--] 'blkdebug: Allow command-line file configuration'
016/22:[----] [--] 'blkdebug: Make filename optional'
017/22:[----] [--] 'blkverify: Allow command-line configuration'
018/22:[----] [--] 'blkverify: Don't require protocol filename'
019/22:[----] [--] 'blkdebug: Alias "errno" as "error"'
020/22:[----] [--] 'qapi: QMP interface for blkdebug and blkverify'
021/22:[----] [--] 'qemu-io: Make filename optional'
022/22:[----] [--] 'iotests: Test new blkdebug/blkverify interface'

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.


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
  qdict: Remove delete from qdict_flatten_qdict()
  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                    |  56 ++++++++++---
 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            | 111 ++++++++++++++++++++-----
 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, 741 insertions(+), 108 deletions(-)
 create mode 100755 tests/qemu-iotests/071
 create mode 100644 tests/qemu-iotests/071.out

-- 
1.8.5.1

[Qemu-devel] [PATCH v5 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>
---
 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 v5 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>
---
 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 v5 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>
---
 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 v5 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 | 58 +++++++++++++++++++++++++++++++++++++++++++++++++++------
 1 file changed, 52 insertions(+), 6 deletions(-)

diff --git a/qobject/qdict.c b/qobject/qdict.c
index fca1902..1d0e66c 100644
--- a/qobject/qdict.c
+++ b/qobject/qdict.c
@@ -477,7 +477,47 @@ 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. */
+    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 +540,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 +570,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 v5 05/22] qdict: Remove delete from qdict_flatten_qdict()

[Max Reitz]

delete is always set to true, therefore it can be removed.

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

diff --git a/qobject/qdict.c b/qobject/qdict.c
index 1d0e66c..ec42b1c 100644
--- a/qobject/qdict.c
+++ b/qobject/qdict.c
@@ -520,18 +520,15 @@ static void qdict_flatten_qlist(QList *qlist, QDict *target, const char *prefix)
 static void qdict_flatten_qdict(QDict *qdict, QDict *target, const char *prefix)
 {
     QObject *value;
-    const QDictEntry *entry, *next;
+    const QDictEntry *entry;
     char *new_key;
-    bool delete;
 
     entry = qdict_first(qdict);
 
     while (entry != NULL) {
 
-        next = qdict_next(qdict, entry);
         value = qdict_entry_value(entry);
         new_key = NULL;
-        delete = false;
 
         if (prefix) {
             new_key = g_strdup_printf("%s.%s", prefix, entry->key);
@@ -542,29 +539,21 @@ static void qdict_flatten_qdict(QDict *qdict, QDict *target, const char *prefix)
              * itself disappears. */
             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. */
             qobject_incref(value);
             qdict_put_obj(target, new_key, value);
-            delete = true;
         }
 
         g_free(new_key);
 
-        if (delete) {
-            qdict_del(qdict, entry->key);
-
-            /* Restart loop after modifying the iterated QDict */
-            entry = qdict_first(qdict);
-            continue;
-        }
+        qdict_del(qdict, entry->key);
 
-        entry = next;
+        /* Restart loop after modifying the iterated QDict */
+        entry = qdict_first(qdict);
     }
 }
 
-- 
1.8.5.1

[Qemu-devel] [PATCH v5 06/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 v5 07/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>
---
 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 v5 08/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>
---
 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 v5 09/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>
---
 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 v5 10/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>
---
 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 v5 11/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 | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/block.c b/block.c
index bef4f82..9659eb5 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 */
@@ -1178,7 +1181,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 v5 12/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.

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 9659eb5..9222669 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);
@@ -970,10 +975,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 v5 13/22] 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 v5 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>
---
 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 v5 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 | 34 +++++++++++++++++++++++++---------
 1 file changed, 25 insertions(+), 9 deletions(-)

diff --git a/block/blkdebug.c b/block/blkdebug.c
index c73a6cf..0c800ae 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 v5 16/22] 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 0c800ae..fdfc6b0 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 v5 17/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 | 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 v5 18/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). 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 v5 19/22] 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 fdfc6b0..522a766 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 v5 20/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 | 94 +++++++++++++++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 90 insertions(+), 4 deletions(-)

diff --git a/qapi-schema.json b/qapi-schema.json
index c3c939c..6ce016c 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 v5 21/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>
---
 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 v5 22/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     | 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

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

[Max Reitz]

Sorry, the subject should say "v5", obviously (but since the patches 
itself are fine, I hope it is okay not to resend the series).

Max

Re: [Qemu-devel] [PATCH v5 05/22] qdict: Remove delete from qdict_flatten_qdict()

[Kevin Wolf]

Am 13.12.2013 um 18:10 hat Max Reitz geschrieben:
> delete is always set to true, therefore it can be removed.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>

Nope, this can't be right.

delete is always set, except for simple types in the top-level QDict.
They get deleted now instead of being left alone.

But it coincides pretty well with the new loop condition. Leaving
uninteresting stuff aside, the loop looks now like this:

    while (entry != NULL) {
        ...
        entry = qdict_first(qdict);
    }

So the loop only terminates when all elements have been removed. How
lucky we are that it really does remove all elements now!


Perhaps some other code is broken, or I missed something else, but
qemu-iotests 067 segfaults after this patch.

Kevin

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

[Kevin Wolf]

Am 13.12.2013 um 18:10 hat Max Reitz geschrieben:
> Use an Error variable in the read_config() function.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>

Reviewed-by: Kevin Wolf <kwolf@redhat.com>

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

[Kevin Wolf]

Am 13.12.2013 um 18:10 hat Max Reitz geschrieben:
> 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>

x-image must die, but why not, as an intermediate step.

Reviewed-by: Kevin Wolf <kwolf@redhat.com>

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

[Kevin Wolf]

Am 13.12.2013 um 18:10 hat Max Reitz geschrieben:
> 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>

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

[Kevin Wolf]

Am 13.12.2013 um 18:10 hat Max Reitz geschrieben:
> 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>

Reviewed-by: Kevin Wolf <kwolf@redhat.com>

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

[Kevin Wolf]

Am 13.12.2013 um 18:10 hat Max Reitz geschrieben:
> 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>

Re: [Qemu-devel] [PATCH v5 08/22] blkdebug: Use command-line in read_config()

[Kevin Wolf]

Am 13.12.2013 um 18:10 hat Max Reitz geschrieben:
> 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>

Re: [Qemu-devel] [PATCH v5 09/22] block: Allow reference for bdrv_file_open()

[Kevin Wolf]

Am 13.12.2013 um 18:10 hat Max Reitz geschrieben:
> 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>

> @@ -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;
> +        }

I suspect this could in fact be an assertion. Users shouldn't have any
way to provoke a call with a reference _and_ options/filename set.

Doesn't make the code less correct, of course, so:

Reviewed-by: Kevin Wolf <kwolf@redhat.com>

Re: [Qemu-devel] [PATCH v5 10/22] block: Pass reference to bdrv_file_open()

[Kevin Wolf]

Am 13.12.2013 um 18:10 hat Max Reitz geschrieben:
> 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>

Ah well, I see. Users do have a way to specify both options and a
reference this way.

Reviewed-by: Kevin Wolf <kwolf@redhat.com>

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

[Eric Blake]

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

On 12/13/2013 10:10 AM, 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 | 58 +++++++++++++++++++++++++++++++++++++++++++++++++++------

> +    /* 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. */

The comment sounds redundant: "never called with prefix == NULL ...
prefix can never be NULL".

-- 
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 v5 11/22] block: Allow block devices without files

[Kevin Wolf]

Am 13.12.2013 um 18:10 hat Max Reitz geschrieben:
> 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 bef4f82..9659eb5 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 */
> @@ -1178,7 +1181,7 @@ int bdrv_open(BlockDriverState *bs, const char *filename, QDict *options,

Manually adding more context before the next hunk:

>     if (!drv) {
>         ret = find_image_format(file, filename, &drv, &local_err);
>     }

We can get file == NULL now, which will cause a segfault in
find_image_format. (For a reproducer of the segfault, try
'x86_64-softmmu/qemu-system-x86_64 -drive foo=bar')

Kevin

Re: [Qemu-devel] [PATCH v5 12/22] block: Allow recursive "file"s

[Kevin Wolf]

Am 13.12.2013 um 18:10 hat Max Reitz geschrieben:
> 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>

Hm, does this do what I think it does?

$ ./qemu-img convert -O qcow2 /home/kwolf/images/hd.img /tmp/hd.qcow2
$ ./qemu-img convert -f raw -O qcow2 /tmp/hd.qcow2 /tmp/hd.qcow2.qcow2
$ x86_64-softmmu/qemu-system-x86_64 -drive driver=qcow2,file.driver=qcow2,file.file.driver=file,file.file.filename=/tmp/hd.qcow2.qcow2

I can't decide whether this is awesomeness or insanity, but in any case
it works with this patch. :-)

Worth a qemu-iotests case, I think.

>  block.c | 17 ++++++++++++-----
>  1 file changed, 12 insertions(+), 5 deletions(-)
> 
> diff --git a/block.c b/block.c
> index 9659eb5..9222669 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);
> @@ -970,10 +975,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;
>  }

Not sure why this hunk is needed, but anyway:

Reviewed-by: Kevin Wolf <kwolf@redhat.com>

Re: [Qemu-devel] [PATCH v5 13/22] qemu-iotests: Fix output of test 051

[Eric Blake]

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

On 12/13/2013 10:10 AM, Max Reitz wrote:
> 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.

Shouldn't this be squashed with the patch that changed the error
message, so that a bisection doesn't hit the broken test?  Besides,
touching the testsuite at the same time as the rest of the code base
shows that you are aware of the changes caused by your code.

-- 
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 v5 13/22] qemu-iotests: Fix output of test 051

[Kevin Wolf]

Am 13.12.2013 um 18:10 hat Max Reitz geschrieben:
> 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'

Shows that our error messages aren't really good yet. bdrv_file_open()
shouldn't be talking about device names, it's the wrong layer. But
that's an unrelated problem.

Reviewed-by: Kevin Wolf <kwolf@redhat.com>

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

[Kevin Wolf]

Am 13.12.2013 um 18:10 hat Max Reitz geschrieben:
> 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>

This is a good one, I've been meaning to do this for a while. One thing
that is left in blockdev_init() and should be removed eventually is the
handling of 'no medium'. But it's unrelated and I'm not sure yet how to
do it right.

Reviewed-by: Kevin Wolf <kwolf@redhat.com>

Re: [Qemu-devel] [PATCH v5 12/22] block: Allow recursive "file"s

[Eric Blake]

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

On 12/13/2013 01:19 PM, Kevin Wolf wrote:
> Am 13.12.2013 um 18:10 hat Max Reitz geschrieben:
>> 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>
> 
> Hm, does this do what I think it does?

That depends on what you think it's doing :)

> 
> $ ./qemu-img convert -O qcow2 /home/kwolf/images/hd.img /tmp/hd.qcow2
> $ ./qemu-img convert -f raw -O qcow2 /tmp/hd.qcow2 /tmp/hd.qcow2.qcow2
> $ x86_64-softmmu/qemu-system-x86_64 -drive driver=qcow2,file.driver=qcow2,file.file.driver=file,file.file.filename=/tmp/hd.qcow2.qcow2
> 
> I can't decide whether this is awesomeness or insanity, but in any case
> it works with this patch. :-)

So if I understood your example, you just created a qcow2 file, where
the non-metadata contents of that file are themselves a qcow2 data format.

Oh my.  This could get nasty if someone ever wanted to try to get
libvirt to feed the nested contents to a guest (right now, libvirt has a
hard-coded assumption that qcow2 files only ever wrap raw data in the
non-metadata portion; there's no easy way to represent in the libvirt
storage volume XML a file where you want the nested guest contents of
qcow2 data wrapped inside the non-metadata portion of an outermost qcow2
file).

I guess it's not much different from a raw block device that has a
partition table, then in the first partition, you recursively add
another partition table (which is what you get when you hand one
partition of a host block device to the guest, and the guest then
partitions what it thinks is a full disk).  And libguestfs is able to
see through nested partition tables, so on that front, the functionality
is similar.

Of course, the recursion has its use for blkverify and blkdebug, but
maybe (for our sanity) it would be worth preventing nesting across any
format that has metadata wrapping contents.

> 
> Worth a qemu-iotests case, I think.

That's for sure :)

-- 
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 v5 15/22] blkdebug: Allow command-line file configuration

[Kevin Wolf]

Am 13.12.2013 um 18:10 hat Max Reitz geschrieben:
> Introduce the "image" option as an alternative to specifying the image
> through the filename.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>

One thing I'd like to see, either in the next version or in a follow-up
series, is to generalise open_image() into a block.c function.

The main difference between blkdebug and blkverify seems to be that
blkverify does image format probing on its main image. This can be a
bool argument to open_image().

Reviewed-by: Kevin Wolf <kwolf@redhat.com>

Re: [Qemu-devel] [PATCH v5 16/22] blkdebug: Make filename optional

[Kevin Wolf]

Am 13.12.2013 um 18:10 hat Max Reitz geschrieben:
> 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 0c800ae..fdfc6b0 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) {

filename can't be NULL here: For a non-NULL input, strstart() doesn't
return NULL, and for a NULL string it segfaults...

I don't think we have the segfault case here, because calling
bdrv_parse_filename() without having a filename in the first place is
quite pointless. I suspect giving no filename has been working even
before this patch.

> +            qdict_put(options, "x-image", qstring_from_str(filename));
> +        }
>          return;
>      }

Having a test case in the commit would show what your intention was (and
that the implementation doesn't do that).

Kevin

Re: [Qemu-devel] [PATCH v5 17/22] blkverify: Allow command-line configuration

[Kevin Wolf]

Am 13.12.2013 um 18:10 hat Max Reitz geschrieben:
> 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);

This changes the behaviour: We now do format probing, and probably
insert a raw block driver between blkverify and the protocol driver.

If you had a generalised open_image() that is shared with blkdebug, you
could specify no probing here, and enable probing for the main image.

Kevin

Re: [Qemu-devel] [PATCH v5 18/22] blkverify: Don't require protocol filename

[Kevin Wolf]

Am 13.12.2013 um 18:10 hat Max Reitz geschrieben:
> 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>

Same comment as for blkdebug.

Kevin

Re: [Qemu-devel] [PATCH v5 19/22] blkdebug: Alias "errno" as "error"

[Kevin Wolf]

Am 13.12.2013 um 18:10 hat Max Reitz geschrieben:
> 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>

I can't say I like this, but it seems we don't have an option.

Reviewed-by: Kevin Wolf <kwolf@redhat.com>

Re: [Qemu-devel] [PATCH v5 20/22] qapi: QMP interface for blkdebug and blkverify

[Kevin Wolf]

Am 13.12.2013 um 18:10 hat Max Reitz geschrieben:
> 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 c3c939c..6ce016c 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',

I bet Eric will tell you that an enum for event would be much nicer. ;-)

Reviewed-by: Kevin Wolf <kwolf@redhat.com>

Re: [Qemu-devel] [PATCH v5 21/22] qemu-io: Make filename optional

[Kevin Wolf]

Am 13.12.2013 um 18:10 hat Max Reitz geschrieben:
> 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>

Re: [Qemu-devel] [PATCH v5 19/22] blkdebug: Alias "errno" as "error"

[Eric Blake]

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

On 12/13/2013 01:49 PM, Kevin Wolf wrote:
> Am 13.12.2013 um 18:10 hat Max Reitz geschrieben:
>> 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>
> 
> I can't say I like this, but it seems we don't have an option.

Don't we already have a way to use QMP keys that collide with C keywords
(for example, "unix" in qapi's SocketAddress becomes "q_unix" in C)?
Why can't we reuse that mechanism, and just add "errno" to the polluted
words list so that the generated C code uses "q_errno" rather than
having to invent an "error" alias?

-- 
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 v5 20/22] qapi: QMP interface for blkdebug and blkverify

[Eric Blake]

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

On 12/13/2013 01:54 PM, Kevin Wolf wrote:
> Am 13.12.2013 um 18:10 hat Max Reitz geschrieben:

>> +# @BlkdebugInjectErrorOptions
>> +#
>> +# Describes a single error injection for blkdebug.
>> +#
>> +# @event:       trigger event name
>> +#

>> +##
>> +{ 'type': 'BlkdebugInjectErrorOptions',
>> +  'data': { 'event': 'str',
> 
> I bet Eric will tell you that an enum for event would be much nicer. ;-)

Am I that predictable?  :)

And see my comments on 19; I think with a minor tweak to qapi.py's
polluted_words, that you could use @errno instead of @error.

-- 
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 v5 22/22] iotests: Test new blkdebug/blkverify interface

[Kevin Wolf]

Am 13.12.2013 um 18:10 hat Max Reitz geschrieben:
> 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     | 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

Quite small. :-)

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

How about doing a successful write/read pair as well?

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

The other way round would be worth an additional test (i.e. using an
existing block device for the raw reference).

Anyway, this is just suggestions for improvement, so:

Reviewed-by: Kevin Wolf <kwolf@redhat.com>

Re: [Qemu-devel] [PATCH v5 12/22] block: Allow recursive "file"s

[Kevin Wolf]

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

Am 13.12.2013 um 21:36 hat Eric Blake geschrieben:
> On 12/13/2013 01:19 PM, Kevin Wolf wrote:
> > Am 13.12.2013 um 18:10 hat Max Reitz geschrieben:
> >> 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>
> > 
> > Hm, does this do what I think it does?
> 
> That depends on what you think it's doing :)
> 
> > 
> > $ ./qemu-img convert -O qcow2 /home/kwolf/images/hd.img /tmp/hd.qcow2
> > $ ./qemu-img convert -f raw -O qcow2 /tmp/hd.qcow2 /tmp/hd.qcow2.qcow2
> > $ x86_64-softmmu/qemu-system-x86_64 -drive driver=qcow2,file.driver=qcow2,file.file.driver=file,file.file.filename=/tmp/hd.qcow2.qcow2
> > 
> > I can't decide whether this is awesomeness or insanity, but in any case
> > it works with this patch. :-)
> 
> So if I understood your example, you just created a qcow2 file, where
> the non-metadata contents of that file are themselves a qcow2 data format.
> 
> Oh my.

Hehe, that was exactly my first thought as well.

But if we're serious about the direction that we're taking with
blockdev-add - that is, letting the user build his graph of block
drivers - then this is something we have to allow. It's just a proof
that we do provide the flexibility we had in mind.

> This could get nasty if someone ever wanted to try to get
> libvirt to feed the nested contents to a guest (right now, libvirt has a
> hard-coded assumption that qcow2 files only ever wrap raw data in the
> non-metadata portion; there's no easy way to represent in the libvirt
> storage volume XML a file where you want the nested guest contents of
> qcow2 data wrapped inside the non-metadata portion of an outermost qcow2
> file).

I don't think there's a good use case for nested qcow2, so I wouldn't
expect libvirt to implement this. Until the first user comes up with a
use case, of course. And like with everything scary that we assume
nobody will ever do, someone will turn up eventually...

> I guess it's not much different from a raw block device that has a
> partition table, then in the first partition, you recursively add
> another partition table (which is what you get when you hand one
> partition of a host block device to the guest, and the guest then
> partitions what it thinks is a full disk).  And libguestfs is able to
> see through nested partition tables, so on that front, the functionality
> is similar.
> 
> Of course, the recursion has its use for blkverify and blkdebug, but
> maybe (for our sanity) it would be worth preventing nesting across any
> format that has metadata wrapping contents.

I don't think qemu should prevent it. The architecture we want to have
in the end doesn't really distinguish between formats and filters, there
are only drivers. And technically there is nothing that allowing this
breaks.

libvirt, on the other hand, can probably expose something simpler. But
as we discussed on KVM Forum, it's not unlikely that you'll have to have
your own blockdev project sooner or later because users will want to
have some of the flexibility - and then maybe nesting qcow2 falls out as
naturally as it does in qemu now.

Kevin

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

Re: [Qemu-devel] [PATCH v5 19/22] blkdebug: Alias "errno" as "error"

[Kevin Wolf]

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

Am 13.12.2013 um 22:00 hat Eric Blake geschrieben:
> On 12/13/2013 01:49 PM, Kevin Wolf wrote:
> > Am 13.12.2013 um 18:10 hat Max Reitz geschrieben:
> >> 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>
> > 
> > I can't say I like this, but it seems we don't have an option.
> 
> Don't we already have a way to use QMP keys that collide with C keywords
> (for example, "unix" in qapi's SocketAddress becomes "q_unix" in C)?
> Why can't we reuse that mechanism, and just add "errno" to the polluted
> words list so that the generated C code uses "q_errno" rather than
> having to invent an "error" alias?

Oh, I didn't know that this exists. Sounds better then, because it makes
the external interfaces uniform, even though internally we'd still have
aliases (errno/q_errno instead of errno/error then).

Kevin

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

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

[Max Reitz]

On 13.12.2013 21:36, Kevin Wolf wrote:
> Am 13.12.2013 um 18:10 hat Max Reitz geschrieben:
>> Introduce the "image" option as an alternative to specifying the image
>> through the filename.
>>
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
> One thing I'd like to see, either in the next version or in a follow-up
> series, is to generalise open_image() into a block.c function.

Yes, I thought of something like that myself. I think, eventually we 
need to merge bdrv_open() and bdrv_file_open() (now that "file"s can be 
nested and there may be block devices without "file"s, there is no 
actual need to separate both).

> The main difference between blkdebug and blkverify seems to be that
> blkverify does image format probing on its main image. This can be a
> bool argument to open_image().

I think, blkdebug should just force the raw driver with a common 
open_image().

Max

Re: [Qemu-devel] [PATCH v5 19/22] blkdebug: Alias "errno" as "error"

[Max Reitz]

On 13.12.2013 22:00, Eric Blake wrote:
> On 12/13/2013 01:49 PM, Kevin Wolf wrote:
>> Am 13.12.2013 um 18:10 hat Max Reitz geschrieben:
>>> 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>
>> I can't say I like this, but it seems we don't have an option.
> Don't we already have a way to use QMP keys that collide with C keywords
> (for example, "unix" in qapi's SocketAddress becomes "q_unix" in C)?
> Why can't we reuse that mechanism, and just add "errno" to the polluted
> words list so that the generated C code uses "q_errno" rather than
> having to invent an "error" alias?

Great, I'll have a look.

Max

Re: [Qemu-devel] [PATCH v5 22/22] iotests: Test new blkdebug/blkverify interface

[Max Reitz]

On 13.12.2013 22:08, Kevin Wolf wrote:
> Am 13.12.2013 um 18:10 hat Max Reitz geschrieben:
>> 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     | 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
> Quite small. :-)

Hm, yes, especially for writes to 0x38000 = 224k *g*.

>> +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
> How about doing a successful write/read pair as well?

Okay, why not.

>> +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"
>> +                }
>> +            }
>> +        }
>> +    }
>> +}
> The other way round would be worth an additional test (i.e. using an
> existing block device for the raw reference).

Okay.

Max

Re: [Qemu-devel] [PATCH v5 05/22] qdict: Remove delete from qdict_flatten_qdict()

[Max Reitz]

On 13.12.2013 19:58, Kevin Wolf wrote:
> Am 13.12.2013 um 18:10 hat Max Reitz geschrieben:
>> delete is always set to true, therefore it can be removed.
>>
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
> Nope, this can't be right.
>
> delete is always set, except for simple types in the top-level QDict.
> They get deleted now instead of being left alone.

Ah, I don't know how I missed the "if (prefix)"... Well then, I feared 
requiring delete would somehow require qdict_flatten_qlist() to have 
something similar, but if this can be wrong for top-level elements only, 
at least that won't be the case.

Max

Re: [Qemu-devel] [PATCH v5 11/22] block: Allow block devices without files

[Max Reitz]

On 13.12.2013 21:06, Kevin Wolf wrote:
> Am 13.12.2013 um 18:10 hat Max Reitz geschrieben:
>> 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 bef4f82..9659eb5 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 */
>> @@ -1178,7 +1181,7 @@ int bdrv_open(BlockDriverState *bs, const char *filename, QDict *options,
> Manually adding more context before the next hunk:
>
>>      if (!drv) {
>>          ret = find_image_format(file, filename, &drv, &local_err);
>>      }
> We can get file == NULL now, which will cause a segfault in
> find_image_format. (For a reproducer of the segfault, try
> 'x86_64-softmmu/qemu-system-x86_64 -drive foo=bar')

Right, thanks.

Max

Re: [Qemu-devel] [PATCH v5 12/22] block: Allow recursive "file"s

[Max Reitz]

On 13.12.2013 21:19, Kevin Wolf wrote:
> Am 13.12.2013 um 18:10 hat Max Reitz geschrieben:
>> 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>
> Hm, does this do what I think it does?
>
> $ ./qemu-img convert -O qcow2 /home/kwolf/images/hd.img /tmp/hd.qcow2
> $ ./qemu-img convert -f raw -O qcow2 /tmp/hd.qcow2 /tmp/hd.qcow2.qcow2
> $ x86_64-softmmu/qemu-system-x86_64 -drive driver=qcow2,file.driver=qcow2,file.file.driver=file,file.file.filename=/tmp/hd.qcow2.qcow2
>
> I can't decide whether this is awesomeness or insanity, but in any case
> it works with this patch. :-)
>
> Worth a qemu-iotests case, I think.
>
>>   block.c | 17 ++++++++++++-----
>>   1 file changed, 12 insertions(+), 5 deletions(-)
>>
>> diff --git a/block.c b/block.c
>> index 9659eb5..9222669 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);
>> @@ -970,10 +975,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;
>>   }
> Not sure why this hunk is needed, but anyway:

Ah, I think I know. It is there because of patch 9. As stated in the 
cover letter, additionally to the double QDECREF on options, I noticed a 
possible NULL dereference of bs on error in the "if (reference)" path 
and fixed it in this version. In v3, it was addressed by this hunk, 
which somehow got into patch 12 rather than 9. I'll remove it, now that 
it's unnecessary.

Max

Re: [Qemu-devel] [PATCH v5 20/22] qapi: QMP interface for blkdebug and blkverify

[Max Reitz]

On 13.12.2013 21:54, Kevin Wolf wrote:
> Am 13.12.2013 um 18:10 hat Max Reitz geschrieben:
>> 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 c3c939c..6ce016c 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',
> I bet Eric will tell you that an enum for event would be much nicer. ;-)

 From my perspective, it isn't that nice, but I see where you are coming 
from, so I'll do it. ;-)

Max

Re: [Qemu-devel] [PATCH v5 17/22] blkverify: Allow command-line configuration

[Max Reitz]

On 13.12.2013 21:46, Kevin Wolf wrote:
> Am 13.12.2013 um 18:10 hat Max Reitz geschrieben:
>> 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);
> This changes the behaviour: We now do format probing, and probably
> insert a raw block driver between blkverify and the protocol driver.
>
> If you had a generalised open_image() that is shared with blkdebug, you
> could specify no probing here, and enable probing for the main image.

My main problem with a shared open_image() was finding a nice global 
name. ;-)

I think I'll add a flag for now. With that flag, we could easily make it 
global, but finding a nice name still remains a problem.

Max

Re: [Qemu-devel] [PATCH v5 16/22] blkdebug: Make filename optional

[Max Reitz]

On 13.12.2013 21:43, Kevin Wolf wrote:
> Am 13.12.2013 um 18:10 hat Max Reitz geschrieben:
>> 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 0c800ae..fdfc6b0 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) {
> filename can't be NULL here: For a non-NULL input, strstart() doesn't
> return NULL, and for a NULL string it segfaults...
>
> I don't think we have the segfault case here, because calling
> bdrv_parse_filename() without having a filename in the first place is
> quite pointless. I suspect giving no filename has been working even
> before this patch.

Right; for NULL input, that function isn't even called ("if 
(drv->bdrv_parse_filename && filename)" in bdrv_file_open()).

Max

>> +            qdict_put(options, "x-image", qstring_from_str(filename));
>> +        }
>>           return;
>>       }
> Having a test case in the commit would show what your intention was (and
> that the implementation doesn't do that).
>
> Kevin