[Qemu-devel] [PATCH 0/3] block: Fix filename generation for blkdebug and nbd

[Qemu-devel] [PATCH 0/3] block: Fix filename generation for blkdebug and nbd

[Max Reitz]

This series fixes the filename generation in nbd and improves the one in
blkdebug.


Max Reitz (3):
  nbd: Fix filename generation
  blkdebug: Simplify and improve filename generation
  iotests: Plain blkdebug filename generation

 block/blkdebug.c           | 98 +++++++++++++---------------------------------
 block/nbd.c                | 44 ++++++++++++++-------
 tests/qemu-iotests/099     |  9 ++++-
 tests/qemu-iotests/099.out |  8 +++-
 4 files changed, 70 insertions(+), 89 deletions(-)

-- 
2.1.0

[Qemu-devel] [PATCH 1/3] nbd: Fix filename generation

[Max Reitz]

Export names may be used with nbd+unix, too, fix nbd_refresh_filename()
accordingly. Also, for nbd+tcp, the documented path schema is
"nbd://host[:port]/export", so use it. Furthermore, as can be seen from
that schema, the port is optional.

That makes six single cases for how the filename can be formatted; it is
not easy to generalize these cases without the resulting statement being
completely unreadable, thus there is simply one snprintf() per case.

Finally, taking the options from BDRVNBDState::socket_opts is wrong,
because those will not contain the export name. Just use
BlockDriverState::options instead.

Reported-by: Stefan Hajnoczi <stefanha@redhat.com>
Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 block/nbd.c | 44 +++++++++++++++++++++++++++++---------------
 1 file changed, 29 insertions(+), 15 deletions(-)

diff --git a/block/nbd.c b/block/nbd.c
index 89775e1..04cc845 100644
--- a/block/nbd.c
+++ b/block/nbd.c
@@ -342,30 +342,44 @@ static void nbd_attach_aio_context(BlockDriverState *bs,
 
 static void nbd_refresh_filename(BlockDriverState *bs)
 {
-    BDRVNBDState *s = bs->opaque;
     QDict *opts = qdict_new();
-    const char *path   = qemu_opt_get(s->socket_opts, "path");
-    const char *host   = qemu_opt_get(s->socket_opts, "host");
-    const char *port   = qemu_opt_get(s->socket_opts, "port");
-    const char *export = qemu_opt_get(s->socket_opts, "export");
+    const char *path   = qdict_get_try_str(bs->options, "path");
+    const char *host   = qdict_get_try_str(bs->options, "host");
+    const char *port   = qdict_get_try_str(bs->options, "port");
+    const char *export = qdict_get_try_str(bs->options, "export");
 
     qdict_put_obj(opts, "driver", QOBJECT(qstring_from_str("nbd")));
 
-    if (path) {
+    if (path && export) {
         snprintf(bs->exact_filename, sizeof(bs->exact_filename),
-                 "nbd+unix:%s", path);
-        qdict_put_obj(opts, "path", QOBJECT(qstring_from_str(path)));
-    } else if (export) {
+                 "nbd+unix:///%s?socket=%s", export, path);
+    } else if (path && !export) {
         snprintf(bs->exact_filename, sizeof(bs->exact_filename),
-                 "nbd:%s:%s/%s", host, port, export);
-        qdict_put_obj(opts, "host",   QOBJECT(qstring_from_str(host)));
-        qdict_put_obj(opts, "port",   QOBJECT(qstring_from_str(port)));
-        qdict_put_obj(opts, "export", QOBJECT(qstring_from_str(export)));
-    } else {
+                 "nbd+unix://?socket=%s", path);
+    } else if (!path && export && port) {
+        snprintf(bs->exact_filename, sizeof(bs->exact_filename),
+                 "nbd://%s:%s/%s", host, port, export);
+    } else if (!path && export && !port) {
+        snprintf(bs->exact_filename, sizeof(bs->exact_filename),
+                 "nbd://%s/%s", host, export);
+    } else if (!path && !export && port) {
+        snprintf(bs->exact_filename, sizeof(bs->exact_filename),
+                 "nbd://%s:%s", host, port);
+    } else if (!path && !export && !port) {
         snprintf(bs->exact_filename, sizeof(bs->exact_filename),
-                 "nbd:%s:%s", host, port);
+                 "nbd://%s", host);
+    }
+
+    if (path) {
+        qdict_put_obj(opts, "path", QOBJECT(qstring_from_str(path)));
+    } else if (port) {
         qdict_put_obj(opts, "host", QOBJECT(qstring_from_str(host)));
         qdict_put_obj(opts, "port", QOBJECT(qstring_from_str(port)));
+    } else {
+        qdict_put_obj(opts, "host", QOBJECT(qstring_from_str(host)));
+    }
+    if (export) {
+        qdict_put_obj(opts, "export", QOBJECT(qstring_from_str(export)));
     }
 
     bs->full_open_options = opts;
