qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [RFC PATCH v2 0/7] qapi: Specify default value for opti


From: Kevin Wolf
Subject: Re: [Qemu-devel] [RFC PATCH v2 0/7] qapi: Specify default value for optional argument in schema json
Date: Wed, 21 May 2014 11:01:13 +0200
User-agent: Mutt/1.5.21 (2010-09-15)

Am 21.05.2014 um 10:42 hat Fam Zheng geschrieben:
> On Wed, 05/21 10:23, Kevin Wolf wrote:
> > Am 21.05.2014 um 09:46 hat Markus Armbruster geschrieben:
> > > Fam Zheng <address@hidden> writes:
> > > 
> > > > On Wed, 05/21 07:54, Markus Armbruster wrote:
> > > >> Fam Zheng <address@hidden> writes:
> > > >> 
> > > >> > On Tue, 05/20 13:13, Eric Blake wrote:
> > > >> >> On 05/20/2014 03:07 AM, Fam Zheng wrote:
> > > >> >> > Please first take a look at patch 7 to see what is supported by 
> > > >> >> > this series.
> > > >> >> > 
> > > >> >> > Patch 1 ~ 3 allows some useful basic types in schema.
> > > >> >> > 
> > > >> >> > Patch 4 ~ 6 implements the new syntax.
> > > >> >> > 
> > > >> >> > Note: The introduced '@arg' sigil, just like the preexisting 
> > > >> >> > '*arg', is
> > > >> >> > reducing the cleanness of the syntax. We should get rid of both 
> > > >> >> > of them in long
> > > >> >> > term. Here, this series compromises on this and introduces '@arg' 
> > > >> >> > because:
> > > >> >> > 
> > > >> >> >   - We have to distinguish the argument property dictionary from 
> > > >> >> > nested struct:
> > > >> >> > 
> > > >> >> >     I.e.:
> > > >> >> > 
> > > >> >> >         'data': {
> > > >> >> >             'arg1': { 'member1': 'int', 'member2': 'str' }
> > > >> >> >             '@arg2': { 'type': 'int', 'default': 100 }
> > > >> >> >          }
> > > >> >> > 
> > > >> >> >     Until we completely drop and forbid the 'arg1' nested struct 
> > > >> >> > use case.
> > > >> >> > 
> > > >> >> >   - Forbidding 'arg1' it's doable, but doing it now means we pull 
> > > >> >> > in many
> > > >> >> >     distractive patches to this series.
> > > >> >> 
> > > >> >> Question - since we WANT to get rid of nested struct, why not 
> > > >> >> reverse
> > > >> >> the sense?  Mark all existing nested structs (weren't there just 
> > > >> >> three
> > > >> >> that we found?) with the '@' sigil, and let the new syntax be
> > > >> >> sigil-free.  Then when we clean up the nesting, we are also getting 
> > > >> >> rid
> > > >> >> of the bad syntax, plus the sigil gives us something to search for 
> > > >> >> in
> > > >> >> knowing how much to clean up.  But if you stick the sigil on the new
> > > >> >> code, instead of the obsolete code, then as more and more places in 
> > > >> >> the
> > > >> >> schema use defaults, it gets harder and harder to remove the use of 
> > > >> >> the
> > > >> >> sigil even if the nested structs are eventually removed.
> > > >> >> 
> > > >> >
> > > >> > It makes not much difference I can see. The hard part is actaully 
> > > >> > dropping
> > > >> > nested, converting from sigil <-> non-sigil is easy. Of course, 
> > > >> > nothing is
> > > >> > seriously hard, there are only three nested structs plus some more
> > > >> > qapi-schema
> > > >> > test code.
> > > >> 
> > > >> Adding three ugly sigils and making everybody include one when they add
> > > >> a nested struct feels much better to me than ugly sigils all over the
> > > >> place.
> > > >
> > > > Well, I could use some background here. Why did we introduce nested 
> > > > structure
> > > > in the first place?
> > > 
> > > Because we could?
> > > 
> > > Felt like a good idea at the time?
> > > 
> > > I quick glance at commit 0f923be and fb3182c suggests they have been
> > > supported since the beginning.  There is no design rationale.
> > 
> > Let me extend Fam's question: Why don't we simply remove them right
> > now? If it's really only three instances, converting them to full
> > types should be a matter of five minutes.
> > 
> 
> Actually, my question is: do we want it independently, or do we want to 
> include
> the removal of nested as the first part of this series?
> 
> I would prefer the former because I feel uncomfortable with making more 
> changes
> in this series, since there are already many things to do: adding qapi types,
> adding argument property dict, adding all test cases for all of them, updating
> documentation, and apply the new syntax in qapi-schema.json. A non-RFC 
> revision
> could be long and hard to review.

The removal of nested structs must come first. Whether it's done as part
of this series or as a separate series that this one will depend on
doesn't really matter that much. I think I would do it as one big series
(and 8 patches with a diffstat like this is not that big, after all),
but if you prefer to split it up, I think that should be okay, too.

Kevin



reply via email to

[Prev in Thread] Current Thread [Next in Thread]