| Message ID | 1342212261-19903-2-git-send-email-lcapitulino@redhat.com |
|---|---|
| State | New |
| Headers | show |
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?
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'.
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.
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.
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,
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(-)