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

[Max Reitz <mreitz@redhat.com>]

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