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.0Eric 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?
[...]
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
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.
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
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
© 2016 - 2024 Red Hat, Inc.