[PATCH 0/2] nbd: Add an unstable query for the client state

[PATCH 0/2] nbd: Add an unstable query for the client state

[Guillaume Vincke]

Based-on: <20261105164124.249395-1-guillaume.vincke@ovhcloud.com>

These changes follow the discussion on:
https://patchew.org/QEMU/20261105164124.249395-1-guillaume.vincke@ovhcloud.com/#au4tsMuhKjq62dVB@redhat.com
Even though Eric Blake suggested a query for NBD server state,
considering the nbd-stale-connection test we are trying to make
deterministic relies on qemu-nbd, which does not have QMP, this
patchset adds a command to query the NBD client state.

The created QMP command output lists the client internals. The optional
connector field is used to give the NBD client connection thread
details only when relevant, i.e. when running or when a channel is
ready to be picked up. Other fields such as in-flight or negotiated
size can easily be added at the top level while others like the
connector's last error would be added into the connector one.

Considering this command accesses the client's private state, it is
implemented in block/nbd.c where BDRVNBDState is defined.

Guillaume Vincke (2):
  nbd: Add an unstable query for the client state
  iotests: Make nbd-stale-connection test deterministic

 block/nbd.c                                   | 57 +++++++++++++
 include/block/nbd.h                           |  9 +++
 nbd/client-connection.c                       | 18 +++++
 qapi/block-core.json                          | 79 +++++++++++++++++++
 tests/qemu-iotests/tests/nbd-stale-connection | 17 ++--
 5 files changed, 175 insertions(+), 5 deletions(-)

-- 
2.43.0

[PATCH 1/2] nbd: Add an unstable query for the client state

[Guillaume Vincke]

At the moment, NBD client internals cannot be accessed. Therefore,
tests have to infer the client state from I/O results and wait with
sleeps, which is racy.

Add the x-debug-query-nbd-client QMP command that reports the state of
the client's connection and of its connector. Other fields will be
added when needed.

The next patch uses this command to make the nbd-stale-connection test
deterministic.

Signed-off-by: Guillaume Vincke <guillaume.vincke@ovhcloud.com>
---
 block/nbd.c             | 57 +++++++++++++++++++++++++++++
 include/block/nbd.h     |  9 +++++
 nbd/client-connection.c | 18 ++++++++++
 qapi/block-core.json    | 79 +++++++++++++++++++++++++++++++++++++++++
 4 files changed, 163 insertions(+)

diff --git a/block/nbd.c b/block/nbd.c
index e5e16722ba..b9f8d92e43 100644
--- a/block/nbd.c
+++ b/block/nbd.c
@@ -35,9 +35,11 @@
 #include "qemu/cutils.h"
 #include "qemu/main-loop.h"
 
+#include "qapi/qapi-commands-block-core.h"
 #include "qapi/qapi-visit-sockets.h"
 #include "qobject/qstring.h"
 #include "qapi/clone-visitor.h"
+#include "qapi/error.h"
 
 #include "block/qdict.h"
 #include "block/nbd.h"
@@ -2246,6 +2248,61 @@ static BlockDriver bdrv_nbd_unix = {
     .bdrv_detach_aio_context    = nbd_detach_aio_context,
 };
 
