[Qemu-devel] [PATCH v2 0/7] blkdebug/blkverify: Allow command-line configuration

[Qemu-devel] [PATCH v2 0/7] blkdebug/blkverify: Allow command-line 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
command-line options. The driver has to be selected through the
file.driver option (or similar), the image filename has to be given as
the filename (obviously) and, depending on the driver, further options
have to be given to control the behavior.

In case of blkverify, the x-raw option specifies the name of the raw
reference image file.

In case of blkdebug, one may either set the config option to the
filename of a configuration file, or the contents of the configuration
file may be given directly on the command line (see description of patch
4 for an example).

v2:
 - Addressed Fam's comments:
    - Added patch 3
    - Fixed patch 4 and modified it to use the function introduced in
      patch 3
 - Removed unnecessary error_propagate() in patch 1 (affects patches 5
   and 6 as well)


Max Reitz (7):
  blkdebug: Use errp for read_config()
  blkdebug: Don't require sophisticated filename
  qdict: Add qdict_array_split()
  qemu-option: Add qemu_config_parse_qdict()
  blkdebug: Always call read_config()
  blkdebug: Use command-line in read_config()
  blkverify: Don't require protocol filename

 block/blkdebug.c           | 49 ++++++++++++++++--------
 block/blkverify.c          |  4 +-
 include/qapi/qmp/qdict.h   |  1 +
 include/qemu/config-file.h |  6 +++
 qobject/qdict.c            | 36 ++++++++++++++++++
 util/qemu-config.c         | 95 ++++++++++++++++++++++++++++++++++++++++++++++
 6 files changed, 174 insertions(+), 17 deletions(-)

-- 
1.8.4.2

[Qemu-devel] [PATCH v2 1/7] 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 | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/block/blkdebug.c b/block/blkdebug.c
index 16d2b91..9dfa712 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,16 @@ 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 "
+                         "'%s'", filename);
         return -errno;
     }
 
     ret = qemu_config_parse(f, config_groups, filename);
     if (ret < 0) {
+        error_setg(errp, "Could not parse blkdebug config file '%s'",
+                   filename);
+        ret = -EINVAL;
         goto fail;
     }
 
@@ -370,9 +375,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.4.2

Re: [Qemu-devel] [PATCH v2 1/7] blkdebug: Use errp for read_config()

[Kevin Wolf]

Am 22.11.2013 um 17:10 hat Max Reitz geschrieben:
> Use an Error variable in the read_config() function.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  block/blkdebug.c | 12 ++++++++----
>  1 file changed, 8 insertions(+), 4 deletions(-)
> 
> diff --git a/block/blkdebug.c b/block/blkdebug.c
> index 16d2b91..9dfa712 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)

For a complete conversion, this should become a void function.

>  {
>      FILE *f;
>      int ret;
> @@ -279,11 +279,16 @@ 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 "
> +                         "'%s'", filename);
>          return -errno;
>      }
>  
>      ret = qemu_config_parse(f, config_groups, filename);
>      if (ret < 0) {
> +        error_setg(errp, "Could not parse blkdebug config file '%s'",
> +                   filename);
> +        ret = -EINVAL;
>          goto fail;
>      }

Any reason for adding the file name here without mentioning it in the
commit message?

I'm not completely sure here, but the information is probably redundant;
the caller already knows what the config file was. On the command line,
this will lead to error messages such as:

qemu: -drive file=blkdebug:/path/to/blkdebug.cfg:foo.qcow2: Could not
read blkdebug config file '/path/to/blkdebug.cfg': No such file or
directory

> @@ -370,9 +375,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;
>          }
>      }

Hm, I see. You actually need to return -errno for bdrv_open(). Then
let's just check if we really want to add the file name in the message.

Kevin

Re: [Qemu-devel] [PATCH v2 1/7] blkdebug: Use errp for read_config()

[Max Reitz]

On 25.11.2013 14:40, Kevin Wolf wrote:
> Am 22.11.2013 um 17:10 hat Max Reitz geschrieben:
>> Use an Error variable in the read_config() function.
>>
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>> ---
>>   block/blkdebug.c | 12 ++++++++----
>>   1 file changed, 8 insertions(+), 4 deletions(-)
>>
>> diff --git a/block/blkdebug.c b/block/blkdebug.c
>> index 16d2b91..9dfa712 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)
> For a complete conversion, this should become a void function.

