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

[Qemu-devel] [PATCH 0/6] 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
3 for an example).


Max Reitz (6):
  blkdebug: Use errp for read_config()
  blkdebug: Don't require sophisticated filename
  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           |  50 +++++++++++++-------
 block/blkverify.c          |   4 +-
 include/qemu/config-file.h |   6 +++
 util/qemu-config.c         | 111 +++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 154 insertions(+), 17 deletions(-)

-- 
1.8.4.2

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

diff --git a/block/blkdebug.c b/block/blkdebug.c
index 16d2b91..76f80c0 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,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);
-        if (ret < 0) {
-            error_setg_errno(errp, -ret, "Could not read blkdebug config file");
+        ret = read_config(s, config, &local_err);
+        if (ret) {
+            error_propagate(errp, local_err);
             goto fail;
         }
     }
-- 
1.8.4.2

[Qemu-devel] [PATCH 2/6] 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 76f80c0..59d33a8 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 3/6] 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         | 111 +++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 117 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..80d82d4 100644
--- a/util/qemu-config.c
+++ b/util/qemu-config.c
@@ -356,3 +356,114 @@ int qemu_read_config_file(const char *filename)
         return -EINVAL;
     }
 }
+
+static void config_parse_qdict_section(QDict *options, QemuOptsList *opts,
+                                       Error **errp)
+{
+    QemuOpts *subopts, *subopts_i;
+    QDict *subqdict, *subqdict_i = NULL;
+    char *prefix = g_strdup_printf("%s.", opts->name);
+    Error *local_err = NULL;
+    size_t orig_size, enum_size;
+
+    qdict_extract_subqdict(options, &subqdict, 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 rules */
+        int i;
+
+        /* Not required anymore */
+        qemu_opts_del(subopts);
+
+        for (i = 0; i < INT_MAX; i++) {
+            char i_prefix[32], opt_name[48];
+            size_t snprintf_ret;
+
+            snprintf_ret = snprintf(i_prefix, 32, "%i.", i);
+            assert(snprintf_ret < 32);
+
+            snprintf_ret = snprintf(opt_name, 48, "%s.%i", opts->name, i);
+            assert(snprintf_ret < 48);
+
+            qdict_extract_subqdict(subqdict, &subqdict_i, i_prefix);
+            if (!qdict_size(subqdict_i)) {
+                break;
+            }
+
+            subopts_i = qemu_opts_create(opts, opt_name, 1, NULL);
+            if (!subopts_i) {
+                error_setg(errp, "Option ID '%s' already in use", opt_name);
+                goto out;
+            }
+
+            qemu_opts_absorb_qdict(subopts_i, subqdict_i, &local_err);
+            if (error_is_set(&local_err)) {
+                error_propagate(errp, local_err);
+                qemu_opts_del(subopts_i);
+                goto out;
+            }
+
+            if (qdict_size(subqdict_i)) {
+                error_setg(errp, "[%s] rules don't support the option '%s'",
+                           opts->name, qdict_first(subqdict_i)->key);
+                qemu_opts_del(subopts_i);
+                goto out;
+            }
+
+            /*
+            if (add_rule(subopts_i, (void *)ard) < 0) {
+                error_setg(errp, "Could not add [%s] rule %i", opts->name, i);
+                goto out;
+            }
+            */
+
+            QDECREF(subqdict_i);
+            subqdict_i = NULL;
+        }
+
+        if (qdict_size(subqdict)) {
+            error_setg(errp, "Unused option '%s' for [%s]",
+                       qdict_first(subqdict)->key, opts->name);
+            goto out;
+        }
+    }
+
+out:
+    QDECREF(subqdict);
+    QDECREF(subqdict_i);
+
+    free(prefix);
+}
+
+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

Re: [Qemu-devel] [PATCH 3/6] qemu-option: Add qemu_config_parse_qdict()

[Fam Zheng]