+XDbgNbdClient *qmp_x_debug_query_nbd_client(const char *node_name,
+                                            Error **errp)
+{
+    BlockDriverState *bs;
+    BDRVNBDState *s;
+    XDbgNbdClient *info;
+
+    bs = bdrv_find_node(node_name);
+    if (!bs) {
+        error_setg(errp, "Node '%s' not found", node_name);
+        return NULL;
+    }
+
+    if (bs->drv != &bdrv_nbd &&
+        bs->drv != &bdrv_nbd_tcp &&
+        bs->drv != &bdrv_nbd_unix) {
+        error_setg(errp, "Node '%s' is not an NBD client", node_name);
+        return NULL;
+    }
+
+    s = bs->opaque;
+    info = g_new0(XDbgNbdClient, 1);
+    WITH_QEMU_LOCK_GUARD(&s->requests_lock) {
+        switch (s->state) {
+        case NBD_CLIENT_CONNECTING_WAIT:
+            info->state = XDBG_NBD_CLIENT_STATE_CONNECTING_WAIT;
+            break;
+        case NBD_CLIENT_CONNECTING_NOWAIT:
+            info->state = XDBG_NBD_CLIENT_STATE_CONNECTING_NOWAIT;
+            break;
+        case NBD_CLIENT_CONNECTED:
+            info->state = XDBG_NBD_CLIENT_STATE_CONNECTED;
+            break;
+        case NBD_CLIENT_QUIT:
+            info->state = XDBG_NBD_CLIENT_STATE_QUIT;
+            break;
+        }
+    }
+
+    switch (nbd_client_connection_get_thread_state(s->conn)) {
+    case NBD_CLIENT_CONNECTION_THREAD_IDLE:
+        break;
+    case NBD_CLIENT_CONNECTION_THREAD_RUNNING:
+        info->connector = g_new0(XDbgNbdClientConnector, 1);
+        info->connector->state = XDBG_NBD_CLIENT_CONNECTOR_STATE_RUNNING;
+        break;
+    case NBD_CLIENT_CONNECTION_THREAD_PARKED:
+        info->connector = g_new0(XDbgNbdClientConnector, 1);
+        info->connector->state = XDBG_NBD_CLIENT_CONNECTOR_STATE_PARKED;
+        break;
+    }
+
+    return info;
+}
+
 static void bdrv_nbd_init(void)
 {
     bdrv_register(&bdrv_nbd);
diff --git a/include/block/nbd.h b/include/block/nbd.h
index ab40842da9..15ddec91ac 100644
--- a/include/block/nbd.h
+++ b/include/block/nbd.h
@@ -492,6 +492,15 @@ const char *nbd_mode_lookup(NBDMode mode);
 /* nbd/client-connection.c */
 void nbd_client_connection_enable_retry(NBDClientConnection *conn);
 
+typedef enum NBDClientConnectionThreadState {
+    NBD_CLIENT_CONNECTION_THREAD_IDLE,
+    NBD_CLIENT_CONNECTION_THREAD_RUNNING,
+    NBD_CLIENT_CONNECTION_THREAD_PARKED,
+} NBDClientConnectionThreadState;
+
+NBDClientConnectionThreadState
+nbd_client_connection_get_thread_state(NBDClientConnection *conn);
+
 NBDClientConnection *nbd_client_connection_new(const SocketAddress *saddr,
                                                bool do_negotiation,
                                                const char *export_name,
diff --git a/nbd/client-connection.c b/nbd/client-connection.c
index aec62934d1..7172ba49b3 100644
--- a/nbd/client-connection.c
+++ b/nbd/client-connection.c
@@ -78,6 +78,24 @@ void nbd_client_connection_enable_retry(NBDClientConnection *conn)
     conn->do_retry = true;
 }
 
+NBDClientConnectionThreadState
+nbd_client_connection_get_thread_state(NBDClientConnection *conn)
+{
+    NBDClientConnectionThreadState ret;
+
+    WITH_QEMU_LOCK_GUARD(&conn->mutex) {
+        if (conn->running) {
+            ret = NBD_CLIENT_CONNECTION_THREAD_RUNNING;
+        } else if (conn->sioc) {
+            ret = NBD_CLIENT_CONNECTION_THREAD_PARKED;
+        } else {
+            ret = NBD_CLIENT_CONNECTION_THREAD_IDLE;
+        }
+    }
+
+    return ret;
+}
+
 NBDClientConnection *nbd_client_connection_new(const SocketAddress *saddr,
                                                bool do_negotiation,
                                                const char *export_name,
diff --git a/qapi/block-core.json b/qapi/block-core.json
index 23c5787d99..973be06dfd 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -2217,6 +2217,85 @@
   'features': [ 'unstable' ],
   'allow-preconfig': true }
 
+##
+# @XDbgNbdClientState:
+#     Enum of NBD client state.
+#
+# @connecting-wait: No active connection.  A request waits for the
+#     connection attempt to finish, while reconnect-delay has not
+#     expired.
+#
+# @connecting-nowait: No active connection.  A request fails at once
+#     if no channel is ready.
+#
+# @connected: Client has a valid connection.
+#
+# @quit: Client will not reconnect.
+#
+# Since: 11.2
+##
+{ 'enum': 'XDbgNbdClientState',
+  'data': [ 'connecting-wait', 'connecting-nowait', 'connected', 'quit' ] }
+
+##
+# @XDbgNbdClientConnectorState:
+#     Enum of NBD client connector state.
+#
+# @running: Connection attempt is in progress.
+#
+# @parked: Connection was successful and channel is kept for the next
+#     request.
+#
+# Since: 11.2
+##
+{ 'enum': 'XDbgNbdClientConnectorState',
+  'data': [ 'running', 'parked' ] }
+
+##
+# @XDbgNbdClientConnector:
+#     The NBD connector information.
+#
+# @state: State of the connector.
+#
+# Since: 11.2
+##
+{ 'struct': 'XDbgNbdClientConnector',
+  'data': { 'state': 'XDbgNbdClientConnectorState' } }
+
+##
+# @XDbgNbdClient:
+#     The NBD connection state of the client and its connector.
+#     The two members are read independently and may not reflect a
+#     single point in time.
+#
+# @state: The client state.
+#
+# @connector: The connector details.  Only present while a connection
+#     attempt is running or its channel is parked.
+#
+# Since: 11.2
+##
+{ 'struct': 'XDbgNbdClient',
+  'data': { 'state': 'XDbgNbdClientState',
+            '*connector': 'XDbgNbdClientConnector' } }
+
+##
+# @x-debug-query-nbd-client:
+#     Get the NBD client state.
+#
+# @node-name: node name of the NBD client to query.
+#
+# Features:
+#
+# @unstable: This command is meant for debugging and testing.
+#
+# Since: 11.2
+##
+{ 'command': 'x-debug-query-nbd-client',
+  'data': { 'node-name': 'str' },
+  'returns': 'XDbgNbdClient',
+  'features': [ 'unstable' ] }
+
 ##
 # @drive-mirror:
 #     Start mirroring a block device's writes to a new destination.
-- 
2.43.0

Re: [PATCH 1/2] nbd: Add an unstable query for the client state

[Markus Armbruster]

Guillaume Vincke <guillaume.vincke@ovhcloud.com> writes:

> At the moment, NBD client internals cannot be accessed. Therefore,
> tests have to infer the client state from I/O results and wait with
> sleeps, which is racy.
>
> Add the x-debug-query-nbd-client QMP command that reports the state of
> the client's connection and of its connector. Other fields will be
> added when needed.
>
> The next patch uses this command to make the nbd-stale-connection test
> deterministic.
>
> Signed-off-by: Guillaume Vincke <guillaume.vincke@ovhcloud.com>

[...]

> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index 23c5787d99..973be06dfd 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json

Why block-core.json and not block-export.json?

> @@ -2217,6 +2217,85 @@
>    'features': [ 'unstable' ],
>    'allow-preconfig': true }
>  
> +##
> +# @XDbgNbdClientState:
> +#     Enum of NBD client state.

