Re: [Qemu-devel] [PATCH 1/8] qemu-option: qemu_opt_set_bool(): fix code duplication

[Markus Armbruster]

Luiz Capitulino <address@hidden> writes:

> Call qemu_opt_set() instead of duplicating opt_set().
>
> Signed-off-by: Luiz Capitulino <address@hidden>
> ---
>  qemu-option.c | 28 +---------------------------
>  1 file changed, 1 insertion(+), 27 deletions(-)
>
> diff --git a/qemu-option.c b/qemu-option.c
> index bb3886c..2cb2835 100644
> --- a/qemu-option.c
> +++ b/qemu-option.c
> @@ -677,33 +677,7 @@ void qemu_opt_set_err(QemuOpts *opts, const char
> *name, const char *value,
>  
>  int qemu_opt_set_bool(QemuOpts *opts, const char *name, bool val)
>  {
> -    QemuOpt *opt;
> -    const QemuOptDesc *desc = opts->list->desc;
> -    int i;
> -
> -    for (i = 0; desc[i].name != NULL; i++) {
> -        if (strcmp(desc[i].name, name) == 0) {
> -            break;
> -        }
> -    }
> -    if (desc[i].name == NULL) {
> -        if (i == 0) {
> -            /* empty list -> allow any */;
> -        } else {
> -            qerror_report(QERR_INVALID_PARAMETER, name);
> -            return -1;
> -        }
> -    }
> -
> -    opt = g_malloc0(sizeof(*opt));
> -    opt->name = g_strdup(name);
> -    opt->opts = opts;
> -    QTAILQ_INSERT_TAIL(&opts->head, opt, next);
> -    if (desc[i].name != NULL) {
> -        opt->desc = desc+i;
> -    }
> -    opt->value.boolean = !!val;
> -    return 0;
> +    return qemu_opt_set(opts, name, val ? "on" : "off");
>  }
>  
>  int qemu_opt_foreach(QemuOpts *opts, qemu_opt_loopfunc func, void *opaque,

Does a bit more than just obvious code de-duplication.  Two things in
particular:

* Error reporting

  Old: qerror_report(); return -1

  New: opt_set() and qemu_opt_set() cooperate, like this:
       opt_set(): error_set(); return;
       qemu_opt_set():
            if (error_is_set(&local_err)) {
                qerror_report_err(local_err);
                error_free(local_err);
                return -1;
            }

  I guess the net effect is the same.  Not sure it's worth mentioning in
  the commit message.

* New sets opt->str to either "on" or "off" depending on val, then lets
  reconstructs the value with qemu_opt_parse().  Which can't fail then.
  I figure the latter part has no further impact.  But what about
  setting opt->str?  Is this a bug fix?