I wanted to do this at first, but blkdebug_open needs to return -errno 
again; in the hunk below, -errno from fopen is returned. We could just 
return some dummy value in blkdebug_open again, but I thought it better 
to pass the correct value.

>>   {
>>       FILE *f;
>>       int ret;
>> @@ -279,11 +279,16 @@ 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 "
>> +                         "'%s'", filename);
>>           return -errno;
>>       }
>>   
>>       ret = qemu_config_parse(f, config_groups, filename);
>>       if (ret < 0) {
>> +        error_setg(errp, "Could not parse blkdebug config file '%s'",
>> +                   filename);
>> +        ret = -EINVAL;
>>           goto fail;
>>       }
> Any reason for adding the file name here without mentioning it in the
> commit message?

No, not really. I don't know why I added this, the original 
error_setg_errno() in blkdebug_open didn't have it, so…

> I'm not completely sure here, but the information is probably redundant;
> the caller already knows what the config file was. On the command line,
> this will lead to error messages such as:
>
> qemu: -drive file=blkdebug:/path/to/blkdebug.cfg:foo.qcow2: Could not
> read blkdebug config file '/path/to/blkdebug.cfg': No such file or
> directory

Personally, I like such redundant information, but I guess I still need 
to learn that qemu sometimes differs from my preferences. ;-)

>> @@ -370,9 +375,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;
>>           }
>>       }
> Hm, I see. You actually need to return -errno for bdrv_open(). Then
> let's just check if we really want to add the file name in the message.

If you don't see the point, I do not either. The original 
error_setg_errno() didn't have it, so we should leave it that way.

Max

[Qemu-devel] [PATCH v2 2/7] 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 9dfa712..b50db9d 100644
--- a/block/blkdebug.c
+++ b/block/blkdebug.c
@@ -315,7 +315,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.4.2

[Qemu-devel] [PATCH v2 3/7] 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 0f3e0a6..ef76281 100644
--- a/qobject/qdict.c
+++ b/qobject/qdict.c
@@ -548,3 +548,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.4.2

[Qemu-devel] [PATCH v2 4/7] 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.4.2

[Qemu-devel] [PATCH v2 5/7] 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 | 40 +++++++++++++++++++++-------------------
 1 file changed, 21 insertions(+), 19 deletions(-)

diff --git a/block/blkdebug.c b/block/blkdebug.c
index b50db9d..1db999b 100644
--- a/block/blkdebug.c
+++ b/block/blkdebug.c
@@ -273,23 +273,25 @@ 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 "
-                         "'%s'", filename);
-        return -errno;
-    }
+    if (filename) {
+        f = fopen(filename, "r");
+        if (f == NULL) {
+            error_setg_errno(errp, errno, "Could not read blkdebug config file "
+                             "'%s'", filename);
+            return -errno;
+        }
 
-    ret = qemu_config_parse(f, config_groups, filename);
-    if (ret < 0) {
-        error_setg(errp, "Could not parse blkdebug config file '%s'",
-                   filename);
-        ret = -EINVAL;
-        goto fail;
+        ret = qemu_config_parse(f, config_groups, filename);
+        if (ret < 0) {
+            error_setg(errp, "Could not parse blkdebug config file '%s'",
+                       filename);
+            ret = -EINVAL;
+            goto fail;
+        }
     }
 
     d.s = s;
@@ -303,7 +305,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;
 }
 
@@ -376,11 +380,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.4.2

[Qemu-devel] [PATCH v2 6/7] 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 1db999b..58bf05a 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");
@@ -294,6 +296,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);
@@ -378,9 +387,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.4.2

[Qemu-devel] [PATCH v2 7/7] 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 option
(like file.driver=blkverify). Contrary to the current reaction, this is
not really a problem; the whole filename just has to be stored (in the
x-image option) and the user has to manually specify the x-raw option.

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

