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