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

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

[Eric Blake]

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..c110dd375ad 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 9.2; 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' } }
@@ -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 9.2; 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' },
diff --git a/include/block/nbd.h b/include/block/nbd.h
index d4f8b21aecc..92987c76fd6 100644
--- a/include/block/nbd.h
+++ b/include/block/nbd.h
@@ -428,9 +428,9 @@ void nbd_client_put(NBDClient *client);
 void nbd_server_is_qemu_nbd(int max_connections);
 bool nbd_server_is_running(void);
 int nbd_server_max_connections(void);
-void nbd_server_start(SocketAddress *addr, const char *tls_creds,
-                      const char *tls_authz, uint32_t max_connections,
-                      Error **errp);
+void nbd_server_start(SocketAddress *addr, uint32_t handshake_max_secs,
+                      const char *tls_creds, const char *tls_authz,
+                      uint32_t max_connections, Error **errp);
 void nbd_server_start_options(NbdServerOptions *arg, Error **errp);

 /* nbd_read
diff --git a/block/monitor/block-hmp-cmds.c b/block/monitor/block-hmp-cmds.c
index bdf2eb50b68..cae6e3064a0 100644
--- a/block/monitor/block-hmp-cmds.c
+++ b/block/monitor/block-hmp-cmds.c
@@ -402,8 +402,8 @@ void hmp_nbd_server_start(Monitor *mon, const QDict *qdict)
         goto exit;
     }

-    nbd_server_start(addr, NULL, NULL, NBD_DEFAULT_MAX_CONNECTIONS,
-                     &local_err);
+    nbd_server_start(addr, NBD_DEFAULT_HANDSHAKE_MAX_SECS, NULL, NULL,
+                     NBD_DEFAULT_MAX_CONNECTIONS, &local_err);
     qapi_free_SocketAddress(addr);
     if (local_err != NULL) {
         goto exit;
diff --git a/blockdev-nbd.c b/blockdev-nbd.c
index f73409ae494..0cdabfbd82c 100644
--- a/blockdev-nbd.c
+++ b/blockdev-nbd.c
@@ -28,6 +28,7 @@ typedef struct NBDConn {

 typedef struct NBDServerData {
     QIONetListener *listener;
+    uint32_t handshake_max_secs;
     QCryptoTLSCreds *tlscreds;
     char *tlsauthz;
     uint32_t max_connections;
@@ -84,8 +85,7 @@ static void nbd_accept(QIONetListener *listener, QIOChannelSocket *cioc,
     nbd_update_server_watch(nbd_server);

     qio_channel_set_name(QIO_CHANNEL(cioc), "nbd-server");
-    /* TODO - expose handshake timeout as QMP option */
-    nbd_client_new(cioc, NBD_DEFAULT_HANDSHAKE_MAX_SECS,
+    nbd_client_new(cioc, nbd_server->handshake_max_secs,
                    nbd_server->tlscreds, nbd_server->tlsauthz,
                    nbd_blockdev_client_closed, conn);
 }
@@ -158,9 +158,9 @@ static QCryptoTLSCreds *nbd_get_tls_creds(const char *id, Error **errp)
 }


