Re: [PATCH 2/2] nbd/server: Allow users to adjust handshake limit in QMP

[Markus Armbruster]

Eric Blake <eblake@redhat.com> writes:

> Although defaulting the handshake limit to 10 seconds was a nice QoI
> change to weed out intentionally slow clients, it can interfere with
> integration testing done with manual NBD_OPT commands over 'nbdsh
> --opt-mode'.  Expose a QMP knob 'handshake-max-secs' to allow the user
> to alter the timeout away from the default.
>
> The parameter name here intentionally matches the spelling of the
> constant added in commit fb1c2aaa98, and not the command-line spelling
> added in the previous patch for qemu-nbd; that's because in QMP,
> longer names serve as good self-documentation, and unlike the command
> line, machines don't have problems generating longer spellings.
>
> Signed-off-by: Eric Blake <eblake@redhat.com>
> ---
>  qapi/block-export.json         | 10 ++++++++++
>  include/block/nbd.h            |  6 +++---
>  block/monitor/block-hmp-cmds.c |  4 ++--
>  blockdev-nbd.c                 | 26 ++++++++++++++++++--------
>  4 files changed, 33 insertions(+), 13 deletions(-)
>
> diff --git a/qapi/block-export.json b/qapi/block-export.json
> index ce33fe378df..58ae6a5e1d7 100644
> --- a/qapi/block-export.json
> +++ b/qapi/block-export.json
> @@ -17,6 +17,10 @@
>  #
>  # @addr: Address on which to listen.
>  #
> +# @handshake-max-secs: Time limit, in seconds, at which a client that
> +#     has not completed the negotiation handshake will be disconnected,
> +#     or 0 for no limit (since 10.0; default: 10).
> +#
>  # @tls-creds: ID of the TLS credentials object (since 2.6).
>  #
>  # @tls-authz: ID of the QAuthZ authorization object used to validate
> @@ -34,6 +38,7 @@
>  ##
>  { 'struct': 'NbdServerOptions',
>    'data': { 'addr': 'SocketAddress',
> +            '*handshake-max-secs': 'uint32',
>              '*tls-creds': 'str',
>              '*tls-authz': 'str',
>              '*max-connections': 'uint32' } }

Standard question on time: are we confident the granularity will
suffice?

On naming...  We use "seconds" (StatsUnit in qapi/stats.json), and "sec"
(SnapshotInfo in qapi/block-core.json), but not "secs".  Do we care?

> @@ -52,6 +57,10 @@
>  #
>  # @addr: Address on which to listen.
>  #
> +# @handshake-max-secs: Time limit, in seconds, at which a client that
> +#     has not completed the negotiation handshake will be disconnected,
> +#     or 0 for no limit (since 10.0; default: 10).
> +#
>  # @tls-creds: ID of the TLS credentials object (since 2.6).
>  #
>  # @tls-authz: ID of the QAuthZ authorization object used to validate
> @@ -72,6 +81,7 @@
>  ##
>  { 'command': 'nbd-server-start',
>    'data': { 'addr': 'SocketAddressLegacy',
> +            '*handshake-max-secs': 'uint32',
>              '*tls-creds': 'str',
>              '*tls-authz': 'str',
>              '*max-connections': 'uint32' },

[...]