"An enumeration of NBD client states" for consistency with enum
descriptions elsewhere.  Or maybe simply "NBD client state".

> +#
> +# @connecting-wait: No active connection.  A request waits for the
> +#     connection attempt to finish, while reconnect-delay has not
> +#     expired.
> +#
> +# @connecting-nowait: No active connection.  A request fails at once
> +#     if no channel is ready.
> +#
> +# @connected: Client has a valid connection.

Double-checking: when we're in this state, there is a client connected.
Correct?

> +#
> +# @quit: Client will not reconnect.

What does this one mean exactly?

> +#
> +# Since: 11.2

Making 11.2 seems unlikely.

> +##
> +{ 'enum': 'XDbgNbdClientState',
> +  'data': [ 'connecting-wait', 'connecting-nowait', 'connected', 'quit' ] }
> +
> +##
> +# @XDbgNbdClientConnectorState:
> +#     Enum of NBD client connector state.

Again, "Enumeration of ... states".

> +#
> +# @running: Connection attempt is in progress.
> +#
> +# @parked: Connection was successful and channel is kept for the next
> +#     request.
> +#
> +# Since: 11.2
> +##
> +{ 'enum': 'XDbgNbdClientConnectorState',
> +  'data': [ 'running', 'parked' ] }
> +
> +##
> +# @XDbgNbdClientConnector:
> +#     The NBD connector information.
> +#
> +# @state: State of the connector.
> +#
> +# Since: 11.2
> +##
> +{ 'struct': 'XDbgNbdClientConnector',
> +  'data': { 'state': 'XDbgNbdClientConnectorState' } }
> +
> +##
> +# @XDbgNbdClient:
> +#     The NBD connection state of the client and its connector.
> +#     The two members are read independently and may not reflect a
> +#     single point in time.