On 2013年11月22日 03:05, Max Reitz wrote:
> 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         | 111 +++++++++++++++++++++++++++++++++++++++++++++
>   2 files changed, 117 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..80d82d4 100644
> --- a/util/qemu-config.c
> +++ b/util/qemu-config.c
> @@ -356,3 +356,114 @@ int qemu_read_config_file(const char *filename)
>           return -EINVAL;
>       }
>   }
> +
> +static void config_parse_qdict_section(QDict *options, QemuOptsList *opts,
> +                                       Error **errp)
> +{
> +    QemuOpts *subopts, *subopts_i;
> +    QDict *subqdict, *subqdict_i = NULL;
> +    char *prefix = g_strdup_printf("%s.", opts->name);
> +    Error *local_err = NULL;
> +    size_t orig_size, enum_size;
> +
> +    qdict_extract_subqdict(options, &subqdict, 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 rules */
> +        int i;
> +
> +        /* Not required anymore */
> +        qemu_opts_del(subopts);
> +
> +        for (i = 0; i < INT_MAX; i++) {
> +            char i_prefix[32], opt_name[48];
> +            size_t snprintf_ret;
> +
> +            snprintf_ret = snprintf(i_prefix, 32, "%i.", i);
> +            assert(snprintf_ret < 32);
> +
> +            snprintf_ret = snprintf(opt_name, 48, "%s.%i", opts->name, i);
> +            assert(snprintf_ret < 48);

Is there a length limit of opts->name? I.e. is this an error case rather 
than an assertion case?

> +
> +            qdict_extract_subqdict(subqdict, &subqdict_i, i_prefix);
> +            if (!qdict_size(subqdict_i)) {
> +                break;
> +            }
> +
> +            subopts_i = qemu_opts_create(opts, opt_name, 1, NULL);

Could pass in errp and skip error_setg below.

> +            if (!subopts_i) {
> +                error_setg(errp, "Option ID '%s' already in use", opt_name);
> +                goto out;
> +            }
> +
> +            qemu_opts_absorb_qdict(subopts_i, subqdict_i, &local_err);
> +            if (error_is_set(&local_err)) {
> +                error_propagate(errp, local_err);
> +                qemu_opts_del(subopts_i);
> +                goto out;
> +            }
> +
> +            if (qdict_size(subqdict_i)) {
> +                error_setg(errp, "[%s] rules don't support the option '%s'",

s/rules don't/section doesn't/

> +                           opts->name, qdict_first(subqdict_i)->key);
> +                qemu_opts_del(subopts_i);
> +                goto out;
> +            }
> +
> +            /*
> +            if (add_rule(subopts_i, (void *)ard) < 0) {
> +                error_setg(errp, "Could not add [%s] rule %i", opts->name, i);

s/rule/section/
Or just remove this hunk?

> +                goto out;
> +            }
> +            */
> +
> +            QDECREF(subqdict_i);
> +            subqdict_i = NULL;
> +        }
> +
> +        if (qdict_size(subqdict)) {
> +            error_setg(errp, "Unused option '%s' for [%s]",
> +                       qdict_first(subqdict)->key, opts->name);
> +            goto out;
> +        }
> +    }
> +
> +out:
> +    QDECREF(subqdict);
> +    QDECREF(subqdict_i);
> +
> +    free(prefix);
> +}
> +
> +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;
> +        }
> +    }
> +}
>

This does the work, but also seems ad-hoc. Providing an array of dicts 
in the command line should be a general logic, which would also be very 
useful for Quorum driver.

Fam

Re: [Qemu-devel] [PATCH 3/6] qemu-option: Add qemu_config_parse_qdict()

[Max Reitz]