-void nbd_server_start(SocketAddress *addr, const char *tls_creds,
-                      const char *tls_authz, uint32_t max_connections,
-                      Error **errp)
+void nbd_server_start(SocketAddress *addr, uint32_t handshake_max_secs,
+                      const char *tls_creds, const char *tls_authz,
+                      uint32_t max_connections, Error **errp)
 {
     if (nbd_server) {
         error_setg(errp, "NBD server already running");
@@ -169,6 +169,7 @@ void nbd_server_start(SocketAddress *addr, const char *tls_creds,

     nbd_server = g_new0(NBDServerData, 1);
     nbd_server->max_connections = max_connections;
+    nbd_server->handshake_max_secs = handshake_max_secs;
     nbd_server->listener = qio_net_listener_new();

     qio_net_listener_set_name(nbd_server->listener,
@@ -206,12 +207,17 @@ void nbd_server_start_options(NbdServerOptions *arg, Error **errp)
     if (!arg->has_max_connections) {
         arg->max_connections = NBD_DEFAULT_MAX_CONNECTIONS;
     }
+    if (!arg->has_handshake_max_secs) {
+        arg->handshake_max_secs = NBD_DEFAULT_HANDSHAKE_MAX_SECS;
+    }

-    nbd_server_start(arg->addr, arg->tls_creds, arg->tls_authz,
-                     arg->max_connections, errp);
+    nbd_server_start(arg->addr, arg->handshake_max_secs, arg->tls_creds,
+                     arg->tls_authz, arg->max_connections, errp);
 }

 void qmp_nbd_server_start(SocketAddressLegacy *addr,
+                          bool has_handshake_max_secs,
+                          uint32_t handshake_max_secs,
                           const char *tls_creds,
                           const char *tls_authz,
                           bool has_max_connections, uint32_t max_connections,
@@ -222,8 +228,12 @@ void qmp_nbd_server_start(SocketAddressLegacy *addr,
     if (!has_max_connections) {
         max_connections = NBD_DEFAULT_MAX_CONNECTIONS;
     }
+    if (!has_handshake_max_secs) {
+        handshake_max_secs = NBD_DEFAULT_HANDSHAKE_MAX_SECS;
+    }

-    nbd_server_start(addr_flat, tls_creds, tls_authz, max_connections, errp);
+    nbd_server_start(addr_flat, handshake_max_secs, tls_creds, tls_authz,
+                     max_connections, errp);
     qapi_free_SocketAddress(addr_flat);
 }

-- 
2.46.0

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

[Vladimir Sementsov-Ogievskiy]

On 09.08.24 19:14, Eric Blake wrote:
> 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>


Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>

> ---
>   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..c110dd375ad 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 9.2; 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' } }
> @@ -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 9.2; 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' },

Hmm, can we make common base for these two structures, to avoid adding things always in two places? At some point would be good to somehow deprecate old syntax and finally remove it. SocketAddressLegacy is marked as deprecated in comment in qapi/sockets.json, but no word in deprecated.rst, and I'm unsure how is it possible to deprecate this.. May be, the only way is introducing new command, and deprecate nbd-server-start altogether. Aha, that should happen when add a possibility to start multiple nbd servers.

-- 
Best regards,
Vladimir

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..c110dd375ad 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 9.2; 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' } }
> @@ -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 9.2; 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' },

Are we confident we'll never need less than a full second?

[...]

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

[Vladimir Sementsov-Ogievskiy]

On 02.10.24 16:49, Markus Armbruster wrote:
> 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..c110dd375ad 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 9.2; 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' } }
>> @@ -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 9.2; 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' },
> 
> Are we confident we'll never need less than a full second?
> 

Hmm, recent "[PATCH v2] chardev: introduce 'reconnect-ms' and deprecate 'reconnect'" shows that at least sometimes second is not enough precision.

Maybe, using milliseconds consistently for all relatively short time intervals in QAPI would be a good rule?

-- 
Best regards,
Vladimir

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

[Markus Armbruster]

Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> writes:

> On 02.10.24 16:49, Markus Armbruster wrote:
>> 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>

[...]

>> Are we confident we'll never need less than a full second?
>
> Hmm, recent "[PATCH v2] chardev: introduce 'reconnect-ms' and deprecate 'reconnect'" shows that at least sometimes second is not enough precision.
>
> Maybe, using milliseconds consistently for all relatively short time intervals in QAPI would be a good rule?

Ideally, we'd use a single unit for time: nanoseconds.  But we missed
that chance long ago, and now are stuck with a mix of seconds,
milliseconds, microseconds, and nanoseconds.

I think a good rule is to pick the first from this list that will surely
provide all the precision we'll ever need.

In this case, milliseconds should do.

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

[Eric Blake]

On Wed, Oct 02, 2024 at 05:14:42PM GMT, Markus Armbruster wrote:
> >> Are we confident we'll never need less than a full second?
> >
> > Hmm, recent "[PATCH v2] chardev: introduce 'reconnect-ms' and deprecate 'reconnect'" shows that at least sometimes second is not enough precision.
> >
> > Maybe, using milliseconds consistently for all relatively short time intervals in QAPI would be a good rule?
> 
> Ideally, we'd use a single unit for time: nanoseconds.  But we missed
> that chance long ago, and now are stuck with a mix of seconds,
> milliseconds, microseconds, and nanoseconds.
> 
> I think a good rule is to pick the first from this list that will surely
> provide all the precision we'll ever need.
> 
> In this case, milliseconds should do.

I'll use milliseconds in the next revision.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.
Virtualization:  qemu.org | libguestfs.org

[PATCH 0/2] nbd: Allow debugging tuning of handshake limit

[Eric Blake]

Reviving a patch that has been sitting in my tree for a while.  It's
mostly useful for low-level integration testing (such as debugging
libnbd as an NBD client).

Eric Blake (2):
  qemu-nbd: Allow users to adjust handshake limit
  nbd/server: Allow users to adjust handshake limit in QMP

 docs/tools/qemu-nbd.rst        |  5 +++++
 qapi/block-export.json         | 10 +++++++++
 include/block/nbd.h            |  6 ++---
 block/monitor/block-hmp-cmds.c |  4 ++--
 blockdev-nbd.c                 | 26 ++++++++++++++-------
 qemu-nbd.c                     | 41 +++++++++++++++++++++-------------
 6 files changed, 64 insertions(+), 28 deletions(-)

-- 
2.48.1

[PATCH 1/2] qemu-nbd: Allow users to adjust handshake limit

[Eric Blake]

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 command line option to allow the user to alter
the timeout away from the default.  This option is unlikely to be used
in enough scenarios to warrant a short option letter.

The option --handshake-limit intentionally differs from the name of
the constant added in commit fb1c2aaa98 (limit instead of max_secs)
and the QMP name to be added in the next commit; this is because
typing a longer command-line name is undesirable and there is
sufficient --help text to document the units.

Signed-off-by: Eric Blake <eblake@redhat.com>
---
 docs/tools/qemu-nbd.rst |  5 +++++
 qemu-nbd.c              | 41 ++++++++++++++++++++++++++---------------
 2 files changed, 31 insertions(+), 15 deletions(-)

diff --git a/docs/tools/qemu-nbd.rst b/docs/tools/qemu-nbd.rst
index 4f21b7904ac..f82ea5fd77b 100644
--- a/docs/tools/qemu-nbd.rst
+++ b/docs/tools/qemu-nbd.rst
@@ -156,6 +156,11 @@ driver options if :option:`--image-opts` is specified.
   Set the NBD volume export description, as a human-readable
   string.

+.. option:: --handshake-limit=N
+
+  Set the timeout for a client to successfully complete its handshake
+  to N seconds (default 10), or 0 for no limit.
+
 .. option:: -L, --list

   Connect as a client and list all details about the exports exposed by
diff --git a/qemu-nbd.c b/qemu-nbd.c
index e7a961a5562..2eb32bc700c 100644
--- a/qemu-nbd.c
+++ b/qemu-nbd.c
@@ -57,19 +57,20 @@
 #define HAVE_NBD_DEVICE 0
 #endif

-#define SOCKET_PATH                "/var/lock/qemu-nbd-%s"
-#define QEMU_NBD_OPT_CACHE         256
-#define QEMU_NBD_OPT_AIO           257
-#define QEMU_NBD_OPT_DISCARD       258
-#define QEMU_NBD_OPT_DETECT_ZEROES 259
-#define QEMU_NBD_OPT_OBJECT        260
-#define QEMU_NBD_OPT_TLSCREDS      261
-#define QEMU_NBD_OPT_IMAGE_OPTS    262
-#define QEMU_NBD_OPT_FORK          263
-#define QEMU_NBD_OPT_TLSAUTHZ      264
-#define QEMU_NBD_OPT_PID_FILE      265
-#define QEMU_NBD_OPT_SELINUX_LABEL 266
-#define QEMU_NBD_OPT_TLSHOSTNAME   267
+#define SOCKET_PATH                  "/var/lock/qemu-nbd-%s"
+#define QEMU_NBD_OPT_CACHE           256
+#define QEMU_NBD_OPT_AIO             257
+#define QEMU_NBD_OPT_DISCARD         258
+#define QEMU_NBD_OPT_DETECT_ZEROES   259
+#define QEMU_NBD_OPT_OBJECT          260
+#define QEMU_NBD_OPT_TLSCREDS        261
+#define QEMU_NBD_OPT_IMAGE_OPTS      262
+#define QEMU_NBD_OPT_FORK            263
+#define QEMU_NBD_OPT_TLSAUTHZ        264
+#define QEMU_NBD_OPT_PID_FILE        265
+#define QEMU_NBD_OPT_SELINUX_LABEL   266
+#define QEMU_NBD_OPT_TLSHOSTNAME     267
+#define QEMU_NBD_OPT_HANDSHAKE_LIMIT 268

 #define MBR_SIZE 512

@@ -80,6 +81,7 @@ static int nb_fds;
 static QIONetListener *server;
 static QCryptoTLSCreds *tlscreds;
 static const char *tlsauthz;
+static int handshake_limit = NBD_DEFAULT_HANDSHAKE_MAX_SECS;

 static void usage(const char *name)
 {
@@ -101,6 +103,7 @@ static void usage(const char *name)
 "  -v, --verbose             display extra debugging information\n"
 "  -x, --export-name=NAME    expose export by name (default is empty string)\n"
 "  -D, --description=TEXT    export a human-readable description\n"
+"      --handshake-limit=N   limit client's handshake to N seconds (default 10)\n"
 "\n"
 "Exposing part of the image:\n"
 "  -o, --offset=OFFSET       offset into the image\n"
@@ -390,8 +393,7 @@ static void nbd_accept(QIONetListener *listener, QIOChannelSocket *cioc,

     nb_fds++;
     nbd_update_server_watch();
-    /* TODO - expose handshake timeout as command line option */
-    nbd_client_new(cioc, NBD_DEFAULT_HANDSHAKE_MAX_SECS,
+    nbd_client_new(cioc, handshake_limit,
                    tlscreds, tlsauthz, nbd_client_closed, NULL);
 }

@@ -569,6 +571,8 @@ int main(int argc, char **argv)
         { "object", required_argument, NULL, QEMU_NBD_OPT_OBJECT },
         { "export-name", required_argument, NULL, 'x' },
         { "description", required_argument, NULL, 'D' },
+        { "handshake-limit", required_argument, NULL,
+          QEMU_NBD_OPT_HANDSHAKE_LIMIT },
         { "tls-creds", required_argument, NULL, QEMU_NBD_OPT_TLSCREDS },
         { "tls-hostname", required_argument, NULL, QEMU_NBD_OPT_TLSHOSTNAME },
         { "tls-authz", required_argument, NULL, QEMU_NBD_OPT_TLSAUTHZ },
@@ -815,6 +819,13 @@ int main(int argc, char **argv)
         case QEMU_NBD_OPT_SELINUX_LABEL:
             selinux_label = optarg;
             break;
+        case QEMU_NBD_OPT_HANDSHAKE_LIMIT:
+            if (qemu_strtoi(optarg, NULL, 0, &handshake_limit) < 0 ||
+                handshake_limit < 0) {
+                error_report("Invalid handshake limit '%s'", optarg);
+                exit(EXIT_FAILURE);
+            }
+            break;
         }
     }

-- 
2.48.1

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

[Eric Blake]

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' } }
@@ -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' },
diff --git a/include/block/nbd.h b/include/block/nbd.h
index d4f8b21aecc..92987c76fd6 100644
--- a/include/block/nbd.h
+++ b/include/block/nbd.h
@@ -428,9 +428,9 @@ void nbd_client_put(NBDClient *client);
 void nbd_server_is_qemu_nbd(int max_connections);
 bool nbd_server_is_running(void);
 int nbd_server_max_connections(void);
-void nbd_server_start(SocketAddress *addr, const char *tls_creds,
-                      const char *tls_authz, uint32_t max_connections,
-                      Error **errp);
+void nbd_server_start(SocketAddress *addr, uint32_t handshake_max_secs,
+                      const char *tls_creds, const char *tls_authz,
+                      uint32_t max_connections, Error **errp);
 void nbd_server_start_options(NbdServerOptions *arg, Error **errp);

 /* nbd_read
diff --git a/block/monitor/block-hmp-cmds.c b/block/monitor/block-hmp-cmds.c
index 1d312513fc4..0cfcbfe7c21 100644
--- a/block/monitor/block-hmp-cmds.c
+++ b/block/monitor/block-hmp-cmds.c
@@ -402,8 +402,8 @@ void hmp_nbd_server_start(Monitor *mon, const QDict *qdict)
         goto exit;
     }

-    nbd_server_start(addr, NULL, NULL, NBD_DEFAULT_MAX_CONNECTIONS,
-                     &local_err);
+    nbd_server_start(addr, NBD_DEFAULT_HANDSHAKE_MAX_SECS, NULL, NULL,
+                     NBD_DEFAULT_MAX_CONNECTIONS, &local_err);
     qapi_free_SocketAddress(addr);
     if (local_err != NULL) {
         goto exit;
diff --git a/blockdev-nbd.c b/blockdev-nbd.c
index 9e61fbaf2b2..e9f53e83d48 100644
--- a/blockdev-nbd.c
+++ b/blockdev-nbd.c
@@ -28,6 +28,7 @@ typedef struct NBDConn {

 typedef struct NBDServerData {
     QIONetListener *listener;
+    uint32_t handshake_max_secs;
     QCryptoTLSCreds *tlscreds;
     char *tlsauthz;
     uint32_t max_connections;
@@ -84,8 +85,7 @@ static void nbd_accept(QIONetListener *listener, QIOChannelSocket *cioc,
     nbd_update_server_watch(nbd_server);

     qio_channel_set_name(QIO_CHANNEL(cioc), "nbd-server");
-    /* TODO - expose handshake timeout as QMP option */
-    nbd_client_new(cioc, NBD_DEFAULT_HANDSHAKE_MAX_SECS,
+    nbd_client_new(cioc, nbd_server->handshake_max_secs,
                    nbd_server->tlscreds, nbd_server->tlsauthz,
                    nbd_blockdev_client_closed, conn);
 }
@@ -162,9 +162,9 @@ static QCryptoTLSCreds *nbd_get_tls_creds(const char *id, Error **errp)
 }


-void nbd_server_start(SocketAddress *addr, const char *tls_creds,
-                      const char *tls_authz, uint32_t max_connections,
-                      Error **errp)
+void nbd_server_start(SocketAddress *addr, uint32_t handshake_max_secs,
+                      const char *tls_creds, const char *tls_authz,
+                      uint32_t max_connections, Error **errp)
 {
     if (nbd_server) {
         error_setg(errp, "NBD server already running");
@@ -173,6 +173,7 @@ void nbd_server_start(SocketAddress *addr, const char *tls_creds,

     nbd_server = g_new0(NBDServerData, 1);
     nbd_server->max_connections = max_connections;
+    nbd_server->handshake_max_secs = handshake_max_secs;
     nbd_server->listener = qio_net_listener_new();

     qio_net_listener_set_name(nbd_server->listener,
@@ -210,12 +211,17 @@ void nbd_server_start_options(NbdServerOptions *arg, Error **errp)
     if (!arg->has_max_connections) {
         arg->max_connections = NBD_DEFAULT_MAX_CONNECTIONS;
     }
+    if (!arg->has_handshake_max_secs) {
+        arg->handshake_max_secs = NBD_DEFAULT_HANDSHAKE_MAX_SECS;
+    }

-    nbd_server_start(arg->addr, arg->tls_creds, arg->tls_authz,
-                     arg->max_connections, errp);
+    nbd_server_start(arg->addr, arg->handshake_max_secs, arg->tls_creds,
+                     arg->tls_authz, arg->max_connections, errp);
 }

 void qmp_nbd_server_start(SocketAddressLegacy *addr,
+                          bool has_handshake_max_secs,
+                          uint32_t handshake_max_secs,
                           const char *tls_creds,
                           const char *tls_authz,
                           bool has_max_connections, uint32_t max_connections,
@@ -226,8 +232,12 @@ void qmp_nbd_server_start(SocketAddressLegacy *addr,
     if (!has_max_connections) {
         max_connections = NBD_DEFAULT_MAX_CONNECTIONS;
     }
+    if (!has_handshake_max_secs) {
+        handshake_max_secs = NBD_DEFAULT_HANDSHAKE_MAX_SECS;
+    }

-    nbd_server_start(addr_flat, tls_creds, tls_authz, max_connections, errp);
+    nbd_server_start(addr_flat, handshake_max_secs, tls_creds, tls_authz,
+                     max_connections, errp);
     qapi_free_SocketAddress(addr_flat);
 }

-- 
2.48.1

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' },

[...]

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

[Eric Blake]

On Wed, Feb 05, 2025 at 07:55:56AM +0100, Markus Armbruster wrote:
> 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.
> >

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

The default if this is unspecified is 10 seconds.  Anything subsecond
requires a fast negotiation between client and server; the more likely
use of this parameter is setting it extremely large (or to 0 to
disable) in order to allow lengthy gdb sessions without the timeout
cutting things short.  So I think seconds is okay.

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

Avoiding abbreviations and using 'seconds' is fine by me, especially
since this won't be typed by many users but remains more of a tool for
use in a debugging arsenel.  I can respin with the longer name, but
will wait for any other review comments first.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.
Virtualization:  qemu.org | libguestfs.org

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

[Markus Armbruster]

Eric Blake <eblake@redhat.com> writes:

> On Wed, Feb 05, 2025 at 07:55:56AM +0100, Markus Armbruster wrote:
>> 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.
>> >
>
>> >  { '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?
>
> The default if this is unspecified is 10 seconds.  Anything subsecond
> requires a fast negotiation between client and server; the more likely
> use of this parameter is setting it extremely large (or to 0 to
> disable) in order to allow lengthy gdb sessions without the timeout
> cutting things short.  So I think seconds is okay.

ACK

>> On naming...  We use "seconds" (StatsUnit in qapi/stats.json), and "sec"
>> (SnapshotInfo in qapi/block-core.json), but not "secs".  Do we care?
>
> Avoiding abbreviations and using 'seconds' is fine by me, especially
> since this won't be typed by many users but remains more of a tool for
> use in a debugging arsenel.  I can respin with the longer name, but
> will wait for any other review comments first.

I like "seconds".

Thanks!

Re: [PATCH 1/2] qemu-nbd: Allow users to adjust handshake limit

[Vladimir Sementsov-Ogievskiy]

On 04.02.25 01:26, Eric Blake wrote:
> 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 command line option to allow the user to alter
> the timeout away from the default.  This option is unlikely to be used
> in enough scenarios to warrant a short option letter.
> 
> The option --handshake-limit intentionally differs from the name of
> the constant added in commit fb1c2aaa98 (limit instead of max_secs)
> and the QMP name to be added in the next commit; this is because
> typing a longer command-line name is undesirable and there is
> sufficient --help text to document the units.
> 
> Signed-off-by: Eric Blake<eblake@redhat.com>

Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>

-- 
Best regards,
Vladimir

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

[Vladimir Sementsov-Ogievskiy]

On 04.02.25 01:26, Eric Blake wrote:
> 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>

Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>

(renaming to -seconds is, of course, OK)

> ---
>   qapi/block-export.json         | 10 ++++++++++
>   include/block/nbd.h            |  6 +++---

[..]

> @@ -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).
> +#

Hmm. [not about the series], shouldn't we finally deprecate older interface?

>   # @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',

[..]

-- 
Best regards,
Vladimir

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

[Eric Blake]

On Thu, Feb 06, 2025 at 10:20:09AM +0300, Vladimir Sementsov-Ogievskiy wrote:
> > ---
> >   qapi/block-export.json         | 10 ++++++++++
> >   include/block/nbd.h            |  6 +++---
> 
> [..]
> 
> > @@ -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).
> > +#
> 
> Hmm. [not about the series], shouldn't we finally deprecate older interface?

By older interface, you are asking about the QMP command
'nbd-server-start' as compared to struct NbdServerOptions.  But the
struct is not directly present in any QMP commands; rather, it only
appears to be used by qemu-storage-daemon as one of its command line
options that needs to set up an NBD server with a JSON-like syntax
that has less nesting than QMP nbd-server-start.  blockdev-nbd.c has
two functions [nbd_server_start_options(NbdServerOPtions *arg...)  and
qmp_nbd_server_start(args...)] that both unpack their slightly
different forms and pass them as parameters to nbd_server_start() that
is then agnostic to whether the older QMP command or newer q-s-d CLI
option was specified.

It looks like libvirt is still using QMP nbd-server-start.  If we were
to start the deprecation process for qemu 10.0, what would the new
command look like?  What would everyone be required to use by qemu
10.2?

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.
Virtualization:  qemu.org | libguestfs.org

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

[Vladimir Sementsov-Ogievskiy]

On 11.02.25 00:46, Eric Blake wrote:
> On Thu, Feb 06, 2025 at 10:20:09AM +0300, Vladimir Sementsov-Ogievskiy wrote:
>>> ---
>>>    qapi/block-export.json         | 10 ++++++++++
>>>    include/block/nbd.h            |  6 +++---
>>
>> [..]
>>
>>> @@ -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).
>>> +#
>>
>> Hmm. [not about the series], shouldn't we finally deprecate older interface?
> 
> By older interface, you are asking about the QMP command
> 'nbd-server-start' as compared to struct NbdServerOptions.  But the
> struct is not directly present in any QMP commands; rather, it only
> appears to be used by qemu-storage-daemon as one of its command line
> options that needs to set up an NBD server with a JSON-like syntax
> that has less nesting than QMP nbd-server-start.  blockdev-nbd.c has
> two functions [nbd_server_start_options(NbdServerOPtions *arg...)  and
> qmp_nbd_server_start(args...)] that both unpack their slightly
> different forms and pass them as parameters to nbd_server_start() that
> is then agnostic to whether the older QMP command or newer q-s-d CLI
> option was specified.
> 
> It looks like libvirt is still using QMP nbd-server-start.  If we were
> to start the deprecation process for qemu 10.0, what would the new
> command look like?  What would everyone be required to use by qemu
> 10.2?
> 

Oh you are right, I was inattentive.

So, probably thing to be deprecated is actually SocketAddressLegacy, which is the only difference.

And why just not move common part of the structures to common base? I'll send a patch with a try.

-- 
Best regards,
Vladimir