That's unfortunate.  It'll do for debugging.  Could it create robustness
issues in tests?

> +#
> +# @state: The client state.
> +#
> +# @connector: The connector details.  Only present while a connection
> +#     attempt is running or its channel is parked.
> +#
> +# Since: 11.2
> +##
> +{ 'struct': 'XDbgNbdClient',
> +  'data': { 'state': 'XDbgNbdClientState',
> +            '*connector': 'XDbgNbdClientConnector' } }

How the combined state machine works is not clear to me.  Is it
documented anywhere?

> +
> +##
> +# @x-debug-query-nbd-client:
> +#     Get the NBD client state.
> +#
> +# @node-name: node name of the NBD client to query.

How would I find the valid node names?  query-block-exports?

> +#
> +# Features:
> +#
> +# @unstable: This command is meant for debugging and testing.
> +#
> +# Since: 11.2
> +##
> +{ 'command': 'x-debug-query-nbd-client',
> +  'data': { 'node-name': 'str' },
> +  'returns': 'XDbgNbdClient',
> +  'features': [ 'unstable' ] }
> +
>  ##
>  # @drive-mirror:
>  #     Start mirroring a block device's writes to a new destination.

Re: [PATCH 1/2] nbd: Add an unstable query for the client state

[guillaume vincke]

On 11/10/26 13:53, Markus Armbruster wrote:

Thanks for the review. Let me clarify before posting a v2.

> 
> Why block-core.json and not block-export.json?

If I get this right, block-export.json covers the server part. I
thought adding this next to BlockdevOptionsNbd was a better fit. I can
change that in v2 if you think block-export.json is a better match.

> 
>> +#
>> +# @connecting-wait: No active connection.  A request waits for the
>> +#     connection attempt to finish, while reconnect-delay has not
>> +#     expired.
>> +#
>> +# @connecting-nowait: No active connection.  A request fails at once
>> +#     if no channel is ready.
>> +#
>> +# @connected: Client has a valid connection.
> 
> Double-checking: when we're in this state, there is a client connected.
> Correct?

Yes. This is a client state that describes the connection as valid: the
channel got negotiated and is ready to process requests.

> 
>> +#
>> +# @quit: Client will not reconnect.
> 
> What does this one mean exactly?

The client stopped for good. Requests fail and it never reconnects.
This state is reached on fatal error or teardown.

> 
>> +#
>> +# Since: 11.2
> 
> Making 11.2 seems unlikely.

Sure. Would updating this to 12.0 make more sense ?

>> +#
>> +# @running: Connection attempt is in progress.
>> +#
>> +# @parked: Connection was successful and channel is kept for the next
>> +#     request.
>> +#
>> +# Since: 11.2
>> +##
>> +{ 'enum': 'XDbgNbdClientConnectorState',
>> +  'data': [ 'running', 'parked' ] }
>> +
>> +##
>> +# @XDbgNbdClientConnector:
>> +#     The NBD connector information.
>> +#
>> +# @state: State of the connector.
>> +#
>> +# Since: 11.2
>> +##
>> +{ 'struct': 'XDbgNbdClientConnector',
>> +  'data': { 'state': 'XDbgNbdClientConnectorState' } }
>> +
>> +##
>> +# @XDbgNbdClient:
>> +#     The NBD connection state of the client and its connector.
>> +#     The two members are read independently and may not reflect a
>> +#     single point in time.
> 
> That's unfortunate.  It'll do for debugging.  Could it create robustness
> issues in tests?

On the nbd-stale-connection test it is not a problem but I can try to
take the locks together in v2.