On 22.11.2013 09:08, Fam Zheng wrote:
> On 2013年11月22日 03:05, Max Reitz wrote:
>> 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         | 111 
>> +++++++++++++++++++++++++++++++++++++++++++++
>>   2 files changed, 117 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..80d82d4 100644
>> --- a/util/qemu-config.c
>> +++ b/util/qemu-config.c
>> @@ -356,3 +356,114 @@ int qemu_read_config_file(const char *filename)
>>           return -EINVAL;
>>       }
>>   }
>> +
>> +static void config_parse_qdict_section(QDict *options, QemuOptsList 
>> *opts,
>> +                                       Error **errp)
>> +{
>> +    QemuOpts *subopts, *subopts_i;
>> +    QDict *subqdict, *subqdict_i = NULL;
>> +    char *prefix = g_strdup_printf("%s.", opts->name);
>> +    Error *local_err = NULL;
>> +    size_t orig_size, enum_size;
>> +
>> +    qdict_extract_subqdict(options, &subqdict, 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 rules */
>> +        int i;
>> +
>> +        /* Not required anymore */
>> +        qemu_opts_del(subopts);
>> +
>> +        for (i = 0; i < INT_MAX; i++) {
>> +            char i_prefix[32], opt_name[48];
>> +            size_t snprintf_ret;
>> +
>> +            snprintf_ret = snprintf(i_prefix, 32, "%i.", i);
>> +            assert(snprintf_ret < 32);
>> +
>> +            snprintf_ret = snprintf(opt_name, 48, "%s.%i", 
>> opts->name, i);
>> +            assert(snprintf_ret < 48);
>
> Is there a length limit of opts->name? I.e. is this an error case 
> rather than an assertion case?

Hm, yes, I had most of the code directly in block/blkdebug.c at first, 
that's why there are some issues here which work fine just for blkdebug, 
but are not that good in the general case. I'll use g_strdup_sprintf 
instead.

>
>> +
>> +            qdict_extract_subqdict(subqdict, &subqdict_i, i_prefix);
>> +            if (!qdict_size(subqdict_i)) {
>> +                break;
>> +            }
>> +
>> +            subopts_i = qemu_opts_create(opts, opt_name, 1, NULL);
>
> Could pass in errp and skip error_setg below.

Oh, right, missed that, thanks.

>
>> +            if (!subopts_i) {
>> +                error_setg(errp, "Option ID '%s' already in use", 
>> opt_name);
>> +                goto out;
>> +            }
>> +
>> +            qemu_opts_absorb_qdict(subopts_i, subqdict_i, &local_err);
>> +            if (error_is_set(&local_err)) {
>> +                error_propagate(errp, local_err);
>> +                qemu_opts_del(subopts_i);
>> +                goto out;
>> +            }
>> +
>> +            if (qdict_size(subqdict_i)) {
>> +                error_setg(errp, "[%s] rules don't support the 
>> option '%s'",
>
> s/rules don't/section doesn't/

Yes, this one also is fine for blkdebug, but not so much for the general 
case.

>
>> +                           opts->name, qdict_first(subqdict_i)->key);
>> +                qemu_opts_del(subopts_i);
>> +                goto out;
>> +            }
>> +
>> +            /*
>> +            if (add_rule(subopts_i, (void *)ard) < 0) {
>> +                error_setg(errp, "Could not add [%s] rule %i", 
>> opts->name, i);
>
> s/rule/section/
> Or just remove this hunk?

Hm, I probably should have completely reviewed my own code before 
sending it. Will do that in v2, promise. ;-)

>
>> +                goto out;
>> +            }
>> +            */
>> +
>> +            QDECREF(subqdict_i);
>> +            subqdict_i = NULL;
>> +        }
>> +
>> +        if (qdict_size(subqdict)) {
>> +            error_setg(errp, "Unused option '%s' for [%s]",
>> +                       qdict_first(subqdict)->key, opts->name);
>> +            goto out;
>> +        }
>> +    }
>> +
>> +out:
>> +    QDECREF(subqdict);
>> +    QDECREF(subqdict_i);
>> +
>> +    free(prefix);
>> +}
>> +
>> +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;
>> +        }
>> +    }
>> +}
>>
>
> This does the work, but also seems ad-hoc. Providing an array of dicts 
> in the command line should be a general logic, which would also be 
> very useful for Quorum driver.

Yes, I thought about quorum, too. I don't think just using QemuOpts 
there would be too bad, since the above function could do all the work 
there as well. I could probably introduce a QDict function which splits 
a QDict with keys enumerated in this way into a QList of QDicts. I'll 
have a look into how much more complex that will be and, depending on 
that, include it in v2 or not.

Thanks for reviewing,

Max

[Qemu-devel] [PATCH 4/6] 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 | 42 ++++++++++++++++++++++--------------------
 1 file changed, 22 insertions(+), 20 deletions(-)

diff --git a/block/blkdebug.c b/block/blkdebug.c
index 59d33a8..8d52173 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,12 +380,10 @@ 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, &local_err);
-        if (ret) {
-            error_propagate(errp, local_err);
-            goto fail;
-        }
+    ret = read_config(s, config, &local_err);
+    if (ret) {
+        error_propagate(errp, local_err);
+        goto fail;
     }
 
     /* Set initial state */
-- 
1.8.4.2

[Qemu-devel] [PATCH 5/6] 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 8d52173..5f94ee7 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, &local_err);
+    ret = read_config(s, config, options, &local_err);
     if (ret) {
         error_propagate(errp, local_err);
         goto fail;
-- 
1.8.4.2

[Qemu-devel] [PATCH 6/6] 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>
---
The x-raw option is undocumented and may be removed at any point in
time; but I don't see a reason why this should happen. Passing the
reference filename through an option maybe was a bit dirty in the past;
but by “exposing” it in the way this patch does, it becomes probably the
only clean solution.

The only problem I can see right now is the naming of the option, but we
should always be able to leave it as an alias, if necessary.
---
 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