Re: [PATCH 15/46] qemu-option: Tidy up opt_set() not to free arguments on failure

[Vladimir Sementsov-Ogievskiy]

24.06.2020 19:43, Markus Armbruster wrote:
> opt_set() frees its argument @value on failure.  Slightly unclean;
> functions ideally do nothing on failure.
>
> To tidy this up, move opt_create() from opt_set() into its callers,
> along with the cleanup.

Hmm, let me think a bit..

So, prior to this patch:

opt_set gets name/value pair and sets the option in opts object, it
seems absolutely obvious and standard behavior for Map-like object.

The fact that for setting an option we create a QemuOpt object, and
somehow register it inside opts object is an implementation detail.

after the patch:

opt_set gets opt object, which is already registered in opts. So,
it seems like option is "partly" set already, and opt_set only
finalize the processing.

And, as opt_set() only finalize the "set" operation, on opt_set
failure we need additional roll-back of "set" operation first step.

Additional fact, indirectly showing that something is unclear here
is that we pass "opts" to opt_set twice: as "opts" parameter and
inside opt: (opt->opts must be the same, assertion won't hurt if
you decide to keep the patch).

=====

Semantics before the patch seems clearer to me.

To improve the situation around "value", we can just g_strdup it
in opt_create as well as "name" argument (and use const char*
type for "value" argument as well)

> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>   util/qemu-option.c | 33 ++++++++++++++++++---------------
>   1 file changed, 18 insertions(+), 15 deletions(-)
>
> diff --git a/util/qemu-option.c b/util/qemu-option.c
> index 3cdf0c0800..14946e81f2 100644
> --- a/util/qemu-option.c
> +++ b/util/qemu-option.c
> @@ -519,36 +519,39 @@ static QemuOpt *opt_create(QemuOpts *opts, const char 
> *name, char *value,
>       return opt;
>   }
>   
> -static void opt_set(QemuOpts *opts, const char *name, char *value,
> -                    bool prepend, bool *help_wanted, Error **errp)
> +static bool opt_set(QemuOpts *opts, QemuOpt *opt, bool *help_wanted,
> +                    Error **errp)
>   {
> -    QemuOpt *opt;
>       const QemuOptDesc *desc;
>       Error *local_err = NULL;
>   
> -    desc = find_desc_by_name(opts->list->desc, name);
> +    desc = find_desc_by_name(opts->list->desc, opt->name);
>       if (!desc && !opts_accepts_any(opts)) {
> -        g_free(value);
> -        error_setg(errp, QERR_INVALID_PARAMETER, name);
> -        if (help_wanted && is_help_option(name)) {
> +        error_setg(errp, QERR_INVALID_PARAMETER, opt->name);
> +        if (help_wanted && is_help_option(opt->name)) {
>               *help_wanted = true;
>           }
> -        return;
> +        return false;
>       }
>   
> -    opt = opt_create(opts, name, value, prepend);
>       opt->desc = desc;
>       qemu_opt_parse(opt, &local_err);
>       if (local_err) {
>           error_propagate(errp, local_err);
> -        qemu_opt_del(opt);
> +        return false;
>       }
> +
> +    return true;
>   }
>   
>   void qemu_opt_set(QemuOpts *opts, const char *name, const char *value,
>                     Error **errp)
>   {
> -    opt_set(opts, name, g_strdup(value), false, NULL, errp);
> +    QemuOpt *opt = opt_create(opts, name, g_strdup(value), false);
> +
> +    if (!opt_set(opts, opt, NULL, errp)) {
> +        qemu_opt_del(opt);
> +    }
>   }
>   
>   void qemu_opt_set_bool(QemuOpts *opts, const char *name, bool val,
> @@ -820,9 +823,9 @@ static void opts_do_parse(QemuOpts *opts, const char 
> *params,
>                             const char *firstname, bool prepend,
>                             bool *help_wanted, Error **errp)
>   {
> -    Error *local_err = NULL;
>       char *option, *value;
>       const char *p;
> +    QemuOpt *opt;
>   
>       for (p = params; *p;) {
>           p = get_opt_name_value(p, firstname, &option, &value);
> @@ -834,10 +837,10 @@ static void opts_do_parse(QemuOpts *opts, const char 
> *params,
>               continue;
>           }
>   
> -        opt_set(opts, option, value, prepend, help_wanted, &local_err);
> +        opt = opt_create(opts, option, value, prepend);
>           g_free(option);
> -        if (local_err) {
> -            error_propagate(errp, local_err);
> +        if (!opt_set(opts, opt, help_wanted, errp)) {
> +            qemu_opt_del(opt);
>               return;
>           }
>       }


--
Best regards,
Vladimir