[PULL 0/4] NBD patches through 2025-03-05

[PULL 0/4] NBD patches through 2025-03-05

[Eric Blake]

The following changes since commit 661c2e1ab29cd9c4d268ae3f44712e8d421c0e56:

  scripts/checkpatch: Fix a typo (2025-03-04 09:30:26 +0800)

are available in the Git repository at:

  https://repo.or.cz/qemu/ericb.git tags/pull-nbd-2025-03-05

for you to fetch changes up to 3e1683485656c095860a8dfbe39ab2d0664b84d9:

  nbd: Defer trace init until after daemonization (2025-03-05 13:00:22 -0600)

----------------------------------------------------------------
NBD patches for 2025-03-05

- Several iotest fixes
- Refactor QMP for NbdServerOptions for less repetition
- Avoid a hang in 'qemu-nbd --fork' when simple trace backend is enabled

----------------------------------------------------------------
Eric Blake (1):
      nbd: Defer trace init until after daemonization

Nir Soffer (1):
      iotest: Unbreak 302 with python 3.13

Thomas Huth (1):
      iotests: Stop NBD server in test 162 before starting the next one

Vladimir Sementsov-Ogievskiy (1):
      qapi: merge common parts of NbdServerOptions and nbd-server-start data

 qapi/block-export.json | 94 ++++++++++++++++++++++----------------------------
 blockdev-nbd.c         |  4 +--
 qemu-nbd.c             | 16 ++++++---
 tests/qemu-iotests/162 |  1 +
 tests/qemu-iotests/302 | 19 +++++++---
 5 files changed, 71 insertions(+), 63 deletions(-)

-- 
2.48.1

[PULL 1/4] iotest: Unbreak 302 with python 3.13

[Eric Blake]

From: Nir Soffer <nirsof@gmail.com>

This test depends on TarFile.addfile() to add tar member header without
writing the member data, which we write ourself using qemu-nbd. Python
3.13 changed the function in a backward incompatible way[1] to require a
file object for tarinfo with non-zero size, breaking the test:

     -[{"name": "vm.ovf", "offset": 512, "size": 6}, {"name": "disk", "offset": 1536, "size": 393216}]
     +Traceback (most recent call last):
     +  File "/home/stefanha/qemu/tests/qemu-iotests/302", line 118, in <module>
     +    tar.addfile(disk)
     +    ~~~~~~~~~~~^^^^^^
     +  File "/usr/lib64/python3.13/tarfile.py", line 2262, in addfile
     +    raise ValueError("fileobj not provided for non zero-size regular file")
     +ValueError: fileobj not provided for non zero-size regular file

The new behavior makes sense for most users, but breaks our unusual
usage. Fix the test to add the member header directly using public but
undocumented attributes. This is more fragile but the test works again.

This also fixes a bug in the previous code - when calling addfile()
without a fileobject, tar.offset points to the start of the member data
instead of the end.

[1] https://github.com/python/cpython/pull/117988

Signed-off-by: Nir Soffer <nirsof@gmail.com>
Message-ID: <20250228195708.48035-1-nirsof@gmail.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Signed-off-by: Eric Blake <eblake@redhat.com>
---
 tests/qemu-iotests/302 | 19 ++++++++++++++-----
 1 file changed, 14 insertions(+), 5 deletions(-)

diff --git a/tests/qemu-iotests/302 b/tests/qemu-iotests/302
index a6d79e727b5..e980ec513f2 100755
--- a/tests/qemu-iotests/302
+++ b/tests/qemu-iotests/302
@@ -115,13 +115,22 @@ with tarfile.open(tar_file, "w") as tar:

     disk = tarfile.TarInfo("disk")
     disk.size = actual_size
-    tar.addfile(disk)

-    # 6. Shrink the tar to the actual size, aligned to 512 bytes.
+    # Since python 3.13 we cannot use addfile() to create the member header.
+    # Add the tarinfo directly using public but undocumented attributes.

