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 <vsement...@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
> 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.
>
> 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?
> ##
> -{ 'struct': 'NbdServerOptions',
> - 'data': { 'addr': 'SocketAddress',
> - '*tls-creds': 'str',
> +{ 'struct': 'NbdServerOptionsBase',
> + 'data': { '*tls-creds': 'str',
> '*tls-authz': 'str',
> '*max-connections': 'uint32' } }
>
> +##
> +# @NbdServerOptions:
> +#
> +# @addr: Address on which to listen.
This might be a good time to add a paragraph on how this is the struct
used for q-s-d command-line server setup...
> +#
> +# Since: 10.0
> +##
> +{ 'struct': 'NbdServerOptions',
> + 'base': 'NbdServerOptionsBase',
> + 'data': { 'addr': 'SocketAddress' } }
> +
> +##
> +# @NbdServerOptionsLegacy:
> +#
> +# @addr: Address on which to listen.
...while this is the version for QMP runtime control.
> +#
> +# Since: 10.0
> +##
> +{ 'struct': 'NbdServerOptionsLegacy',
> + 'base': 'NbdServerOptionsBase',
> + 'data': { 'addr': 'SocketAddressLegacy' } }
> +
> ##
> # @nbd-server-start:
> #
> @@ -50,31 +65,13 @@
> # intended difference is using SocketAddressLegacy instead of
> # SocketAddress.
> #
> -# @addr: Address on which to listen.
> -#
> -# @tls-creds: ID of the TLS credentials object (since 2.6).
> -#
> -# @tls-authz: ID of the QAuthZ authorization object used to validate
> -# the client's x509 distinguished name. This object is is only
> -# resolved at time of use, so can be deleted and recreated on the
> -# fly while the NBD server is active. If missing, it will default
> -# to denying access (since 4.0).
> -#
> -# @max-connections: The maximum number of connections to allow at the
> -# same time, 0 for unlimited. Setting this to 1 also stops the
> -# server from advertising multiple client support (since 5.2;
> -# default: 100).
> -#
> # Errors:
> # - if the server is already running
> #
> # Since: 1.3
> ##
> { 'command': 'nbd-server-start',
> - 'data': { 'addr': 'SocketAddressLegacy',
> - '*tls-creds': 'str',
> - '*tls-authz': 'str',
> - '*max-connections': 'uint32' },
> + 'data': 'NbdServerOptionsLegacy',
> 'allow-preconfig': true }
At any rate, I like the consolidation.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc.
Virtualization: qemu.org | libguestfs.org
