Patchwork [1/8] qemu-option: qemu_opt_set_bool(): fix code duplication

login
register
mail settings
Submitter Luiz Capitulino
Date July 13, 2012, 8:44 p.m.
Message ID <1342212261-19903-2-git-send-email-lcapitulino@redhat.com>
Download mbox | patch
Permalink /patch/170955/
State New
Headers show

Comments

Luiz Capitulino - July 13, 2012, 8:44 p.m.
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(-)
Markus Armbruster - July 21, 2012, 8:09 a.m.
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.

* 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?
Luiz Capitulino - July 23, 2012, 4:10 p.m.
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'.
Markus Armbruster - July 23, 2012, 6:14 p.m.
Luiz Capitulino <lcapitulino@redhat.com> writes:

> 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'.

Uses of opt->str, and what happens when it isn't set:

* qemu_opt_get(): returns NULL, which means "not set".  Bug can bite
  when value isn't the default value.

* qemu_opt_parse(): passes NULL to parse_option_bool(), which treats it
  like "on".  Wrong if the value is actually false.  Bug can bite when
  qemu_opts_validate() runs after qemu_opt_set_bool().

* qemu_opt_del(): passes NULL to g_free(), which is just fine.

* qemu_opt_foreach(): passes NULL to the callback, which is unlikely to
  be prepared for it.

* qemu_opts_print(): prints NULL, which crashes on some systems.

* qemu_opts_to_qdict(): passes NULL to qstring_from_str(), which
  crashes.

Right now, the only use of qemu_opt_set_bool() is the one in vl.c.
Can't see how to break it, but I didn't look hard.

I recommend to document the bug fix in the commit message.
Luiz Capitulino - July 23, 2012, 7:46 p.m.
On Mon, 23 Jul 2012 20:14:31 +0200
Markus Armbruster <armbru@redhat.com> wrote:

> Luiz Capitulino <lcapitulino@redhat.com> writes:
> 
> > 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'.
> 
> Uses of opt->str, and what happens when it isn't set:
> 
> * qemu_opt_get(): returns NULL, which means "not set".  Bug can bite
>   when value isn't the default value.
> 
> * qemu_opt_parse(): passes NULL to parse_option_bool(), which treats it
>   like "on".  Wrong if the value is actually false.  Bug can bite when
>   qemu_opts_validate() runs after qemu_opt_set_bool().
> 
> * qemu_opt_del(): passes NULL to g_free(), which is just fine.
> 
> * qemu_opt_foreach(): passes NULL to the callback, which is unlikely to
>   be prepared for it.
> 
> * qemu_opts_print(): prints NULL, which crashes on some systems.
> 
> * qemu_opts_to_qdict(): passes NULL to qstring_from_str(), which
>   crashes.

Thanks for the clarification.

> 
> Right now, the only use of qemu_opt_set_bool() is the one in vl.c.
> Can't see how to break it, but I didn't look hard.
> 
> I recommend to document the bug fix in the commit message.

Ok, will do.

Patch

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,