Re: [Qemu-devel] [RFC PATCH v2 0/7] qapi: Specify default value for optional argument in schema json

[Kevin Wolf]

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.

Kevin