Re: [PATCH] qapi: merge common parts of NbdServerOptions and nbd-server-start data

[Markus Armbruster <armbru@redhat.com>]

Eric Blake <eblake@redhat.com> writes:

> On Wed, Feb 12, 2025 at 05:33:51PM +0300, Vladimir Sementsov-Ogievskiy wrote:
>> Instead of comment
>> "Keep this type consistent with the nbd-server-start arguments", we
>> can simply merge these things.
>> 
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
>> ---
>> 
>> No problem for me to rebase on top of
>> [PATCH 0/2] nbd: Allow debugging tuning of handshake limit
>> if it goes earlier.
>
> I just sent the pull request for that, so this will indeed need
> rebasing.  But it's still worth reviewing as written.
>
>> 
>> Also, not that order of nbd-server-start is changed. I think the order

s/not/note/, I presume.

>> could not be a contract of JSON interface.
>
> Correct - QMP does not mandate wire ordering, so although the change
> causes C paramter rearrangement, it is not a breaking change.

Correct.

>> We could instead of making common base structure go another way
>> and define two structures with same data=NbdServerOptionsCommon, and
>> different bases (will have to define additional base strucutres with
>> SocketAddress and SocketAddressLegacy fields). But it would look a bit
>> unusual in QAPI.
>> 
>>  blockdev-nbd.c         |  4 +--
>>  qapi/block-export.json | 57 ++++++++++++++++++++----------------------
>>  2 files changed, 29 insertions(+), 32 deletions(-)
>> 
>> diff --git a/blockdev-nbd.c b/blockdev-nbd.c
>> index 9e61fbaf2b..b0b8993a1b 100644
>> --- a/blockdev-nbd.c
>> +++ b/blockdev-nbd.c
>> @@ -215,10 +215,10 @@ void nbd_server_start_options(NbdServerOptions *arg, Error **errp)
>>                       arg->max_connections, errp);
>>  }
>>  
>> -void qmp_nbd_server_start(SocketAddressLegacy *addr,
>> -                          const char *tls_creds,
>> +void qmp_nbd_server_start(const char *tls_creds,
>>                            const char *tls_authz,
>>                            bool has_max_connections, uint32_t max_connections,
>> +                          SocketAddressLegacy *addr,
>>                            Error **errp)
>>  {
>>      SocketAddress *addr_flat = socket_address_flatten(addr);
>> diff --git a/qapi/block-export.json b/qapi/block-export.json
>> index 117b05d13c..5eb94213db 100644
>> --- a/qapi/block-export.json
>> +++ b/qapi/block-export.json
>> @@ -9,13 +9,7 @@
>>  { 'include': 'block-core.json' }
>>  
>>  ##
>> -# @NbdServerOptions:
>> -#
>> -# Keep this type consistent with the nbd-server-start arguments.  The
>> -# only intended difference is using SocketAddress instead of
>> -# SocketAddressLegacy.
>> -#
>> -# @addr: Address on which to listen.
>> +# @NbdServerOptionsBase:
>>  #
>>  # @tls-creds: ID of the TLS credentials object (since 2.6).
>>  #
>> @@ -30,14 +24,35 @@
>>  #     server from advertising multiple client support (since 5.2;
>>  #     default: 100)
>>  #
>> -# Since: 4.2
>> +# Since: 10.0
>
> Markus, when refactoring types like this, should Since stay at the
> point in time where the fields were first introduced (4.2) or at the
> time where the refactoring introduced the new type (10.0)?  Or does
> type inlining make it all moot if this tag never shows up in the docs?

To answer this, we need to consider what doc comments are for:
documenting our external QMP interface for its users.

Types are not part of this QMP interface, only their members can be,
namely as arguments of commands and events, possibly nested into other
arguments.  Types can change in ways that do not affect the interface.
Like in this patch.

Since only members are part of the interface, only members need since
information.

Having to write down since information member by member all the time
would be onerous, so we provide a shorthand: write it down for the type,
and any members without explicit since use that.

This leads us to the answer to your question.  When you refactor types,
take care to preserve the interface documentation's since information,
just like you preserve the actual interface.

Much of the time, this is as trivial as "don't mess with since".

Sometimes, it involves things like adding a new type with an old since.

Occasionally, there's a conflict.  Say you unify two similar types into
one, a perfectly sane refactoring.  But the two types can have
conflicting since information.  Perhaps one type's member @foo goes back
to 4.2, and the other's goes back only to 9.1.  We don't have good
solutions for that.  We do our best in prose.

Maintaining accurate since information is bothersome and error prone.
It should be done by the machine, not us.  John and I have pondered ways
to do this, but we have other projects to finish first.

Questions?

[...]