-- 
2.1.0

[Qemu-devel] [PATCH 2/3] blkdebug: Simplify and improve filename generation

[Max Reitz]

Instead of actually recreating the options from scratch, just reuse the
options given for creating the BDS, which are the configuration file
name and additional options. In case there are no additional options we
can thus create a plain filename.

This obviously results in a different output for qemu-iotest 099 which
exactly tests this filename generation. Fix it up as well.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 block/blkdebug.c           | 98 +++++++++++++---------------------------------
 tests/qemu-iotests/099.out |  4 +-
 2 files changed, 29 insertions(+), 73 deletions(-)

diff --git a/block/blkdebug.c b/block/blkdebug.c
index f8fbb0f..cbe7715 100644
--- a/block/blkdebug.c
+++ b/block/blkdebug.c
@@ -719,93 +719,49 @@ static int64_t blkdebug_getlength(BlockDriverState *bs)
 
 static void blkdebug_refresh_filename(BlockDriverState *bs)
 {
-    BDRVBlkdebugState *s = bs->opaque;
-    struct BlkdebugRule *rule;
     QDict *opts;
-    QList *inject_error_list = NULL, *set_state_list = NULL;
-    QList *suspend_list = NULL;
-    int event;
+    const QDictEntry *e;
+    bool force_json = false;
+
+    for (e = qdict_first(bs->options); e; e = qdict_next(bs->options, e)) {
+        if (strcmp(qdict_entry_key(e), "config") &&
+            strcmp(qdict_entry_key(e), "x-image") &&
+            strcmp(qdict_entry_key(e), "image") &&
+            strncmp(qdict_entry_key(e), "image.", strlen("image.")))
+        {
+            force_json = true;
+            break;
+        }
+    }
 
-    if (!bs->file->full_open_options) {
+    if (force_json && !bs->file->full_open_options) {
         /* The config file cannot be recreated, so creating a plain filename
          * is impossible */
         return;
     }
 
+    if (!force_json && bs->file->exact_filename[0]) {
+        snprintf(bs->exact_filename, sizeof(bs->exact_filename),
+                 "blkdebug:%s:%s", qdict_get_try_str(bs->options, "config"),
+                 bs->file->exact_filename);
+    }
+
     opts = qdict_new();
     qdict_put_obj(opts, "driver", QOBJECT(qstring_from_str("blkdebug")));
 
     QINCREF(bs->file->full_open_options);
     qdict_put_obj(opts, "image", QOBJECT(bs->file->full_open_options));
 
-    for (event = 0; event < BLKDBG_EVENT_MAX; event++) {
-        QLIST_FOREACH(rule, &s->rules[event], next) {
-            if (rule->action == ACTION_INJECT_ERROR) {
-                QDict *inject_error = qdict_new();
-
-                qdict_put_obj(inject_error, "event", QOBJECT(qstring_from_str(
-                              BlkdebugEvent_lookup[rule->event])));
-                qdict_put_obj(inject_error, "state",
-                              QOBJECT(qint_from_int(rule->state)));
-                qdict_put_obj(inject_error, "errno", QOBJECT(qint_from_int(
-                              rule->options.inject.error)));
-                qdict_put_obj(inject_error, "sector", QOBJECT(qint_from_int(
-                              rule->options.inject.sector)));
-                qdict_put_obj(inject_error, "once", QOBJECT(qbool_from_int(
-                              rule->options.inject.once)));
-                qdict_put_obj(inject_error, "immediately",
-                              QOBJECT(qbool_from_int(
-                              rule->options.inject.immediately)));
-
-                if (!inject_error_list) {
-                    inject_error_list = qlist_new();
-                }
-
-                qlist_append_obj(inject_error_list, QOBJECT(inject_error));
-            } else if (rule->action == ACTION_SET_STATE) {
-                QDict *set_state = qdict_new();
-
-                qdict_put_obj(set_state, "event", QOBJECT(qstring_from_str(
-                              BlkdebugEvent_lookup[rule->event])));
-                qdict_put_obj(set_state, "state",
-                              QOBJECT(qint_from_int(rule->state)));
-                qdict_put_obj(set_state, "new_state", QOBJECT(qint_from_int(
-                              rule->options.set_state.new_state)));
-
-                if (!set_state_list) {
-                    set_state_list = qlist_new();
-                }
-
-                qlist_append_obj(set_state_list, QOBJECT(set_state));
-            } else if (rule->action == ACTION_SUSPEND) {
-                QDict *suspend = qdict_new();
-
-                qdict_put_obj(suspend, "event", QOBJECT(qstring_from_str(
-                              BlkdebugEvent_lookup[rule->event])));
-                qdict_put_obj(suspend, "state",
-                              QOBJECT(qint_from_int(rule->state)));
-                qdict_put_obj(suspend, "tag", QOBJECT(qstring_from_str(
-                              rule->options.suspend.tag)));
-
-                if (!suspend_list) {
-                    suspend_list = qlist_new();
-                }
-
-                qlist_append_obj(suspend_list, QOBJECT(suspend));
-            }
+    for (e = qdict_first(bs->options); e; e = qdict_next(bs->options, e)) {
+        if (strcmp(qdict_entry_key(e), "x-image") &&
+            strcmp(qdict_entry_key(e), "image") &&
+            strncmp(qdict_entry_key(e), "image.", strlen("image.")))
+        {
+            qobject_incref(qdict_entry_value(e));
+            qdict_put_obj(opts, qdict_entry_key(e), qdict_entry_value(e));
         }
     }
 
-    if (inject_error_list) {
-        qdict_put_obj(opts, "inject-error", QOBJECT(inject_error_list));
-    }
-    if (set_state_list) {
-        qdict_put_obj(opts, "set-state", QOBJECT(set_state_list));
-    }
-    if (suspend_list) {
-        qdict_put_obj(opts, "suspend", QOBJECT(suspend_list));
-    }
-
     bs->full_open_options = opts;
 }
 
diff --git a/tests/qemu-iotests/099.out b/tests/qemu-iotests/099.out
index 55be4d4..50cc527 100644
--- a/tests/qemu-iotests/099.out
+++ b/tests/qemu-iotests/099.out
@@ -12,9 +12,9 @@ blkverify:TEST_DIR/t.IMGFMT.compare:TEST_DIR/t.IMGFMT
 
 === Testing JSON filename for blkdebug ===
 
-json:{"driver": "IMGFMT", "file": {"inject-error": [{"immediately": false, "once": false, "state": 0, "sector": -1, "event": "l1_update", "errno": 5}], "image": {"driver": "file", "filename": "TEST_DIR/t.IMGFMT"}, "driver": "blkdebug"}}
+json:{"driver": "IMGFMT", "file": {"image": {"driver": "file", "filename": "TEST_DIR/t.IMGFMT"}, "driver": "blkdebug", "inject-error.0.event": "l1_update"}}
 
 === Testing indirectly enforced JSON filename ===
 
-json:{"driver": "raw", "file": {"test": {"driver": "IMGFMT", "file": {"inject-error": [{"immediately": false, "once": false, "state": 0, "sector": -1, "event": "l1_update", "errno": 5}], "image": {"driver": "file", "filename": "TEST_DIR/t.IMGFMT"}, "driver": "blkdebug"}}, "driver": "blkverify", "raw": {"driver": "file", "filename": "TEST_DIR/t.IMGFMT.compare"}}}
+json:{"driver": "raw", "file": {"test": {"driver": "IMGFMT", "file": {"image": {"driver": "file", "filename": "TEST_DIR/t.IMGFMT"}, "driver": "blkdebug", "inject-error.0.event": "l1_update"}}, "driver": "blkverify", "raw": {"driver": "file", "filename": "TEST_DIR/t.IMGFMT.compare"}}}
 *** done
-- 
2.1.0

[Qemu-devel] [PATCH 3/3] iotests: Plain blkdebug filename generation

[Max Reitz]

Add a test whether blkdebug is able to generate a plain filename if
given a configuration file and a file to be tested only.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 tests/qemu-iotests/099     | 9 ++++++++-
 tests/qemu-iotests/099.out | 4 ++++
 2 files changed, 12 insertions(+), 1 deletion(-)

diff --git a/tests/qemu-iotests/099 b/tests/qemu-iotests/099
index ffc7ea7..74aa6d9 100755
--- a/tests/qemu-iotests/099
+++ b/tests/qemu-iotests/099
@@ -107,8 +107,15 @@ echo
 # generate a JSON object here as well
 test_qemu "file.driver=blkverify,file.raw.filename=$TEST_IMG.compare,file.test.file.driver=blkdebug,file.test.file.image.filename=$TEST_IMG,file.test.file.inject-error.0.event=l1_update"
 
+echo
+echo '=== Testing plain filename for blkdebug ==='
+echo
+
+touch "$TEST_DIR/blkdebug.conf"
+test_qemu "file.driver=blkdebug,file.config=$TEST_DIR/blkdebug.conf,file.image.filename=$TEST_IMG"
+
 
-rm -f "$TEST_IMG.compare"
+rm -f "$TEST_IMG.compare" "$TEST_DIR/blkdebug.conf"
 
 # success, all done
 echo "*** done"
diff --git a/tests/qemu-iotests/099.out b/tests/qemu-iotests/099.out
index 50cc527..3eebace 100644
--- a/tests/qemu-iotests/099.out
+++ b/tests/qemu-iotests/099.out
@@ -17,4 +17,8 @@ json:{"driver": "IMGFMT", "file": {"image": {"driver": "file", "filename": "TEST
 === Testing indirectly enforced JSON filename ===
 
 json:{"driver": "raw", "file": {"test": {"driver": "IMGFMT", "file": {"image": {"driver": "file", "filename": "TEST_DIR/t.IMGFMT"}, "driver": "blkdebug", "inject-error.0.event": "l1_update"}}, "driver": "blkverify", "raw": {"driver": "file", "filename": "TEST_DIR/t.IMGFMT.compare"}}}
+
+=== Testing plain filename for blkdebug ===
+
+blkdebug:TEST_DIR/blkdebug.conf:TEST_DIR/t.IMGFMT
 *** done
-- 
2.1.0

Re: [Qemu-devel] [PATCH 1/3] nbd: Fix filename generation

[Paolo Bonzini]

Il 24/09/2014 21:38, Max Reitz ha scritto:
> Export names may be used with nbd+unix, too, fix nbd_refresh_filename()
> accordingly. Also, for nbd+tcp, the documented path schema is
> "nbd://host[:port]/export", so use it. Furthermore, as can be seen from
> that schema, the port is optional.
> 
> That makes six single cases for how the filename can be formatted; it is
> not easy to generalize these cases without the resulting statement being
> completely unreadable, thus there is simply one snprintf() per case.

You could use GString, but I guess it won't be much better...

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>