> 
>> +#
>> +# @state: The client state.
>> +#
>> +# @connector: The connector details.  Only present while a connection
>> +#     attempt is running or its channel is parked.
>> +#
>> +# Since: 11.2
>> +##
>> +{ 'struct': 'XDbgNbdClient',
>> +  'data': { 'state': 'XDbgNbdClientState',
>> +            '*connector': 'XDbgNbdClientConnector' } }
> 
> How the combined state machine works is not clear to me.  Is it
> documented anywhere?

I did not find a proper documentation. I found the comments on the code
to be quite explicit though. The connector only runs in a connecting-*
state. In that state, when processing a request, the client tries to
get the channel from the connector. When the connector has a valid
channel but the client did not request it, the connector is in parked
state. Do you think this command should document that behavior?

> 
>> +
>> +##
>> +# @x-debug-query-nbd-client:
>> +#     Get the NBD client state.
>> +#
>> +# @node-name: node name of the NBD client to query.
> 
> How would I find the valid node names?  query-block-exports?

The node-name is listed by query-named-block-nodes or is the one passed
to blockdev-add. I will add a reference to those commands in the
docstring to clarify.

Re: [PATCH 1/2] nbd: Add an unstable query for the client state

[Eric Blake]

On Tue, Nov 10, 2026 at 04:07:35PM +0100, guillaume vincke wrote:
> On 11/10/26 13:53, Markus Armbruster wrote:
> 
> Thanks for the review. Let me clarify before posting a v2.
> 
> > 
> > Why block-core.json and not block-export.json?
> 
> If I get this right, block-export.json covers the server part. I
> thought adding this next to BlockdevOptionsNbd was a better fit. I can
> change that in v2 if you think block-export.json is a better match.

I'll trust Markus on this part of the review.

> > > +#
> > > +# @quit: Client will not reconnect.
> > 
> > What does this one mean exactly?
> 
> The client stopped for good. Requests fail and it never reconnects.
> This state is reached on fatal error or teardown.

So either the server gave up on the client (a protocol error such as
an incorrect magic number that means the server has lost sync and
prefers not to further entertain further input from the client), or
the client gracefully terminated (the NBD_OPT_ABORT during handshake
or NBD_CMD_DISC during normal operation for the client to tell the
server it is done). I don't know if we can tell those apart, nor if we
need to.  For debugging purposes, merely knowing that the connection
is done (even if the fd is still in the process of being cleaned up)
is already good enough for most purposes.

> 
> > 
> > > +#
> > > +# Since: 11.2
> > 
> > Making 11.2 seems unlikely.
> 
> Sure. Would updating this to 12.0 make more sense ?

Agreed - this is too much on the "new feature" side to make soft
freeze; even if it only exists to make debugging easier and the iotest
that is now in 11.2 a bit more robust, I think aiming the new QMP
command for 12.0 is better.

> > > +#
> > > +# @state: The client state.
> > > +#
> > > +# @connector: The connector details.  Only present while a connection
> > > +#     attempt is running or its channel is parked.
> > > +#
> > > +# Since: 11.2
> > > +##
> > > +{ 'struct': 'XDbgNbdClient',
> > > +  'data': { 'state': 'XDbgNbdClientState',
> > > +            '*connector': 'XDbgNbdClientConnector' } }
> > 
> > How the combined state machine works is not clear to me.  Is it
> > documented anywhere?
> 
> I did not find a proper documentation. I found the comments on the code
> to be quite explicit though. The connector only runs in a connecting-*
> state. In that state, when processing a request, the client tries to
> get the channel from the connector. When the connector has a valid
> channel but the client did not request it, the connector is in parked
> state. Do you think this command should document that behavior?
> 
> > 
> > > +
> > > +##
> > > +# @x-debug-query-nbd-client:
> > > +#     Get the NBD client state.
> > > +#
> > > +# @node-name: node name of the NBD client to query.
> > 
> > How would I find the valid node names?  query-block-exports?
> 
> The node-name is listed by query-named-block-nodes or is the one passed
> to blockdev-add. I will add a reference to those commands in the
> docstring to clarify.

The export name need not match the node name (although it is common
for management apps to set the export name to copy the node name).

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

Re: [PATCH 1/2] nbd: Add an unstable query for the client state

[Markus Armbruster]

guillaume vincke <guillaume.vincke@ovhcloud.com> writes:

