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

[Luiz Capitulino <lcapitulino@redhat.com>]

On Sat, 21 Jul 2012 10:09:09 +0200
Markus Armbruster <armbru@redhat.com> wrote:

> Luiz Capitulino <lcapitulino@redhat.com> writes:
> 
> > Call qemu_opt_set() instead of duplicating opt_set().
> >
> > Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
> > ---
> >  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.

The end result of calling qemu_opt_set_bool() is the same. The difference
between qerror_report() and qerror_report_err() is that the former gets error
information from the error call, while the latter gets error information
from the Error object. But they do the same thing.

> * 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?

I don't remember if opt->str is read after the QemuOpt object is built. If
it's, then yes, this is a bug fix. Otherwise it just make the final
QemuOpt object more 'conforming'.