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