Re: [PATCH 2/9] qapi: Mark unstable QMP parts with feature 'unstable'

[Markus Armbruster]

John Snow <jsnow@redhat.com> writes:

> On Mon, Oct 25, 2021 at 1:25 AM Markus Armbruster <armbru@redhat.com> wrote:
>
>> Add special feature 'unstable' everywhere the name starts with 'x-',
>> except for InputBarrierProperties member x-origin and
>> MemoryBackendProperties member x-use-canonical-path-for-ramblock-id,
>> because these two are actually stable.
>>
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>  qapi/block-core.json | 123 +++++++++++++++++++++++++++++++------------
>>  qapi/migration.json  |  35 +++++++++---
>>  qapi/misc.json       |   6 ++-
>>  qapi/qom.json        |  11 ++--
>>  4 files changed, 130 insertions(+), 45 deletions(-)
>>
>> diff --git a/qapi/block-core.json b/qapi/block-core.json
>> index 6d3217abb6..ce2c1352cb 100644
>> --- a/qapi/block-core.json
>> +++ b/qapi/block-core.json
>> @@ -1438,6 +1438,9 @@
>>  #
>>  # @x-perf: Performance options. (Since 6.0)
>>  #
>> +# Features:
>> +# @unstable: Member @x-perf is experimental.
>> +#
>>
>
> It'd be a lot cooler if we could annotate the unstable member directly
> instead of confusing it with the syntax that might describe the entire
> struct/union/command/etc, but ... eh, it's just a doc field, so I'm not
> gonna press on this. I don't have the energy to get into a doc formatting
> standard discussion right now, so: sure, why not?

By design, we have a single doc comment block for the entire definition.

When Kevin invented feature flags (merge commit 4747524f9f2), he added
them just to struct types.  He added "feature sections" for documenting
features.  It mirrors the "argument sections" for documenting members.
Makes sense.

Aside: he neglected to update qapi-code-gen.rst section "Definition
documentation", and I failed to catch it.  I'll make up for it.

Peter and I then added feature flags to the remaining definitions
(commit 23394b4c39 and 013b4efc9b).  Feature sections make sense there,
too.

I then added them to struct members (commit 84ab008687).  Instead of
doing something fancy for documenting feature flags of members, I simply
used the existing feature sections.  This conflates member features with
struct features.

What could "something fancy" be?  All we have for members is "argument
sections", which are basically a name plus descriptive text.  We'd have
to add structure to that, say nest feature sections into argument
sections.  I have no appetite for that right now.

>
>
>>  # Note: @on-source-error and @on-target-error only affect background
>>  #       I/O.  If an error occurs during a guest write request, the
>> device's
>>  #       rerror/werror actions will be used.
>> @@ -1452,7 +1455,9 @@
>>              '*on-source-error': 'BlockdevOnError',
>>              '*on-target-error': 'BlockdevOnError',
>>              '*auto-finalize': 'bool', '*auto-dismiss': 'bool',
>> -            '*filter-node-name': 'str', '*x-perf': 'BackupPerf'  } }
>> +            '*filter-node-name': 'str',
>> +            '*x-perf': { 'type': 'BackupPerf',
>> +                         'features': [ 'unstable' ] } } }
>>
>>  ##
>>  # @DriveBackup:

[...]

> Seems OK, but I didn't audit for false positives/negatives. Trusting your
> judgment here. (It looks like Phil started to audit this in his reply to
> your previous commit, so I'll trust that.)

I'm pretty sure the x- names that don't get feature 'unstable' are
actually stable; see my reply to Kashyap.

I did check git history for each that does get feature 'unstable'.
Double-checking is of course welcome.

> Acked-by: John Snow <jsnow@redhat.com>

Thanks!