Re: [Qemu-devel] [PATCH v2 2/6] qapi: support nested structs in OptsVisitor

[Kővágó Zoltán]

2015-06-17 13:18 keltezéssel, Markus Armbruster írta:
> Copying Kevin because similar issues exist in the block layer.
>
> Gerd Hoffmann <address@hidden> writes:
>
> > On Mi, 2015-06-17 at 09:50 +0200, Markus Armbruster wrote:
> > > Copying László because his fingerprints are on OptsVisitor.
> > >
> > > "Kővágó, Zoltán" <address@hidden> writes:
> > >
> > > > The current OptsVisitor flattens the whole structure, if there are
> > > > same named
> > > > fields under different paths (like `in' and `out' in `Audiodev'),
> > > > the current
> > > > visitor can't cope with them (for example setting
> > > > frequency=44100' will set the
> > > > in's frequency to 44100 and leave out's frequency unspecified).
> > > >
> > > > This patch fixes it, by the following changes:
> > > > 1) Specifying just the field name will apply to all fields that has the
> > > >     specified name (this means it would set both in's and out's frequency to
> > > >     44100 in the above example).
>
> What if they have different types?
>
> What if one of them can't take the value?

Currently it will error out, requiring the user to be more explicit. 
Probably not the best solution, but I can't really think of a better 
solution. (If we would ignore invalid values that would be very 
confusing imho.)

> > > > 2) Optionally user can specify the path in the hierarchy. Names
> > > > are separated by
> > > >     a dot (e.g. `in.frequency', `foo.bar.something', etc). The user need not
> > > >     specify the whole path, only the last few components
> > > > (i.e. `bar.something' is
> > > >     equivalent to `foo.bar.something' if only `foo' has a `bar'
> > > > field). This way
> > > >     1) is just a special case of this when only the last component
> > > > is specified.
> > > > 3) In case of an ambiguity (e.g
> > > > frequency=44100,in.frequency=8000') the longest
> > > >     matching (the most specific) path wins (so in this example,
> > > > in's frequency
> > > >     would become 8000, because `in.frequency' is more specific that
> > > > frequency',
> > > >     and out's frequency would become 44100, because only
> > > > frequency' matches it).
>
> The current rule for multiple assignments is "last one wins".  E.g. in
>
>      -drive if=none,file=tmp.img,file=tmp.qcow2
>
> file=tmp.qcow2 wins.
>
> If I understand correctly, this patch amends the rule to "last most
> specific one wins".  Correct?

Yes. (But I didn't really checked that as I didn't know about the "last 
one win", and just thought it's an artifact of the current implementation.)

> > > Can you explain why the complexity is needed, i.e. why we can't just
> > > require full paths always?
> >
> > Keeping the short names is required for -netdev backward compatibility.
>
> I suspect mostly because NetLegacy and Netdev aren't flat unions.
> Could be self-inflicted pain.
>
> What about flattening them instead?  Assuming that's possible; I'd have
> to try.
>
> > Restricting to short or full (i.e. something= or foo.bar.something=, but
> > disallow bar.something=) should not be a problem.  I'm not sure this
> > simplifies things much though.  We have to build the full path anyway,
> > and I think bar.something= is just a convenient thing we get almost for
> > free ...
>
> We've been bitten by convenience features before.  Adding them tends to
> be cheap, but maintaining compatibility can become a terrible headache.