Re: [Qemu-devel] [PATCH V13 4/6] create some QemuOpts functons

[Dong Xu Wang <wdongxu@linux.vnet.ibm.com>]

On 2013/5/7 15:38, Markus Armbruster wrote:
> Dong Xu Wang <wdongxu@linux.vnet.ibm.com> writes:
>
>> On 2013/5/6 20:20, Markus Armbruster wrote:
>>> Dong Xu Wang <wdongxu@linux.vnet.ibm.com> writes:
>>>
>>>> These functions will be used in next commit. qemu_opt_get_(*)_del functions
>>>> are used to make sure we have the same behaviors as before: after get an
>>>> option value, options++.
>>>
>>> I don't understand the last sentence.
>>>
I will make the sentence clear next version.

In block layer, I created 2 series of functions:
1) one is qemu_opt_get_del series, it is used to parse create options, 
after parsing one parameter, will delete it.
2) the other is qemu_opt_replace_set series, it will delete related opt 
and then add a new one.

>>>> Signed-off-by: Dong Xu Wang <wdongxu@linux.vnet.ibm.com>
>>>> ---
>>>>    include/qemu/option.h |  11 +++++-
>>>>    util/qemu-option.c    | 103 ++++++++++++++++++++++++++++++++++++++++++++++----
>>>>    2 files changed, 105 insertions(+), 9 deletions(-)
>>>>
>>>> diff --git a/include/qemu/option.h b/include/qemu/option.h
>>>> index c7a5c14..d63e447 100644
>>>> --- a/include/qemu/option.h
>>>> +++ b/include/qemu/option.h
>>>> @@ -108,6 +108,7 @@ struct QemuOptsList {
>>>>    };
>>>>
>>>>    const char *qemu_opt_get(QemuOpts *opts, const char *name);
>>>> +const char *qemu_opt_get_del(QemuOpts *opts, const char *name);
>>>>    /**
>>>>     * qemu_opt_has_help_opt:
>>>>     * @opts: options to search for a help request
>>>> @@ -121,13 +122,18 @@ const char *qemu_opt_get(QemuOpts *opts, const char *name);
>>>>     */
>>>>    bool qemu_opt_has_help_opt(QemuOpts *opts);
>>>>    bool qemu_opt_get_bool(QemuOpts *opts, const char *name, bool defval);
>>>> +bool qemu_opt_get_bool_del(QemuOpts *opts, const char *name, bool defval);
>>>>    uint64_t qemu_opt_get_number(QemuOpts *opts, const char *name, uint64_t defval);
>>>>    uint64_t qemu_opt_get_size(QemuOpts *opts, const char *name, uint64_t defval);
>>>> +uint64_t qemu_opt_get_size_del(QemuOpts *opts, const char *name,
>>>> +                               uint64_t defval);
>>>>    int qemu_opt_set(QemuOpts *opts, const char *name, const char *value);
>>>> +int qemu_opt_replace_set(QemuOpts *opts, const char *name, const char *value);
>>>>    void qemu_opt_set_err(QemuOpts *opts, const char *name, const char *value,
>>>>                          Error **errp);
>>>>    int qemu_opt_set_bool(QemuOpts *opts, const char *name, bool val);
>>>>    int qemu_opt_set_number(QemuOpts *opts, const char *name, int64_t val);
>>>> +int qemu_opt_replace_set_number(QemuOpts *opts, const char *name, int64_t val);
>>>>    typedef int (*qemu_opt_loopfunc)(const char *name, const char *value, void *opaque);
>>>>    int qemu_opt_foreach(QemuOpts *opts, qemu_opt_loopfunc func, void *opaque,
>>>>                         int abort_on_failure);
>>>> @@ -144,7 +150,10 @@ const char *qemu_opts_id(QemuOpts *opts);
>>>>    void qemu_opts_del(QemuOpts *opts);
>>>>    void qemu_opts_validate(QemuOpts *opts, const QemuOptDesc *desc, Error **errp);
>>>>    int qemu_opts_do_parse(QemuOpts *opts, const char *params, const char *firstname);
>>>> -QemuOpts *qemu_opts_parse(QemuOptsList *list, const char *params, int permit_abbrev);
>>>> +int qemu_opts_do_parse_replace(QemuOpts *opts, const char *params,
>>>> +                               const char *firstname);
>>>> +QemuOpts *qemu_opts_parse(QemuOptsList *list, const char *params,
>>>> +                          int permit_abbrev);
>>>>    void qemu_opts_set_defaults(QemuOptsList *list, const char *params,
>>>>                                int permit_abbrev);
>>>>    QemuOpts *qemu_opts_from_qdict(QemuOptsList *list, const QDict *qdict,
>>>> diff --git a/util/qemu-option.c b/util/qemu-option.c
>>>> index 0488c27..5db6d76 100644
>>>> --- a/util/qemu-option.c
>>>> +++ b/util/qemu-option.c
>>>> @@ -33,6 +33,8 @@
>>>>    #include "qapi/qmp/qerror.h"
>>>>    #include "qemu/option_int.h"
>>>>
>>>> +static void qemu_opt_del(QemuOpt *opt);
>>>> +
>>>>    /*
>>>>     * Extracts the name of an option from the parameter string (p points at the
>>>>     * first byte of the option name)
>>>> @@ -549,6 +551,16 @@ const char *qemu_opt_get(QemuOpts *opts, const char *name)
>>>      const char *qemu_opt_get(QemuOpts *opts, const char *name)
>>>      {
>>>          QemuOpt *opt = qemu_opt_find(opts, name);
>>>          const QemuOptDesc *desc;
>>>          desc = find_desc_by_name(opts->list->desc, name);
>>>
>>>          return opt ? opt->str :
>>>>            (desc && desc->def_value_str ? desc->def_value_str : NULL);
>>>>    }
>>>>
>>>> +const char *qemu_opt_get_del(QemuOpts *opts, const char *name)
>>>> +{
>>>> +    QemuOpt *opt = qemu_opt_find(opts, name);
>>>> +    const char *str = opt ? g_strdup(opt->str) : NULL;
>>>> +    if (opt) {
>>>> +        qemu_opt_del(opt);
>>>> +    }
>>>> +    return str;
>>>> +}
>>>> +
>>>
>>> Unlike qemu_opt_del(), this one doesn't use def_value_str.  Why?  Isn't
>>> that a trap for users of this function?
>>>
>>> Same question for the qemu_opt_get_FOO_del() that follow.
>
> I'm still interested in answers :)
>
Here I made a mistake, it should use def_value_str, will fix in next 
version.
>>>>    bool qemu_opt_has_help_opt(QemuOpts *opts)
>>>>    {
>>>>        QemuOpt *opt;
>>>> @@ -577,6 +589,22 @@ bool qemu_opt_get_bool(QemuOpts *opts, const char *name, bool defval)
>>>>        return opt->value.boolean;
>>>>    }
>>>>
>>>> +bool qemu_opt_get_bool_del(QemuOpts *opts, const char *name, bool defval)
>>>> +{
>>>> +    QemuOpt *opt = qemu_opt_find(opts, name);
>>>> +    bool ret;
>>>> +
>>>> +    if (opt == NULL) {
>>>> +        return defval;
>>>> +    }
>>>> +    ret = opt->value.boolean;
>>>> +    assert(opt->desc && opt->desc->type == QEMU_OPT_BOOL);
>>>> +    if (opt) {
>>>> +        qemu_opt_del(opt);
>>>> +    }
>>>> +    return ret;
>>>> +}
>>>> +
>>>>    uint64_t qemu_opt_get_number(QemuOpts *opts, const char *name, uint64_t defval)
>>>>    {
>>>>        QemuOpt *opt = qemu_opt_find(opts, name);
>>>> @@ -609,6 +637,23 @@ uint64_t qemu_opt_get_size(QemuOpts *opts, const char *name, uint64_t defval)
>>>>        return opt->value.uint;
>>>>    }
>>>>
>>>> +uint64_t qemu_opt_get_size_del(QemuOpts *opts, const char *name,
>>>> +                               uint64_t defval)
>>>> +{
>>>> +    QemuOpt *opt = qemu_opt_find(opts, name);
>>>> +    uint64_t ret;
>>>> +
>>>> +    if (opt == NULL) {
>>>> +        return defval;
>>>> +    }
>>>> +    ret = opt->value.uint;
>>>> +    assert(opt->desc && opt->desc->type == QEMU_OPT_SIZE);
>>>> +    if (opt) {
>>>> +        qemu_opt_del(opt);
>>>> +    }
>>>> +    return ret;
>>>> +}
>>>> +
>>>>    static void qemu_opt_parse(QemuOpt *opt, Error **errp)
>>>>    {
>>>>        if (opt->desc == NULL)
>>>> @@ -646,7 +691,7 @@ static bool opts_accepts_any(const QemuOpts *opts)
>>>>    }
>>>>
>>>>    static void opt_set(QemuOpts *opts, const char *name, const char *value,
>>>> -                    bool prepend, Error **errp)
>>>> +                    bool prepend, bool replace, Error **errp)
>>>>    {
>>>>        QemuOpt *opt;
>>>>        const QemuOptDesc *desc;
>>>> @@ -658,6 +703,11 @@ static void opt_set(QemuOpts *opts, const char *name, const char *value,
>>>>            return;
>>>>        }
>>>>
>>>> +    opt = qemu_opt_find(opts, name);
>>>> +    if (replace && opt) {
>>>> +        qemu_opt_del(opt);
>>>> +    }
>>>> +
>>>>        opt = g_malloc0(sizeof(*opt));
>>>>        opt->name = g_strdup(name);
>>>>        opt->opts = opts;
>>>> @@ -667,7 +717,6 @@ static void opt_set(QemuOpts *opts, const char *name, const char *value,
>>>>            QTAILQ_INSERT_TAIL(&opts->head, opt, next);
>>>>        }
>>>>        opt->desc = desc;
>>>> -    opt->str = g_strdup(value);
>>>>        opt->str = g_strdup(value ?: desc->def_value_str);
>>>>        qemu_opt_parse(opt, &local_err);
>>>>        if (error_is_set(&local_err)) {
>>>
>>> Here you plug the leak you created in PATCH 1/6.
>>>
>>>> @@ -680,7 +729,29 @@ int qemu_opt_set(QemuOpts *opts, const char *name, const char *value)
>>>>    {
>>>>        Error *local_err = NULL;
>>>>
>>>> -    opt_set(opts, name, value, false, &local_err);
>>>> +    opt_set(opts, name, value, false, false, &local_err);
>>>> +    if (error_is_set(&local_err)) {
>>>> +        qerror_report_err(local_err);
>>>> +        error_free(local_err);
>>>> +        return -1;
>>>> +    }
>>>> +
>>>> +    return 0;
>>>> +}
>>>> +
>>>> +/*
>>>> + * If name exists, the function will delete the opt first and then add a new
>>>> + * one.
>>>> + */
>>>> +int qemu_opt_replace_set(QemuOpts *opts, const char *name, const char *value)
>>>> +{
>>>> +    Error *local_err = NULL;
>>>> +    QemuOpt *opt = qemu_opt_find(opts, name);
>>>> +
>>>> +    if (opt) {
>>>> +        qemu_opt_del(opt);
>>>> +    }
>>>
>>> Why?  Won't opt_set() delete it already?
>>>
>> No, opt_set will not delete it, but add a new opt. In block layer,
>
> Then I'm confused...
>
>> while parsing options, if the parameter is parsed, it should be
>> deleted and won't be used again, so I delete it manually.
>>
>>> Same question for the qemu_opt_replace_set_FOO() that follow.
>>>
>>>> +    opt_set(opts, name, value, false, true, &local_err);
>
> ... because here you pass true for parameter replace, which I (naively?)
> expect to delete the option.  Can you unconfuse me?
>

Sorry, I used qemu_opt_del twice, will clean.

>>>>        if (error_is_set(&local_err)) {
>>>>            qerror_report_err(local_err);
>>>>            error_free(local_err);
>>>> @@ -693,7 +764,7 @@ int qemu_opt_set(QemuOpts *opts, const char *name, const char *value)
>>>>    void qemu_opt_set_err(QemuOpts *opts, const char *name, const char *value,
>>>>                          Error **errp)
>>>>    {
>>>> -    opt_set(opts, name, value, false, errp);
>>>> +    opt_set(opts, name, value, false, false, errp);
>>>>    }
>>>>
>>>>    int qemu_opt_set_bool(QemuOpts *opts, const char *name, bool val)
>>>> @@ -740,6 +811,16 @@ int qemu_opt_set_number(QemuOpts *opts, const char *name, int64_t val)
>>>>        return 0;
>>>>    }
>>>>
>>>> +int qemu_opt_replace_set_number(QemuOpts *opts, const char *name, int64_t val)
>>>> +{
>>>> +    QemuOpt *opt = qemu_opt_find(opts, name);
>>>> +
>>>> +    if (opt) {
>>>> +        qemu_opt_del(opt);
>>>> +    }
>>>> +    return qemu_opt_set_number(opts, name, val);
>>>> +}
>>>> +
>>>>    int qemu_opt_foreach(QemuOpts *opts, qemu_opt_loopfunc func, void *opaque,
>>>>                         int abort_on_failure)
>>>>    {
>>>> @@ -917,7 +998,7 @@ int qemu_opts_print(QemuOpts *opts)
>>>>    }
>>>>
>>>>    static int opts_do_parse(QemuOpts *opts, const char *params,
>>>> -                         const char *firstname, bool prepend)
>>>> +                         const char *firstname, bool prepend, bool replace)
>>>>    {
>>>>        char option[128], value[1024];
>>>>        const char *p,*pe,*pc;
>>>> @@ -953,7 +1034,7 @@ static int opts_do_parse(QemuOpts *opts, const char *params,
>>>>            }
>>>>            if (strcmp(option, "id") != 0) {
>>>>                /* store and parse */
>>>> -            opt_set(opts, option, value, prepend, &local_err);
>>>> +            opt_set(opts, option, value, prepend, replace, &local_err);
>>>>                if (error_is_set(&local_err)) {
>>>>                    qerror_report_err(local_err);
>>>>                    error_free(local_err);
>>>> @@ -969,7 +1050,13 @@ static int opts_do_parse(QemuOpts *opts, const char *params,
>>>>
>>>>    int qemu_opts_do_parse(QemuOpts *opts, const char *params, const char *firstname)
>>>>    {
>>>> -    return opts_do_parse(opts, params, firstname, false);
>>>> +    return opts_do_parse(opts, params, firstname, false, false);
>>>> +}
>>>> +
>>>> +int qemu_opts_do_parse_replace(QemuOpts *opts, const char *params,
>>>> +                               const char *firstname)
>>>> +{
>>>> +    return opts_do_parse(opts, params, firstname, false, true);
>>>>    }
>>>>
>>>>    static QemuOpts *opts_parse(QemuOptsList *list, const char *params,
>>>> @@ -1008,7 +1095,7 @@ static QemuOpts *opts_parse(QemuOptsList *list, const char *params,
>>>>            return NULL;
>>>>        }
>>>>
>>>> -    if (opts_do_parse(opts, params, firstname, defaults) != 0) {
>>>> +    if (opts_do_parse(opts, params, firstname, defaults, false) != 0) {
>>>>            qemu_opts_del(opts);
>>>>            return NULL;
>>>>        }
>>>
>>>
>
>
>