> On 11/10/26 13:53, Markus Armbruster wrote:
>
> Thanks for the review. Let me clarify before posting a v2.
>
>> Why block-core.json and not block-export.json?
>
> If I get this right, block-export.json covers the server part. I
> thought adding this next to BlockdevOptionsNbd was a better fit. I can
> change that in v2 if you think block-export.json is a better match.

My question was a question, not a change request :)

>>> +#
>>> +# @connecting-wait: No active connection.  A request waits for the
>>> +#     connection attempt to finish, while reconnect-delay has not
>>> +#     expired.
>>> +#
>>> +# @connecting-nowait: No active connection.  A request fails at once
>>> +#     if no channel is ready.
>>> +#
>>> +# @connected: Client has a valid connection.
>>
>> Double-checking: when we're in this state, there is a client connected.
>> Correct?
>
> Yes. This is a client state that describes the connection as valid: the
> channel got negotiated and is ready to process requests.
>
>> 
>>> +#
>>> +# @quit: Client will not reconnect.
>>
>> What does this one mean exactly?
>
> The client stopped for good. Requests fail and it never reconnects.
> This state is reached on fatal error or teardown.

Work some of that into the description?

>>> +#
>>> +# Since: 11.2
>>
>> Making 11.2 seems unlikely.
>
> Sure. Would updating this to 12.0 make more sense ?

Yes.

>>> +#
>>> +# @running: Connection attempt is in progress.
>>> +#
>>> +# @parked: Connection was successful and channel is kept for the next
>>> +#     request.
>>> +#
>>> +# Since: 11.2
>>> +##
>>> +{ 'enum': 'XDbgNbdClientConnectorState',
>>> +  'data': [ 'running', 'parked' ] }
>>> +
>>> +##
>>> +# @XDbgNbdClientConnector:
>>> +#     The NBD connector information.
>>> +#
>>> +# @state: State of the connector.
>>> +#
>>> +# Since: 11.2
>>> +##
>>> +{ 'struct': 'XDbgNbdClientConnector',
>>> +  'data': { 'state': 'XDbgNbdClientConnectorState' } }
>>> +
>>> +##
>>> +# @XDbgNbdClient:
>>> +#     The NBD connection state of the client and its connector.
>>> +#     The two members are read independently and may not reflect a
>>> +#     single point in time.
>>
>> That's unfortunate.  It'll do for debugging.  Could it create robustness
>> issues in tests?
>
> On the nbd-stale-connection test it is not a problem but I can try to
> take the locks together in v2.

Worth a try, I guess.

>>> +#
>>> +# @state: The client state.
>>> +#
>>> +# @connector: The connector details.  Only present while a connection
>>> +#     attempt is running or its channel is parked.

When exactly is this member present?

Is it always present when @state has certain values, and never present
when it doesn't?  Or is it more complicated?

>>> +#
>>> +# Since: 11.2
>>> +##
>>> +{ 'struct': 'XDbgNbdClient',
>>> +  'data': { 'state': 'XDbgNbdClientState',
>>> +            '*connector': 'XDbgNbdClientConnector' } }
>>
>> How the combined state machine works is not clear to me.  Is it
>> documented anywhere?
>
> I did not find a proper documentation. I found the comments on the code
> to be quite explicit though. The connector only runs in a connecting-*
> state. In that state, when processing a request, the client tries to
> get the channel from the connector. When the connector has a valid
> channel but the client did not request it, the connector is in parked
> state. Do you think this command should document that behavior?

Perhaps the doc comments are good enough for a debugging command to be
used by people who know how the state machine works.

I don't, I've read the doc comments several times, and the state machine
is still a mystery to me.  Can we do better with reasonable effort?

>>> +
>>> +##
>>> +# @x-debug-query-nbd-client:
>>> +#     Get the NBD client state.
>>> +#
>>> +# @node-name: node name of the NBD client to query.
>>
>> How would I find the valid node names?  query-block-exports?
>
> The node-name is listed by query-named-block-nodes or is the one passed
> to blockdev-add. I will add a reference to those commands in the
> docstring to clarify.

What exactly makes such a node name an "NBD client"?

Re: [PATCH 1/2] nbd: Add an unstable query for the client state

