[Qemu-devel] [PATCH v3 0/2] block: Improve filename generation for blkdebug

[Qemu-devel] [PATCH v3 0/2] block: Improve filename generation for blkdebug

[Max Reitz]

Eric has gloriously reviewed v2 of this series, and by adopting v2
averted a kitten crisis and the need for me to find a freely usable
kitten picture (although I guess Wikipedia would have been the way to
go). I do have own pictures, but they are currently about 400 km air
distance away from me.


The changes in v3:
- Patch 1: Added a check for the "config" option being NULL (which
  happens if no config file is given, either by just not giving the
  option or by using the blkdebug::foo syntax); in this case, omit the
  config filename from the output (to receive "blkdebug::foo") [Eric]
- Patch 2: Add a test for this case


git-backport-diff output against v2 (and v1):

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/2:[0003] [FC] 'blkdebug: Simplify and improve filename generation'
002/2:[0010] [FC] 'iotests: Plain blkdebug filename generation'


Max Reitz (2):
  blkdebug: Simplify and improve filename generation
  iotests: Plain blkdebug filename generation

 block/blkdebug.c           | 99 +++++++++++++---------------------------------
 tests/qemu-iotests/099     | 15 ++++++-
 tests/qemu-iotests/099.out | 12 +++++-
 3 files changed, 52 insertions(+), 74 deletions(-)

-- 
1.9.3

[Qemu-devel] [PATCH v3 1/2] 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           | 99 +++++++++++++---------------------------------
 tests/qemu-iotests/099.out |  4 +-
 2 files changed, 30 insertions(+), 73 deletions(-)

diff --git a/block/blkdebug.c b/block/blkdebug.c
index 862d93b..9ce35cd 100644
--- a/block/blkdebug.c
+++ b/block/blkdebug.c
@@ -721,93 +721,50 @@ 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
-- 
1.9.3

Re: [Qemu-devel] [PATCH v3 1/2] blkdebug: Simplify and improve filename generation

[Eric Blake]

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

On 11/11/2014 02:23 AM, Max Reitz wrote:
> 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           | 99 +++++++++++++---------------------------------
>  tests/qemu-iotests/099.out |  4 +-
>  2 files changed, 30 insertions(+), 73 deletions(-)
> 

> +    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") ?: "",

This is one case where I _like_ the fact that we allow the gcc extension
operator.  It's so much more verbose to store the function call result
into a temporary to avoid calling it twice.

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

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


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

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

[Max Reitz]

Add one test whether blkdebug is able to generate a plain filename if
given a configuration file and a file to be tested only; and add another
test whether blkdebug is able to do the same without being given a
configuration file.

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

diff --git a/tests/qemu-iotests/099 b/tests/qemu-iotests/099
index ffc7ea7..132aa0b 100755
--- a/tests/qemu-iotests/099
+++ b/tests/qemu-iotests/099
@@ -107,8 +107,21 @@ 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"
+
+echo
+echo '=== Testing plain filename for blkdebug without configuration file ==='
+echo
+
+test_qemu "file.driver=blkdebug,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..2fa06ff 100644
--- a/tests/qemu-iotests/099.out
+++ b/tests/qemu-iotests/099.out
@@ -17,4 +17,12 @@ 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
+
+=== Testing plain filename for blkdebug without configuration file ===
+
+blkdebug::TEST_DIR/t.IMGFMT
 *** done
-- 
1.9.3

Re: [Qemu-devel] [PATCH v3 2/2] iotests: Plain blkdebug filename generation

[Eric Blake]

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

On 11/11/2014 02:23 AM, Max Reitz wrote:
> Add one test whether blkdebug is able to generate a plain filename if
> given a configuration file and a file to be tested only; and add another
> test whether blkdebug is able to do the same without being given a
> configuration file.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  tests/qemu-iotests/099     | 15 ++++++++++++++-
>  tests/qemu-iotests/099.out |  8 ++++++++
>  2 files changed, 22 insertions(+), 1 deletion(-)
> 
> diff --git a/tests/qemu-iotests/099 b/tests/qemu-iotests/099

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

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


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

Re: [Qemu-devel] [PATCH v3 0/2] block: Improve filename generation for blkdebug

[Stefan Hajnoczi]

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

On Tue, Nov 11, 2014 at 10:23:43AM +0100, Max Reitz wrote:
> Eric has gloriously reviewed v2 of this series, and by adopting v2
> averted a kitten crisis and the need for me to find a freely usable
> kitten picture (although I guess Wikipedia would have been the way to
> go). I do have own pictures, but they are currently about 400 km air
> distance away from me.
> 
> 
> The changes in v3:
> - Patch 1: Added a check for the "config" option being NULL (which
>   happens if no config file is given, either by just not giving the
>   option or by using the blkdebug::foo syntax); in this case, omit the
>   config filename from the output (to receive "blkdebug::foo") [Eric]
> - Patch 2: Add a test for this case
> 
> 
> git-backport-diff output against v2 (and v1):
> 
> 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/2:[0003] [FC] 'blkdebug: Simplify and improve filename generation'
> 002/2:[0010] [FC] 'iotests: Plain blkdebug filename generation'
> 
> 
> Max Reitz (2):
>   blkdebug: Simplify and improve filename generation
>   iotests: Plain blkdebug filename generation
> 
>  block/blkdebug.c           | 99 +++++++++++++---------------------------------
>  tests/qemu-iotests/099     | 15 ++++++-
>  tests/qemu-iotests/099.out | 12 +++++-
>  3 files changed, 52 insertions(+), 74 deletions(-)
> 
> -- 
> 1.9.3
> 
> 

Thanks, applied to my block-next tree:
https://github.com/stefanha/qemu/commits/block-next

Stefan

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