From d0f439ddd37e1354daca3234976295189985cb04 Mon Sep 17 00:00:00 2001 From: Nir Soffer Date: Fri, 28 Feb 2025 21:57:08 +0200 Subject: [PATCH 1/4] iotest: Unbreak 302 with python 3.13 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 + 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 Message-ID: <20250228195708.48035-1-nirsof@gmail.com> Reviewed-by: Eric Blake Reviewed-by: Stefan Hajnoczi Signed-off-by: Eric Blake --- 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 a6d79e727b..e980ec513f 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} From e2668ba1ed44ad56f2f1653ff5f53b277d534fac Mon Sep 17 00:00:00 2001 From: Thomas Huth Date: Tue, 25 Feb 2025 08:06:50 +0100 Subject: [PATCH 2/4] iotests: Stop NBD server in test 162 before starting the next one 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 Message-ID: <20250225070650.387638-1-thuth@redhat.com> Reviewed-by: Eric Blake Signed-off-by: Eric Blake --- tests/qemu-iotests/162 | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/qemu-iotests/162 b/tests/qemu-iotests/162 index 94dae60d30..956c2c5f33 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 From 57f3962bf17c088c3567d216e3eaa1b3131be5a4 Mon Sep 17 00:00:00 2001 From: Vladimir Sementsov-Ogievskiy Date: Wed, 19 Feb 2025 22:19:14 +0300 Subject: [PATCH 3/4] qapi: merge common parts of NbdServerOptions and nbd-server-start data 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 Message-ID: <20250219191914.440451-1-vsementsov@yandex-team.ru> Reviewed-by: Eric Blake Signed-off-by: Eric Blake --- blockdev-nbd.c | 4 +- qapi/block-export.json | 94 +++++++++++++++++++----------------------- 2 files changed, 44 insertions(+), 54 deletions(-) diff --git a/blockdev-nbd.c b/blockdev-nbd.c index 3f6f4ef92b..1e3e634b87 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); diff --git a/qapi/block-export.json b/qapi/block-export.json index 68dcec7edc..c783e01a53 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 } ## From 3e1683485656c095860a8dfbe39ab2d0664b84d9 Mon Sep 17 00:00:00 2001 From: Eric Blake Date: Thu, 27 Feb 2025 16:06:15 -0600 Subject: [PATCH 4/4] nbd: Defer trace init until after daemonization 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 Signed-off-by: Eric Blake Message-ID: <20250227220625.870246-2-eblake@redhat.com> Reviewed-by: Thomas Huth --- qemu-nbd.c | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-) diff --git a/qemu-nbd.c b/qemu-nbd.c index 05b61da51e..ed5895861b 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));