On Mon, 23 Jul 2012 20:14:31 +0200 Markus Armbruster <arm...@redhat.com> wrote:
> Luiz Capitulino <lcapitul...@redhat.com> writes: > > > On Sat, 21 Jul 2012 10:09:09 +0200 > > Markus Armbruster <arm...@redhat.com> wrote: > > > >> Luiz Capitulino <lcapitul...@redhat.com> writes: > >> > >> > Call qemu_opt_set() instead of duplicating opt_set(). > >> > > >> > Signed-off-by: Luiz Capitulino <lcapitul...@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.