[guillaume vincke]

On 11/11/26 15:22, Markus Armbruster wrote:
> guillaume vincke <guillaume.vincke@ovhcloud.com> writes:
> 
>> On 11/10/26 13:53, Markus Armbruster wrote:
>>
>> Thanks for the review. Let me clarify before posting a v2.
>>
>>> Why block-core.json and not block-export.json?
>>
>> If I get this right, block-export.json covers the server part. I
>> thought adding this next to BlockdevOptionsNbd was a better fit. I can
>> change that in v2 if you think block-export.json is a better match.
> 
> My question was a question, not a change request :)
> 

I was a bit too eager there :) I will keep it where it stands for now.

>>>> +#
>>>> +# @connecting-wait: No active connection.  A request waits for the
>>>> +#     connection attempt to finish, while reconnect-delay has not
>>>> +#     expired.
>>>> +#
>>>> +# @connecting-nowait: No active connection.  A request fails at once
>>>> +#     if no channel is ready.
>>>> +#
>>>> +# @connected: Client has a valid connection.
>>>
>>> Double-checking: when we're in this state, there is a client connected.
>>> Correct?
>>
>> Yes. This is a client state that describes the connection as valid: the
>> channel got negotiated and is ready to process requests.
>>
>>>
>>>> +#
>>>> +# @quit: Client will not reconnect.
>>>
>>> What does this one mean exactly?
>>
>> The client stopped for good. Requests fail and it never reconnects.
>> This state is reached on fatal error or teardown.
> 
> Work some of that into the description?
> 

Will do!
>>>> +#
>>>> +# @running: Connection attempt is in progress.
>>>> +#
>>>> +# @parked: Connection was successful and channel is kept for the next
>>>> +#     request.
>>>> +#
>>>> +# Since: 11.2
>>>> +##
>>>> +{ 'enum': 'XDbgNbdClientConnectorState',
>>>> +  'data': [ 'running', 'parked' ] }
>>>> +
>>>> +##
>>>> +# @XDbgNbdClientConnector:
>>>> +#     The NBD connector information.
>>>> +#
>>>> +# @state: State of the connector.
>>>> +#
>>>> +# Since: 11.2
>>>> +##
>>>> +{ 'struct': 'XDbgNbdClientConnector',
>>>> +  'data': { 'state': 'XDbgNbdClientConnectorState' } }
>>>> +
>>>> +##
>>>> +# @XDbgNbdClient:
>>>> +#     The NBD connection state of the client and its connector.
>>>> +#     The two members are read independently and may not reflect a
>>>> +#     single point in time.
>>>
>>> That's unfortunate.  It'll do for debugging.  Could it create robustness
>>> issues in tests?
>>
>> On the nbd-stale-connection test it is not a problem but I can try to
>> take the locks together in v2.
> 
> Worth a try, I guess.
> 

Will add that in v2.

>>>> +#
>>>> +# @state: The client state.
>>>> +#
>>>> +# @connector: The connector details.  Only present while a connection
>>>> +#     attempt is running or its channel is parked.
> 
> When exactly is this member present?
> 
> Is it always present when @state has certain values, and never present
> when it doesn't?  Or is it more complicated?
> 

It is never present when @state is connected. on connecting-* it is
absent until lazy reconnection is triggered (first request after a
failure). In quit, a running attempt or a parked channel stays until
the node is closed, so the connector can be absent, running or parked.

>>>> +#
>>>> +# Since: 11.2
>>>> +##
>>>> +{ 'struct': 'XDbgNbdClient',
>>>> +  'data': { 'state': 'XDbgNbdClientState',
>>>> +            '*connector': 'XDbgNbdClientConnector' } }
>>>
>>> How the combined state machine works is not clear to me.  Is it
>>> documented anywhere?
>>
>> I did not find a proper documentation. I found the comments on the code
>> to be quite explicit though. The connector only runs in a connecting-*
>> state. In that state, when processing a request, the client tries to
>> get the channel from the connector. When the connector has a valid
>> channel but the client did not request it, the connector is in parked
>> state. Do you think this command should document that behavior?
> 
> Perhaps the doc comments are good enough for a debugging command to be
> used by people who know how the state machine works.
> 
> I don't, I've read the doc comments several times, and the state machine
> is still a mystery to me.  Can we do better with reasonable effort?
> 

