Re: [Qemu-devel] [PATCH v22 03/25] improve some functions in qemu-option.c

[Chunyan Liu <cyliu@suse.com>]

[-- Attachment #1: Type: text/plain, Size: 3405 bytes --]

2014-03-11 5:21 GMT+08:00 Eric Blake <eblake@redhat.com>:

> On 03/10/2014 02:29 PM, Eric Blake wrote:
>
> >> +    opt = qemu_opt_find(opts, name);
> >> +    if (opt) {
> >> +        g_free((char *)opt->str);
> >
> > ...which means the cast is pointless here.
> >
> > Hmm.  This means that you are giving opt_set() the behavior of 'last
> > version wins', by silently overwriting earlier versions.  If I'm
> > understanding the existing code correctly, the previous behavior was
> > that calling opt_set twice in a row on the same name would inject BOTH
> > names into 'opts', but then subsequent lookups on opts would find the
> > FIRST hit.  Doesn't that mean this is a semantic change:
> >
> > qemu -opt key=value1,key=value2
> >
> > would previously set key to value1, but now sets key to value2.
>
> I've played with this a bit more, and now am more confused.  QemuOpts is
> a LOT to comprehend.
>
> Pre-patch, 'qemu-system-x86_64 -nodefaults -machine
> type=none,type-noone' displayed a help message about unknown machine
> type "noone", while swapping type=noone,type=none proceeded with the
> 'none' type.  So the last version silently won, which was not the
> behavior I had predicted.
>
> Post-patch, I get a compilation error (so how did you test your patch?):
>

Mostly tested ./qemu-img commands where QEMUOptionParameter is used.
I really didn't think of test QemuOpts fully, and about the test suite, I
have no full
knowledge about how many things need to be tested, how many things need to
be
covered?


> qapi/opts-visitor.c: In function ‘opts_start_struct’:
> qapi/opts-visitor.c:146:31: error: assignment discards ‘const’ qualifier
> from pointer target type [-Werror]
>          ov->fake_id_opt->name = "id";
>                                ^
>

This is a place needed to be changed but missing, since name and str
changed to be (char *), here should be g_strdup("id"). Due to my
configuration,
it appears as a warning instead of error which is missed. Updated 3/25.


> If I press on in spite of that warning, then I get the same behavior
> where the last type= still wins on behavior.  So I'm not sure how it all
> worked, but at least behavior wise, my one test didn't uncover a
> regression.
>
> Still, I'd feel a LOT better with a testsuite of what QemuOpts is
> supposed to be able to do.  tests/test-opts-visitor.c was the only file
> in tests/ that even mentions QemuOpts.
>
>

>
>> @@ -744,16 +777,24 @@ 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)
> >
> >> +    opt = qemu_opt_find(opts, name);
> >> +    if (opt) {
> >> +        g_free((char *)opt->str);
> >
> > Another pointless cast.
>
> Maybe not pointless, if you end up not removing the const in the struct
> declaration due to the compile error; but that brings me back to my
> earlier question - since the compiler error proves that we have places
> that are assigning compile-time string constants into the name field, we
> must NOT call g_free on those copies - how does your code distinguish
> between a QemuOpt that is built up by mallocs, vs. one that is described
> by compile-time constants?
>
> --
> Eric Blake   eblake redhat com    +1-919-301-3266
> Libvirt virtualization library http://libvirt.org
>
>

[-- Attachment #2: Type: text/html, Size: 4859 bytes --]