diff --git a/block/blkverify.c b/block/blkverify.c
index 3c63528..bdbdd68 100644
--- a/block/blkverify.c
+++ b/block/blkverify.c
@@ -78,7 +78,9 @@ 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 (except for the filename) */
+        qdict_put(options, "x-image", qstring_from_str(filename));
         return;
     }
 
-- 
1.8.4.2

Re: [Qemu-devel] [PATCH v2 7/7] blkverify: Don't require protocol filename

[Kevin Wolf]

Am 22.11.2013 um 17: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 option
> (like file.driver=blkverify). Contrary to the current reaction, this is
> not really a problem; the whole filename just has to be stored (in the
> x-image option) and the user has to manually specify the x-raw option.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  block/blkverify.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/block/blkverify.c b/block/blkverify.c
> index 3c63528..bdbdd68 100644
> --- a/block/blkverify.c
> +++ b/block/blkverify.c
> @@ -78,7 +78,9 @@ 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 (except for the filename) */
> +        qdict_put(options, "x-image", qstring_from_str(filename));
>          return;
>      }

We don't want users to specify x-raw options, that's why it starts with
"x-" in the first place. So I'm not sure if this patch is a useful
intermediate step to make.

What we want to allow in the end is something like this:

    { "execute": "blockdev-add", "options": {
        { "driver": "blkverify",
          "image": {
              "driver": "qcow2",
              "file": ... },
          "raw:" {
              "driver": "raw",
              "file": ... } } }

Where "image" and "raw" are both of the BlockdevRef union type in QAPI,
i.e. there could also be a string that references an existing block
device.

We'll probably want a function that takes a BlockdevRef and returns a
BlockDriverState; either by bdrv_open() on a new one, or by bdrv_ref()
on an existing one.

Fam already has some code to achieve this in his BlockOp blockers
series, though not yet in a reusable way. I guess this series is the
good reason to actually request something reusable and then make use of
it here.

I guess you two just need to coordinate who's going to implement it (Fam
by default, I'd assume?)

Kevin

Re: [Qemu-devel] [PATCH v2 7/7] blkverify: Don't require protocol filename

[Max Reitz]

On 25.11.2013 15:40, Kevin Wolf wrote:
> Am 22.11.2013 um 17: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 option
>> (like file.driver=blkverify). Contrary to the current reaction, this is
>> not really a problem; the whole filename just has to be stored (in the
>> x-image option) and the user has to manually specify the x-raw option.
>>
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>> ---
>>   block/blkverify.c | 4 +++-
>>   1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/block/blkverify.c b/block/blkverify.c
>> index 3c63528..bdbdd68 100644
>> --- a/block/blkverify.c
>> +++ b/block/blkverify.c
>> @@ -78,7 +78,9 @@ 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 (except for the filename) */
>> +        qdict_put(options, "x-image", qstring_from_str(filename));
>>           return;
>>       }
> We don't want users to specify x-raw options, that's why it starts with
> "x-" in the first place. So I'm not sure if this patch is a useful
> intermediate step to make.

Well, it's the last patch of the series, so we could just leave it out 
for the time being. ;-)

> What we want to allow in the end is something like this:
>
>      { "execute": "blockdev-add", "options": {
>          { "driver": "blkverify",
>            "image": {
>                "driver": "qcow2",
>                "file": ... },
>            "raw:" {
>                "driver": "raw",
>                "file": ... } } }
>
> Where "image" and "raw" are both of the BlockdevRef union type in QAPI,
> i.e. there could also be a string that references an existing block
> device.
>
> We'll probably want a function that takes a BlockdevRef and returns a
> BlockDriverState; either by bdrv_open() on a new one, or by bdrv_ref()
> on an existing one.

Hm, yes, that seems far better.

> Fam already has some code to achieve this in his BlockOp blockers
> series, though not yet in a reusable way. I guess this series is the
> good reason to actually request something reusable and then make use of
> it here.
>
> I guess you two just need to coordinate who's going to implement it (Fam
> by default, I'd assume?)

Hm, are you referring to patch 5/7? I don't see right now how we could 
use that code for BlockdevRef… I'd agree we could probably use such code 
(using BlockdevRef) there instead, but not really the other way round. 
So, I think I could try to implement that function myself first.

Max