would something like this help:

1- the connector is absent while connected
2- a failure moves the client to connecting-wait. It gets to Nowait
    when the configured reconnect-delay is reached.
3- In connecting-* states, the next request starts the connector which
    gets into running.
4- On wait, the request blocks and takes the channel when it's ready.
    On nowait, the request fails at once if no parked channel is
    available.
5- on successful connection, the connector parks the channel.
6- when the channel gets picked up by the client, it gets to connected
    and the connector is cleared.
7- on fatal error or when teardown is requested, the state gets to
    quit.


>>>> +
>>>> +##
>>>> +# @x-debug-query-nbd-client:
>>>> +#     Get the NBD client state.
>>>> +#
>>>> +# @node-name: node name of the NBD client to query.
>>>
>>> How would I find the valid node names?  query-block-exports?
>>
>> The node-name is listed by query-named-block-nodes or is the one passed
>> to blockdev-add. I will add a reference to those commands in the
>> docstring to clarify.
> 
> What exactly makes such a node name an "NBD client"?

It's a node whose driver is NBD. It gets added with blockdev-add with
"driver": "nbd". Any other node gets the error "is not an NBD client".
Would changing the doc to "node name of a block node using the nbd
driver" with a reference to the query-named-block-nodes be better ?

[PATCH 2/2] iotests: Make nbd-stale-connection test deterministic

[Guillaume Vincke]

At the moment, the test sleeps to wait for the connector to park a new
channel, which can fail on slow or loaded hosts.

Poll the x-debug-query-nbd-client command to verify the current state
instead of relying on timing.

Signed-off-by: Guillaume Vincke <guillaume.vincke@ovhcloud.com>
---
 tests/qemu-iotests/tests/nbd-stale-connection | 17 ++++++++++++-----
 1 file changed, 12 insertions(+), 5 deletions(-)

diff --git a/tests/qemu-iotests/tests/nbd-stale-connection b/tests/qemu-iotests/tests/nbd-stale-connection
index e361bccfa9..265e18adcf 100755
--- a/tests/qemu-iotests/tests/nbd-stale-connection
+++ b/tests/qemu-iotests/tests/nbd-stale-connection
@@ -12,14 +12,21 @@ import iotests
 from iotests import qemu_nbd_popen, qemu_img_create, file_path, log
 
 
-# estimated duration required by the connect thread to reach a running server
-MAX_NBD_CONNECT_DURATION = 1
-
 iotests.script_initialize(supported_fmts=['qcow2'])
 nbd_sock = file_path('nbd-sock', base_dir=iotests.sock_dir)
 disk = file_path('disk')
 qemu_img_create('-f', iotests.imgfmt, disk, '1M')
 
+
+def wait_for_parked_connection(virtm, node_name):
+    with iotests.Timeout(30, "connect-thread did not reach 'parked' state"):
+        while True:
+            client = virtm.qmp('x-debug-query-nbd-client', node_name=node_name)
+            if client['return'].get('connector', {}).get('state') == 'parked':
+                return
+            time.sleep(0.1)
+
+
 with iotests.VM() as vm:
 
     vm.launch()
@@ -44,7 +51,7 @@ with iotests.VM() as vm:
         # in CONNECTING, the operation starts the connect thread
         log(vm.qmp('x-qemu-io', device='nbd0', command='write -P 0x5a 0 64k'))
         # wait for the connect thread to get a new channel and stop
-        time.sleep(MAX_NBD_CONNECT_DURATION)
+        wait_for_parked_connection(vm, 'nbd0')
 
     iotests.log('=== NBD server down. The parked channel is closed ===')
 
@@ -56,5 +63,5 @@ with iotests.VM() as vm:
         # Wait for it to succeed
         iotests.log('=== Wait for the connect thread to park the new channel. '
                     'Subsequent operations are successful ===')
-        time.sleep(MAX_NBD_CONNECT_DURATION)
+        wait_for_parked_connection(vm, 'nbd0')
         log(vm.qmp('x-qemu-io', device='nbd0', command='write -P 0x5a 0 64k'))
-- 
2.43.0