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

[Chunyan Liu]

2014-03-17 5:19 GMT+08:00 Leandro Dorileo <address@hidden>:

Hi Chunyan,

On Tue, Mar 11, 2014 at 09:00:04PM +0000, Leandro Dorileo wrote:
> Hi,
>
> On Tue, Mar 11, 2014 at 03:26:51PM +0800, Chunyan Liu wrote:
> > 2014-03-11 5:21 GMT+08:00 Eric Blake <address@hidden>:
> >
> > > 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?
>
> The testsuite should test the QemuOpts implementation, not the current users.

I have just posted a patch introducing a basic QemuOpt testsuite, we can use it
as a start.

Thanks a lot, I've seen that.

 

Regards...

--
Leandro Dorileo