Re: [PATCH v2 3/3] qapi: deprecate drive-backup

[Markus Armbruster]

Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> writes:

> 08.06.2021 14:12, Markus Armbruster wrote:
>> Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> writes:
>> 
>> [...]
>> 
>>> TODO: We also need to deprecate drive-backup transaction action..
>>> But union members in QAPI doesn't support 'deprecated' feature. I tried
>>> to dig a bit, but failed :/ Markus, could you please help with it? At
>>> least by advice?
>> 
>> There are two closely related things in play here: the union branch and
>> the corresponding enum value.
>> 
>> So far, the QAPI schema language doesn't support tacking feature flags
>> to either.
>> 
>> If an enum value is deprecated, any union branches corresponding to it
>> must also be deprecated (because their use requires using the deprecated
>> enum value).
>> 
>> The converse is not true, but I can't see a use for deprecating a union
>> branch without also deprecating the enum member.
>> 
>> I think we can implement feature flags just for enum members, then
>> document that 'deprecated' enum value implies corresponding union
>> branches are also deprecated.
>> 
>> I have unfinished patches implementing feature flags for enum members.
>> 
>> Since TransactionAction is a simple union, the corresponding enum is
>> implicit.  We can make it explicit by converting to a flat union.
>> Simple unions need to die anyway.
>
>
> Does BlockStatsSpecific from qapi/block-core.json a correct example of flat 
> union you mean? I can make patch to convert TransactionAction to be similar 
> if that helps (discriminator field should be called "type", yes?).

>From docs/devel/qapi-code-gen.txt:

    A simple union can always be re-written as a flat union where the base
    class has a single member named 'type', and where each branch of the
    union has a struct with a single member named 'data'.  That is,

     { 'union': 'Simple', 'data': { 'one': 'str', 'two': 'int' } }

    is identical on the wire to:

     { 'enum': 'Enum', 'data': ['one', 'two'] }
     { 'struct': 'Branch1', 'data': { 'data': 'str' } }
     { 'struct': 'Branch2', 'data': { 'data': 'int' } }
     { 'union': 'Flat', 'base': { 'type': 'Enum' }, 'discriminator': 'type',
       'data': { 'one': 'Branch1', 'two': 'Branch2' } }

The generated C isn't identical, but adjusting the code using it should
be straightforward.

>> Does this make sense?
>> 
>
> Yes if it helps)
>
> Did you also look at John's 
> https://gitlab.com/jsnow/qemu/-/commits/hack-deprecate-union-branches/ ?

Not yet.

> I hope you and John will send patches that you have, I'll help with reviewing 
> (keep me in CC), and finally we'll get the feature.

Sounds like a plan.  I need to get my post-vacation e-mail pileup under
control first.