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?

PATCH 5 puts the new feature flags to use.  It makes sense only on top
of Vladimir's deprecation of drive-backup.  See its commit message for
a reference.

Based on my "[PATCH 00/22] qapi: Remove simple unions from the schema
language".

Based-on: Message-Id: <20210913123932.3306639-1-armbru@redhat.com>

Markus Armbruster (5):
  qapi: Enable enum member introspection to show more than name
  qapi: Add feature flags to enum members
  qapi: Move compat policy from QObject to generic visitor
  qapi: Implement deprecated-input={reject,crash} for enum values
  block: Deprecate transaction type drive-backup

 docs/devel/qapi-code-gen.rst                  |  4 ++-
 qapi/compat.json                              |  3 +++
 qapi/introspect.json                          | 23 ++++++++++++++--
 qapi/transaction.json                         |  5 +++-
 include/qapi/qobject-input-visitor.h          |  4 ---
 include/qapi/qobject-output-visitor.h         |  4 ---
 include/qapi/util.h                           |  6 ++++-
 include/qapi/visitor-impl.h                   |  3 +++
 include/qapi/visitor.h                        |  9 +++++++
 qapi/qapi-visit-core.c                        | 27 ++++++++++++++++---
 qapi/qmp-dispatch.c                           |  4 +--
 qapi/qobject-input-visitor.c                  | 14 +---------
 qapi/qobject-output-visitor.c                 | 14 +---------
 scripts/qapi/expr.py                          |  3 ++-
 scripts/qapi/introspect.py                    | 19 ++++++++++---
 scripts/qapi/schema.py                        | 22 +++++++++++++--
 scripts/qapi/types.py                         | 17 +++++++++++-
 tests/qapi-schema/doc-good.json               |  5 +++-
 tests/qapi-schema/doc-good.out                |  3 +++
 tests/qapi-schema/doc-good.txt                |  3 +++
 .../qapi-schema/enum-dict-member-unknown.err  |  2 +-
 tests/qapi-schema/qapi-schema-test.json       |  3 ++-
 tests/qapi-schema/qapi-schema-test.out        |  1 +
 tests/qapi-schema/test-qapi.py                |  1 +
 24 files changed, 144 insertions(+), 55 deletions(-)

-- 
2.31.1

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..  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.

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.

> 
> PATCH 5 puts the new feature flags to use.  It makes sense only on top
> of Vladimir's deprecation of drive-backup.  See its commit message for
> a reference.
> 
> Based on my "[PATCH 00/22] qapi: Remove simple unions from the schema
> language".
> 
> Based-on: Message-Id: <20210913123932.3306639-1-armbru@redhat.com>
> 
> Markus Armbruster (5):
>    qapi: Enable enum member introspection to show more than name
>    qapi: Add feature flags to enum members
>    qapi: Move compat policy from QObject to generic visitor
>    qapi: Implement deprecated-input={reject,crash} for enum values
>    block: Deprecate transaction type drive-backup
> 
>   docs/devel/qapi-code-gen.rst                  |  4 ++-
>   qapi/compat.json                              |  3 +++
>   qapi/introspect.json                          | 23 ++++++++++++++--
>   qapi/transaction.json                         |  5 +++-
>   include/qapi/qobject-input-visitor.h          |  4 ---
>   include/qapi/qobject-output-visitor.h         |  4 ---
>   include/qapi/util.h                           |  6 ++++-
>   include/qapi/visitor-impl.h                   |  3 +++
>   include/qapi/visitor.h                        |  9 +++++++
>   qapi/qapi-visit-core.c                        | 27 ++++++++++++++++---
>   qapi/qmp-dispatch.c                           |  4 +--
>   qapi/qobject-input-visitor.c                  | 14 +---------
>   qapi/qobject-output-visitor.c                 | 14 +---------
>   scripts/qapi/expr.py                          |  3 ++-
>   scripts/qapi/introspect.py                    | 19 ++++++++++---
>   scripts/qapi/schema.py                        | 22 +++++++++++++--
>   scripts/qapi/types.py                         | 17 +++++++++++-
>   tests/qapi-schema/doc-good.json               |  5 +++-
>   tests/qapi-schema/doc-good.out                |  3 +++
>   tests/qapi-schema/doc-good.txt                |  3 +++
>   .../qapi-schema/enum-dict-member-unknown.err  |  2 +-
>   tests/qapi-schema/qapi-schema-test.json       |  3 ++-
>   tests/qapi-schema/qapi-schema-test.out        |  1 +
>   tests/qapi-schema/test-qapi.py                |  1 +
>   24 files changed, 144 insertions(+), 55 deletions(-)
> 


-- 
Best regards,
Vladimir

Vladimir Sementsov-Ogievskiy <vsementsov@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.

> 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.

16.09.2021 15:57, Markus Armbruster wrote:
> Vladimir Sementsov-Ogievskiy <vsementsov@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

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

> 16.09.2021 15:57, Markus Armbruster wrote:
>> Vladimir Sementsov-Ogievskiy <vsementsov@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.

We need to distinguish between three separate things:

(1) Deprecating certain interface usage

(2) QAPI feature flag 'deprecated'

(3) Policy for handling deprecated interface usage

Note that (2) cannot cover all of (1) for two reasons, only one of them
fixable:

* Some interfaces still aren't QAPI-based.  QAPIfying them all is hard.

* Feature flags only apply to *syntactic* elements, such as a command
  argument.

  Example for a deprecation where they don't apply: we deprecated use of
  chardev-add argument "wait" together with "server": false in v4.0
  (it's an error since v6.0).  Use without "server": false remains okay.

Note further that (3) may cover both less and more than (2).

Before this series, it covers exactly (2).  Afterwards, there is a hole:

    # Limitation: deprecated-output policy @hide is not implemented for
    # enumeration values.  They behave the same as with policy @accept.

But it could also go beyond (2) in the future:

    # Limitation: covers only syntactic aspects of QMP, i.e. stuff tagged
    # with feature 'deprecated'.  We may want to extend it to cover
    # semantic aspects, CLI, and experimental features.

> 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?

Hmm, interesting idea.  Needs to be documented, obviously.

[...]