[PATCH RFC 0/5] Subject: [PATCH RFC 0/5] qapi: Add feature flags to enum members

[Vladimir Sementsov-Ogievskiy]

16.09.2021 15:57, Markus Armbruster wrote:
> Vladimir Sementsov-Ogievskiy <vsementsov at virtuozzo.com> writes:
> 
>> Great! Thanks for working on this!
>>
>> 15.09.2021 22:24, Markus Armbruster wrote:
>>> PATCH 1+2 add feature flags to enum members.  Awkward due to an
>>> introspection design mistake; see PATCH 1 for details.  Feedback
>>> welcome, in particular from management application guys.
>>> PATCH 3+4 implement policy deprecated-input={reject,crash} for enum
>>> values.
>>>
>>> Policy deprecated-output=hide is not implemented, because we can't
>>> hide a value without hiding the entire member, which is almost
>>> certainly more than the requester of this policy bargained for.
>>> Perhaps we want a new policy deprecated-output=crash to help us catch
>>> unwanted use of deprecated enum values.  Thoughts?
>>
>> I think crash policy for output is doubtful. If we have query-*
>> command that returns a lot of things and some of them are deprecated
>> the whole command will always crash..
> 
> Policy "crash" is not quite as crazy as it may look :)
> 
> The non-default policies for handling deprecated interfaces are intended
> for testing users of these interfaces.
> 
> Input policy reject and output policy hide enable "testing the future".
> 
> Input policy crash is a robust way to double-check "management
> application does not send deprecated input".  This is not quite the same
> as "testing the future".  A management application may choose to send
> deprecated input, detect the failure and recover.  Testing that should
> pass with input policy reject, but not with input policy crash.
> 
> Output policy crash could be a way to double-check "the management
> application does not make QEMU send deprecated output".
> 
> Example: Say we deprecate BlockdevDriver member 'vvfat'.  We know that
> output of query-named-block-nodes can contain 'vvfat' only if something
> creates such nodes.  Input policy reject will reject attempts to use
> this driver with blockdev-add.  But what if the management application
> uses some other way to create such nodes, not (yet) covered by input
> policy?  Output policy crash could be used to double-check it doesn't.
> 
> Except it doesn't actually work, because as you said, testing will
> likely produce *other* deprecated output that should *not* crash the
> test.
> 
> Perhaps a policy hide-or-else-crash could work.
> 
>>                                         Deprecated is "use" of the
>> deprecated field, but we can't control does user use a specific field
>> or not.
>>
>> If some deprecated output is a consequence of deprecated input, we'd
>> better always show it.. Or in other words, we shouldn't deprecate such
>> output, deprecating of the corresponding input is enough.
> 
> Deprecating only input may require duplicating definitions used both for
> input and output, unless we replace today's feature flags by something
> more sophisticated.
> 
> Example: BlockdevDriver is used both as input of blockdev-add and as
> output of query-named-block-nodes.  Deprecate member 'vvfat' affects
> both input and output.


If we deprecate a vvfat, but still have some not deprecated ways to create vvfat node, that's a kind of bug[1] in deprecation system: if vvfat is deprecated, all ways to create vvfat should go through input compat policy.

So, making qemu crash on trying to output "vvfat" for me looks like a workaround for that bug[1]. And it's strange to create and interface to workaround the internal bug..

May be, we can just enable hide-or-else-crash behavior automatically, when user choose input=crash and output=hide?

> 
>> So, we are saying about showing some internal state to the user. And
>> part of this state becomes deprecated because internal implementation
>> changed. I think the only correct thing to do is handle
>> deprecated=hide by hand, in the place where we set this deprecated
>> field. Only at this place we can decide, should we simulate old
>> deprecated output value or not. For this we'll need a possibility to
>> get current policy at any place, but that doesn't seem to be a
>> problem, I see, it's just a global variable compat_policy.
> 
> I'm not sure I fully got this.
> 
> Compat policies are not about optionally providing older versions of an
> interface ("simulate old deprecated output value").  They try to help
> with testing users of interfaces.  One aspect of that is optionally
> approximating expected future interfaces.
> 

-- 
Best regards,
Vladimir