-    tar_size = offset + (disk.size + 511) & ~511
-    tar.fileobj.seek(tar_size)
-    tar.fileobj.truncate(tar_size)
+    buf = disk.tobuf(tar.format, tar.encoding, tar.errors)
+    tar.fileobj.write(buf)
+    tar.members.append(disk)
+
+    # Update the offset and position to the location of the next member.
+
+    tar.offset = offset + (disk.size + 511) & ~511
+    tar.fileobj.seek(tar.offset)
+
+    # 6. Shrink the tar to the actual size.
+
+    tar.fileobj.truncate(tar.offset)

 with tarfile.open(tar_file) as tar:
     members = [{"name": m.name, "size": m.size, "offset": m.offset_data}
-- 
2.48.1

[PULL 2/4] iotests: Stop NBD server in test 162 before starting the next one

[Eric Blake]

From: Thomas Huth <thuth@redhat.com>

Test 162 recently started failing for me for no obvious reasons (I
did not spot any suspicious commits in this area), but looking in
the 162.out.bad log file, there was a suspicious message at the end:

 qemu-nbd: Cannot lock pid file: Resource temporarily unavailable

And indeed, the test starts the NBD server two times, without stopping
the first server before running the second one, so the second one can
indeed fail to lock the PID file. Thus let's make sure to stop the
first server before the test continues with the second one. With this
change, the test works fine for me again.

Signed-off-by: Thomas Huth <thuth@redhat.com>
Message-ID: <20250225070650.387638-1-thuth@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Signed-off-by: Eric Blake <eblake@redhat.com>
---
 tests/qemu-iotests/162 | 1 +
 1 file changed, 1 insertion(+)

diff --git a/tests/qemu-iotests/162 b/tests/qemu-iotests/162
index 94dae60d304..956c2c5f339 100755
--- a/tests/qemu-iotests/162
+++ b/tests/qemu-iotests/162
@@ -65,6 +65,7 @@ done

 $QEMU_IMG info "json:{'driver': 'nbd', 'host': 'localhost', 'port': $port}" \
     | grep '^image' | sed -e "s/$port/PORT/"
+_stop_nbd_server

 # This is a test for NBD's bdrv_refresh_filename() implementation: It expects
 # either host or path to be set, but it must not assume that they are set to
-- 
2.48.1

[PULL 3/4] qapi: merge common parts of NbdServerOptions and nbd-server-start data

[Eric Blake]

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

Instead of comment
"Keep this type consistent with the nbd-server-start arguments", we
can simply merge these things.

Note that each field of new base already has "since" tag, equal in both
original copies. So "since" information is saved.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
Message-ID: <20250219191914.440451-1-vsementsov@yandex-team.ru>
Reviewed-by: Eric Blake <eblake@redhat.com>
Signed-off-by: Eric Blake <eblake@redhat.com>
---
 qapi/block-export.json | 94 +++++++++++++++++++-----------------------
 blockdev-nbd.c         |  4 +-
 2 files changed, 44 insertions(+), 54 deletions(-)

diff --git a/qapi/block-export.json b/qapi/block-export.json
index 68dcec7edc5..c783e01a532 100644
--- a/qapi/block-export.json
+++ b/qapi/block-export.json
@@ -9,53 +9,7 @@
 { 'include': 'block-core.json' }

 ##
-# @NbdServerOptions:
-#
-# Keep this type consistent with the nbd-server-start arguments.  The
-# only intended difference is using SocketAddress instead of
-# SocketAddressLegacy.
-#
-# @addr: Address on which to listen.
-#
-# @handshake-max-seconds: Time limit, in seconds, at which a client
-#     that has not completed the negotiation handshake will be
-#     disconnected, 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
-#     the client's x509 distinguished name.  This object is is only
-#     resolved at time of use, so can be deleted and recreated on the
-#     fly while the NBD server is active.  If missing, it will default
-#     to denying access (since 4.0).
-#
-# @max-connections: The maximum number of connections to allow at the
-#     same time, 0 for unlimited.  Setting this to 1 also stops the
-#     server from advertising multiple client support (since 5.2;
-#     default: 100)
-#
-# Since: 4.2
-##
-{ 'struct': 'NbdServerOptions',
-  'data': { 'addr': 'SocketAddress',
-            '*handshake-max-seconds': 'uint32',
-            '*tls-creds': 'str',
-            '*tls-authz': 'str',
-            '*max-connections': 'uint32' } }
-
-##
-# @nbd-server-start:
-#
-# Start an NBD server listening on the given host and port.  Block
-# devices can then be exported using @nbd-server-add.  The NBD server
-# will present them as named exports; for example, another QEMU
-# instance could refer to them as "nbd:HOST:PORT:exportname=NAME".
-#
-# Keep this type consistent with the NbdServerOptions type.  The only
-# intended difference is using SocketAddressLegacy instead of
-# SocketAddress.
-#
-# @addr: Address on which to listen.
+# @NbdServerOptionsBase:
 #
 # @handshake-max-seconds: Time limit, in seconds, at which a client
 #     that has not completed the negotiation handshake will be
@@ -73,6 +27,46 @@
 #     same time, 0 for unlimited.  Setting this to 1 also stops the
 #     server from advertising multiple client support (since 5.2;
 #     default: 100).
+##
+{ 'struct': 'NbdServerOptionsBase',
+  'data': { '*handshake-max-seconds': 'uint32',
+            '*tls-creds': 'str',
+            '*tls-authz': 'str',
+            '*max-connections': 'uint32' } }
+
+##
+# @NbdServerOptions:
+#
+# Keep this type consistent with the NbdServerOptionsLegacy type.  The
+# only intended difference is using SocketAddress instead of
+# SocketAddressLegacy.
+#
+# @addr: Address on which to listen (since 4.2).
+##
+{ 'struct': 'NbdServerOptions',
+  'base': 'NbdServerOptionsBase',
+  'data': { 'addr': 'SocketAddress' } }
+
+##
+# @NbdServerOptionsLegacy:
+#
+# Keep this type consistent with the NbdServerOptions type.  The only
+# intended difference is using SocketAddressLegacy instead of
+# SocketAddress.
+#
+# @addr: Address on which to listen (since 1.3).
+##
+{ 'struct': 'NbdServerOptionsLegacy',
+  'base': 'NbdServerOptionsBase',
+  'data': { 'addr': 'SocketAddressLegacy' } }
+
+##
+# @nbd-server-start:
+#
+# Start an NBD server listening on the given host and port.  Block
+# devices can then be exported using @nbd-server-add.  The NBD server
+# will present them as named exports; for example, another QEMU
+# instance could refer to them as "nbd:HOST:PORT:exportname=NAME".
 #
 # Errors:
 #     - if the server is already running
@@ -80,11 +74,7 @@
 # Since: 1.3
 ##
 { 'command': 'nbd-server-start',
-  'data': { 'addr': 'SocketAddressLegacy',
-            '*handshake-max-seconds': 'uint32',
-            '*tls-creds': 'str',
-            '*tls-authz': 'str',
-            '*max-connections': 'uint32' },
+  'data': 'NbdServerOptionsLegacy',
   'allow-preconfig': true }

 ##
diff --git a/blockdev-nbd.c b/blockdev-nbd.c
index 3f6f4ef92b4..1e3e634b87d 100644
--- a/blockdev-nbd.c
+++ b/blockdev-nbd.c
@@ -219,12 +219,12 @@ void nbd_server_start_options(NbdServerOptions *arg, Error **errp)
                      arg->tls_authz, arg->max_connections, errp);
 }

