[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v22 03/25] improve some functions in qemu-option
From: |
Eric Blake |
Subject: |
Re: [Qemu-devel] [PATCH v22 03/25] improve some functions in qemu-option.c |
Date: |
Mon, 10 Mar 2014 15:21:04 -0600 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:24.0) Gecko/20100101 Thunderbird/24.3.0 |
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?):
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";
^
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
signature.asc
Description: OpenPGP digital signature
- Re: [Qemu-devel] [PATCH v22 02/25] qapi: output def_value_str when query command line options, (continued)
[Qemu-devel] [PATCH v22 03/25] improve some functions in qemu-option.c, Chunyan Liu, 2014/03/10
Re: [Qemu-devel] [PATCH v22 03/25] improve some functions in qemu-option.c, Chunyan Liu, 2014/03/12
Re: [Qemu-devel] [PATCH v22 03/25] improve some functions in qemu-option.c, Leandro Dorileo, 2014/03/17
[Qemu-devel] [PATCH v22 05/25] add some QemuOpts functions for replace work, Chunyan Liu, 2014/03/10