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

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

[Max Reitz]

This is technically a v2 to
"block: Fix filename generation for blkdebug and nbd" which contained a
fix for the NBD filename generation, which has been merged into master
by now. The rest, however, has not been yet, due to lack of reviews.

Instead of pinging an old series with an already merged patch I'm
therefore just resending the remaining two patches with this nice
inciting cover letter which really, really makes you want to review the
patches because they are so lovely. I don't want to say good, because I
know my patches, but you want to review them either way because you
can't stand the excitement to know what's in them.

There is only one way of ever finding out: Review them! It'll bring
great joy and maybe even glory!


Also, if you don't review them and I have to send a v3, I'll attach a
picture of a really cute kitten and tell you that this kitten will cry
really badly if you don't adopt these patches. After another month, I
will send a picture of a crying kitten.

So you better review this version.


git-backport-diff against 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:[----] [--] 'blkdebug: Simplify and improve filename generation'
002/2:[----] [--] 'iotests: Plain blkdebug filename generation'


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

 block/blkdebug.c           | 98 +++++++++++++---------------------------------
 tests/qemu-iotests/099     |  9 ++++-
 tests/qemu-iotests/099.out |  8 +++-
 3 files changed, 41 insertions(+), 74 deletions(-)

-- 
1.9.3

[Qemu-devel] [PATCH v2 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           | 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 862d93b..10d4f8d 100644
--- a/block/blkdebug.c
+++ b/block/blkdebug.c
@@ -721,93 +721,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
-- 
1.9.3

[Qemu-devel] [PATCH v2 2/2] 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
-- 
1.9.3

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

[Max Reitz]

On 2014-11-10 at 17:59, Max Reitz wrote:
> This is technically a v2 to
> "block: Fix filename generation for blkdebug and nbd" which contained a
> fix for the NBD filename generation, which has been merged into master
> by now. The rest, however, has not been yet, due to lack of reviews.
>
> Instead of pinging an old series with an already merged patch I'm
> therefore just resending the remaining two patches with this nice
> inciting cover letter which really, really makes you want to review the
> patches because they are so lovely. I don't want to say good, because I
> know my patches, but you want to review them either way because you
> can't stand the excitement to know what's in them.
>
> There is only one way of ever finding out: Review them! It'll bring
> great joy and maybe even glory!
>
>
> Also, if you don't review them and I have to send a v3, I'll attach a
> picture of a really cute kitten and tell you that this kitten will cry
> really badly if you don't adopt these patches. After another month, I
> will send a picture of a crying kitten.
>
> So you better review this version.
>
>
> git-backport-diff against 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:[----] [--] 'blkdebug: Simplify and improve filename generation'
> 002/2:[----] [--] 'iotests: Plain blkdebug filename generation'
>
>
> Max Reitz (2):
>    blkdebug: Simplify and improve filename generation
>    iotests: Plain blkdebug filename generation
>
>   block/blkdebug.c           | 98 +++++++++++++---------------------------------
>   tests/qemu-iotests/099     |  9 ++++-
>   tests/qemu-iotests/099.out |  8 +++-
>   3 files changed, 41 insertions(+), 74 deletions(-)

Oops, forgot to CC Peter. I bet he can't resist yet non-existing cute 
kittens.

Max

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

[Eric Blake]

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

On 11/10/2014 09:59 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           | 98 +++++++++++++---------------------------------
>  tests/qemu-iotests/099.out |  4 +-
>  2 files changed, 29 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"),

If there is no "config" entry, this can pass NULL as a %s counterpart to
snprintf, which is undefined behavior (works on glibc, but might crash
elsewhere).

Everything else looked okay, but I'm afraid I may have given you reason
to post a v3 and make a kitten cry :(

-- 
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 v2 2/2] iotests: Plain blkdebug filename generation

[Eric Blake]

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

On 11/10/2014 09:59 AM, Max Reitz wrote:
> 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(-)

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 v2 1/2] blkdebug: Simplify and improve filename generation

[Max Reitz]

On 2014-11-10 at 20:15, Eric Blake wrote:
> On 11/10/2014 09:59 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           | 98 +++++++++++++---------------------------------
>>   tests/qemu-iotests/099.out |  4 +-
>>   2 files changed, 29 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"),
> If there is no "config" entry, this can pass NULL as a %s counterpart to
> snprintf, which is undefined behavior (works on glibc, but might crash
> elsewhere).

Well, depends on the definition of "works". "blkdebug:(null):foo" isn't 
that great either.

> Everything else looked okay, but I'm afraid I may have given you reason
> to post a v3 and make a kitten cry :(

I do have a reason to post a v3, but the kitten would have to cry only 
if I had to post a v3 because there were no reviews. This way, 
everything is fine, thank you very much for your reviews (on the other 
series as well). :-)

Max