-void qmp_nbd_server_start(SocketAddressLegacy *addr,
-                          bool has_handshake_max_secs,
+void qmp_nbd_server_start(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,
+                          SocketAddressLegacy *addr,
                           Error **errp)
 {
     SocketAddress *addr_flat = socket_address_flatten(addr);
-- 
2.48.1

[PULL 4/4] nbd: Defer trace init until after daemonization

[Eric Blake]

At least the simple trace backend works by spawning a helper thread,
and setting up an atexit() handler that coordinates completion with
the helper thread.  But since atexit registrations survive fork() but
helper threads do not, this means that qemu-nbd configured to use the
simple trace will deadlock waiting for a thread that no longer exists
when it has daemonized.

Better is to follow the example of vl.c: don't call any setup
functions that might spawn helper threads until we are in the final
process that will be doing the work worth tracing.

Tested by configuring with --enable-trace-backends=simple, then running
  qemu-nbd --fork --trace=nbd_\*,file=qemu-nbd.trace -f raw -r README.rst
followed by `nbdinfo nbd://localhost`, and observing that the trace
file is now created without hanging.

Reported-by: Thomas Huth <thuth@redhat.com>
Signed-off-by: Eric Blake <eblake@redhat.com>
Message-ID: <20250227220625.870246-2-eblake@redhat.com>
Reviewed-by: Thomas Huth <thuth@redhat.com>
---
 qemu-nbd.c | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/qemu-nbd.c b/qemu-nbd.c
index 05b61da51ea..ed5895861bb 100644
--- a/qemu-nbd.c
+++ b/qemu-nbd.c
@@ -852,10 +852,6 @@ int main(int argc, char **argv)
         export_name = "";
     }

-    if (!trace_init_backends()) {
-        exit(1);
-    }
-    trace_init_file();
     qemu_set_log(LOG_TRACE, &error_fatal);

     socket_activation = check_socket_activation();
@@ -1045,6 +1041,18 @@ int main(int argc, char **argv)
 #endif /* WIN32 */
     }

+    /*
+     * trace_init must be done after daemonization.  Why? Because at
+     * least the simple backend spins up a helper thread as well as an
+     * atexit() handler that waits on that thread, but the helper
+     * thread won't survive a fork, leading to deadlock in the child
+     * if we initialized pre-fork.
+     */
+    if (!trace_init_backends()) {
+        exit(1);
+    }
+    trace_init_file();
+
     if (opts.device != NULL && sockpath == NULL) {
         sockpath = g_malloc(128);
         snprintf(sockpath, 128, SOCKET_PATH, basename(opts.device));
-- 
2.48.1

Re: [PULL 0/4] NBD patches through 2025-03-05

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 116 bytes --]

Applied, thanks.

Please update the changelog at https://wiki.qemu.org/ChangeLog/10.0 for any user-visible changes.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]