[PULL v2 2/2] iotests: test NBD+TLS+iothread

[Eric Blake]

Prevent regressions when using NBD with TLS in the presence of
iothreads, adding coverage the fix to qio channels made in the
previous patch.

The shell function pick_unused_port() was copied from
nbdkit.git/tests/functions.sh.in, where it had all authors from Red
Hat, agreeing to the resulting relicensing from 2-clause BSD to GPLv2.

CC: qemu-sta...@nongnu.org
CC: "Richard W.M. Jones" 
Signed-off-by: Eric Blake 
Message-ID: <20240531180639.1392905-6-ebl...@redhat.com>
Reviewed-by: Daniel P. Berrangé 
---
 tests/qemu-iotests/tests/nbd-tls-iothread | 168 ++
 tests/qemu-iotests/tests/nbd-tls-iothread.out |  54 ++
 2 files changed, 222 insertions(+)
 create mode 100755 tests/qemu-iotests/tests/nbd-tls-iothread
 create mode 100644 tests/qemu-iotests/tests/nbd-tls-iothread.out

diff --git a/tests/qemu-iotests/tests/nbd-tls-iothread 
b/tests/qemu-iotests/tests/nbd-tls-iothread
new file mode 100755
index 000..a2fb07206e5
--- /dev/null
+++ b/tests/qemu-iotests/tests/nbd-tls-iothread
@@ -0,0 +1,168 @@
+#!/usr/bin/env bash
+# group: rw quick
+#
+# Test of NBD+TLS+iothread
+#
+# Copyright (C) 2024 Red Hat, Inc.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+#
+
+# creator
+owner=ebl...@redhat.com
+
+seq=`basename $0`
+echo "QA output created by $seq"
+
+status=1# failure is the default!
+
+_cleanup()
+{
+_cleanup_qemu
+_cleanup_test_img
+rm -f "$dst_image"
+tls_x509_cleanup
+}
+trap "_cleanup; exit \$status" 0 1 2 3 15
+
+# get standard environment, filters and checks
+cd ..
+. ./common.rc
+. ./common.filter
+. ./common.qemu
+. ./common.tls
+. ./common.nbd
+
+_supported_fmt qcow2  # Hardcoded to qcow2 command line and QMP below
+_supported_proto file
+
+# pick_unused_port
+#
+# Picks and returns an "unused" port, setting the global variable
+# $port.
+#
+# This is inherently racy, but we need it because qemu does not currently
+# permit NBD+TLS over a Unix domain socket
+pick_unused_port ()
+{
+if ! (ss --version) >/dev/null 2>&1; then
+_notrun "ss utility required, skipped this test"
+fi
+
+# Start at a random port to make it less likely that two parallel
+# tests will conflict.
+port=$(( 5 + (RANDOM%15000) ))
+while ss -ltn | grep -sqE ":$port\b"; do
+((port++))
+if [ $port -eq 65000 ]; then port=5; fi
+done
+echo picked unused port
+}
+
+tls_x509_init
+
+size=1G
+DST_IMG="$TEST_DIR/dst.qcow2"
+
+echo
+echo "== preparing TLS creds and spare port =="
+
+pick_unused_port
+tls_x509_create_root_ca "ca1"
+tls_x509_create_server "ca1" "server1"
+tls_x509_create_client "ca1" "client1"
+tls_obj_base=tls-creds-x509,id=tls0,verify-peer=true,dir="${tls_dir}"
+
+echo
+echo "== preparing image =="
+
+_make_test_img $size
+$QEMU_IMG create -f qcow2 "$DST_IMG" $size | _filter_img_create
+
+echo
+echo === Starting Src QEMU ===
+echo
+
+_launch_qemu -machine q35 \
+-object iothread,id=iothread0 \
+-object "${tls_obj_base}"/client1,endpoint=client \
+-device '{"driver":"pcie-root-port", "id":"root0", "multifunction":true,
+  "bus":"pcie.0"}' \
+-device '{"driver":"virtio-scsi-pci", "id":"virtio_scsi_pci0",
+  "bus":"root0", "iothread":"iothread0"}' \
+-device '{"driver":"scsi-hd", "id":"image1", "drive":"drive_image1",
+  "bus":"virtio_scsi_pci0.0"}' \
+-blockdev '{"driver":"file", "cache":{"direct":true, "no-flush":false},
+"filename":"'"$TEST_IMG"'", "node-name":"drive_sys1"}' \
+-blockdev '{"driver":"qcow2", "node-name":"drive_image1",
+"file":"drive_sys1"}'
+h1=$QEMU_HANDLE
+_send_qemu_cmd $h1 '{"execute": "qmp_capabilities"}' 'return'
+
+echo
+echo === Starting Dst VM2 ===
+echo
+
+_launch_qemu -machine q35 \
+-object 

[PULL v2 0/2] NBD patches for 2024-05-30

[Eric Blake]

The following changes since commit 3b2fe44bb7f605f179e5e7feb2c13c2eb3abbb80:

  Merge tag 'pull-request-2024-05-29' of https://gitlab.com/thuth/qemu into 
staging (2024-05-29 08:38:20 -0700)

are available in the Git repository at:

  https://repo.or.cz/qemu/ericb.git tags/pull-nbd-2024-05-30-v2

for you to fetch changes up to a73c99378022ebb785481e84cfe1e81097546268:

  iotests: test NBD+TLS+iothread (2024-06-03 09:17:11 -0500)


NBD patches for 2024-05-30

- Fix AioContext assertion with NBD+TLS


Eric Blake (2):
  qio: Inherit follow_coroutine_ctx across TLS
  iotests: test NBD+TLS+iothread

 io/channel-tls.c  |  26 ++--
 io/channel-websock.c  |   1 +
 tests/qemu-iotests/tests/nbd-tls-iothread | 168 ++
 tests/qemu-iotests/tests/nbd-tls-iothread.out |  54 +
 4 files changed, 238 insertions(+), 11 deletions(-)
 create mode 100755 tests/qemu-iotests/tests/nbd-tls-iothread
 create mode 100644 tests/qemu-iotests/tests/nbd-tls-iothread.out

-- 
2.45.1

[PULL v2 1/2] qio: Inherit follow_coroutine_ctx across TLS

[Eric Blake]

Since qemu 8.2, the combination of NBD + TLS + iothread crashes on an
assertion failure:

qemu-kvm: ../io/channel.c:534: void qio_channel_restart_read(void *): Assertion 
`qemu_get_current_aio_context() == qemu_coroutine_get_aio_context(co)' failed.

It turns out that when we removed AioContext locking, we did so by
having NBD tell its qio channels that it wanted to opt in to
qio_channel_set_follow_coroutine_ctx(); but while we opted in on the
main channel, we did not opt in on the TLS wrapper channel.
qemu-iotests has coverage of NBD+iothread and NBD+TLS, but apparently
no coverage of NBD+TLS+iothread, or we would have noticed this
regression sooner.  (I'll add that in the next patch)

But while we could manually opt in to the TLS channel in nbd/server.c
(a one-line change), it is more generic if all qio channels that wrap
other channels inherit the follow status, in the same way that they
inherit feature bits.

CC: Stefan Hajnoczi 
CC: Daniel P. Berrangé 
CC: qemu-sta...@nongnu.org
Fixes: https://issues.redhat.com/browse/RHEL-34786
Fixes: 06e0f098 ("io: follow coroutine AioContext in qio_channel_yield()", 
v8.2.0)
Signed-off-by: Eric Blake 
Reviewed-by: Stefan Hajnoczi 
Reviewed-by: Daniel P. Berrangé 
Message-ID: <20240518025246.791593-5-ebl...@redhat.com>
---
 io/channel-tls.c | 26 +++---
 io/channel-websock.c |  1 +
 2 files changed, 16 insertions(+), 11 deletions(-)

diff --git a/io/channel-tls.c b/io/channel-tls.c
index 1d9c9c72bfb..67b9760 100644
--- a/io/channel-tls.c
+++ b/io/channel-tls.c
@@ -69,37 +69,40 @@ qio_channel_tls_new_server(QIOChannel *master,
const char *aclname,
Error **errp)
 {
-QIOChannelTLS *ioc;
+QIOChannelTLS *tioc;
+QIOChannel *ioc;

-ioc = QIO_CHANNEL_TLS(object_new(TYPE_QIO_CHANNEL_TLS));
+tioc = QIO_CHANNEL_TLS(object_new(TYPE_QIO_CHANNEL_TLS));
+ioc = QIO_CHANNEL(tioc);

-ioc->master = master;
+tioc->master = master;
+ioc->follow_coroutine_ctx = master->follow_coroutine_ctx;
 if (qio_channel_has_feature(master, QIO_CHANNEL_FEATURE_SHUTDOWN)) {
-qio_channel_set_feature(QIO_CHANNEL(ioc), 
QIO_CHANNEL_FEATURE_SHUTDOWN);
+qio_channel_set_feature(ioc, QIO_CHANNEL_FEATURE_SHUTDOWN);
 }
 object_ref(OBJECT(master));

-ioc->session = qcrypto_tls_session_new(
+tioc->session = qcrypto_tls_session_new(
 creds,
 NULL,
 aclname,
 QCRYPTO_TLS_CREDS_ENDPOINT_SERVER,
 errp);
-if (!ioc->session) {
+if (!tioc->session) {
 goto error;
 }

 qcrypto_tls_session_set_callbacks(
-ioc->session,
+tioc->session,
 qio_channel_tls_write_handler,
 qio_channel_tls_read_handler,
-ioc);
+tioc);

-trace_qio_channel_tls_new_server(ioc, master, creds, aclname);
-return ioc;
+trace_qio_channel_tls_new_server(tioc, master, creds, aclname);
+return tioc;

  error:
-object_unref(OBJECT(ioc));
+object_unref(OBJECT(tioc));
 return NULL;
 }

@@ -116,6 +119,7 @@ qio_channel_tls_new_client(QIOChannel *master,
 ioc = QIO_CHANNEL(tioc);

 tioc->master = master;
+ioc->follow_coroutine_ctx = master->follow_coroutine_ctx;
 if (qio_channel_has_feature(master, QIO_CHANNEL_FEATURE_SHUTDOWN)) {
 qio_channel_set_feature(ioc, QIO_CHANNEL_FEATURE_SHUTDOWN);
 }
diff --git a/io/channel-websock.c b/io/channel-websock.c
index a12acc27cf2..de39f0d182d 100644
--- a/io/channel-websock.c
+++ b/io/channel-websock.c
@@ -883,6 +883,7 @@ qio_channel_websock_new_server(QIOChannel *master)
 ioc = QIO_CHANNEL(wioc);

 wioc->master = master;
+ioc->follow_coroutine_ctx = master->follow_coroutine_ctx;
 if (qio_channel_has_feature(master, QIO_CHANNEL_FEATURE_SHUTDOWN)) {
 qio_channel_set_feature(ioc, QIO_CHANNEL_FEATURE_SHUTDOWN);
 }
-- 
2.45.1

[PATCH v3 1/2] qio: Inherit follow_coroutine_ctx across TLS

[Eric Blake]

Since qemu 8.2, the combination of NBD + TLS + iothread crashes on an
assertion failure:

qemu-kvm: ../io/channel.c:534: void qio_channel_restart_read(void *): Assertion 
`qemu_get_current_aio_context() == qemu_coroutine_get_aio_context(co)' failed.

It turns out that when we removed AioContext locking, we did so by
having NBD tell its qio channels that it wanted to opt in to
qio_channel_set_follow_coroutine_ctx(); but while we opted in on the
main channel, we did not opt in on the TLS wrapper channel.
qemu-iotests has coverage of NBD+iothread and NBD+TLS, but apparently
no coverage of NBD+TLS+iothread, or we would have noticed this
regression sooner.  (I'll add that in the next patch)

But while we could manually opt in to the TLS channel in nbd/server.c
(a one-line change), it is more generic if all qio channels that wrap
other channels inherit the follow status, in the same way that they
inherit feature bits.

CC: Stefan Hajnoczi 
CC: Daniel P. Berrangé 
CC: qemu-sta...@nongnu.org
Fixes: https://issues.redhat.com/browse/RHEL-34786
Fixes: 06e0f098 ("io: follow coroutine AioContext in qio_channel_yield()", 
v8.2.0)
Signed-off-by: Eric Blake 
Reviewed-by: Stefan Hajnoczi 
Reviewed-by: Daniel P. Berrangé 
Message-ID: <20240518025246.791593-5-ebl...@redhat.com>
---
 io/channel-tls.c | 26 +++---
 io/channel-websock.c |  1 +
 2 files changed, 16 insertions(+), 11 deletions(-)

diff --git a/io/channel-tls.c b/io/channel-tls.c
index 1d9c9c72bfb..67b9760 100644
--- a/io/channel-tls.c
+++ b/io/channel-tls.c
@@ -69,37 +69,40 @@ qio_channel_tls_new_server(QIOChannel *master,
const char *aclname,
Error **errp)
 {
-QIOChannelTLS *ioc;
+QIOChannelTLS *tioc;
+QIOChannel *ioc;

-ioc = QIO_CHANNEL_TLS(object_new(TYPE_QIO_CHANNEL_TLS));
+tioc = QIO_CHANNEL_TLS(object_new(TYPE_QIO_CHANNEL_TLS));
+ioc = QIO_CHANNEL(tioc);

-ioc->master = master;
+tioc->master = master;
+ioc->follow_coroutine_ctx = master->follow_coroutine_ctx;
 if (qio_channel_has_feature(master, QIO_CHANNEL_FEATURE_SHUTDOWN)) {
-qio_channel_set_feature(QIO_CHANNEL(ioc), 
QIO_CHANNEL_FEATURE_SHUTDOWN);
+qio_channel_set_feature(ioc, QIO_CHANNEL_FEATURE_SHUTDOWN);
 }
 object_ref(OBJECT(master));

-ioc->session = qcrypto_tls_session_new(
+tioc->session = qcrypto_tls_session_new(
 creds,
 NULL,
 aclname,
 QCRYPTO_TLS_CREDS_ENDPOINT_SERVER,
 errp);
-if (!ioc->session) {
+if (!tioc->session) {
 goto error;
 }

 qcrypto_tls_session_set_callbacks(
-ioc->session,
+tioc->session,
 qio_channel_tls_write_handler,
 qio_channel_tls_read_handler,
-ioc);
+tioc);

-trace_qio_channel_tls_new_server(ioc, master, creds, aclname);
-return ioc;
+trace_qio_channel_tls_new_server(tioc, master, creds, aclname);
+return tioc;

  error:
-object_unref(OBJECT(ioc));
+object_unref(OBJECT(tioc));
 return NULL;
 }

@@ -116,6 +119,7 @@ qio_channel_tls_new_client(QIOChannel *master,
 ioc = QIO_CHANNEL(tioc);

 tioc->master = master;
+ioc->follow_coroutine_ctx = master->follow_coroutine_ctx;
 if (qio_channel_has_feature(master, QIO_CHANNEL_FEATURE_SHUTDOWN)) {
 qio_channel_set_feature(ioc, QIO_CHANNEL_FEATURE_SHUTDOWN);
 }
diff --git a/io/channel-websock.c b/io/channel-websock.c
index a12acc27cf2..de39f0d182d 100644
--- a/io/channel-websock.c
+++ b/io/channel-websock.c
@@ -883,6 +883,7 @@ qio_channel_websock_new_server(QIOChannel *master)
 ioc = QIO_CHANNEL(wioc);

 wioc->master = master;
+ioc->follow_coroutine_ctx = master->follow_coroutine_ctx;
 if (qio_channel_has_feature(master, QIO_CHANNEL_FEATURE_SHUTDOWN)) {
 qio_channel_set_feature(ioc, QIO_CHANNEL_FEATURE_SHUTDOWN);
 }
-- 
2.45.1

[PATCH v3 0/2] Fix NBD+TLS regression in presence of iothread

[Eric Blake]

In v3:
- 2/2: fix iotest filtering [kwolf]

v2 was here:
https://lists.gnu.org/archive/html/qemu-devel/2024-05/msg03517.html

and this time, I'll wait for R-b before sending my v2 pull request:
https://lists.gnu.org/archive/html/qemu-devel/2024-05/msg06206.html

Eric Blake (2):
  qio: Inherit follow_coroutine_ctx across TLS
  iotests: test NBD+TLS+iothread

 io/channel-tls.c  |  26 +--
 io/channel-websock.c  |   1 +
 tests/qemu-iotests/tests/nbd-tls-iothread | 168 ++
 tests/qemu-iotests/tests/nbd-tls-iothread.out |  54 ++
 4 files changed, 238 insertions(+), 11 deletions(-)
 create mode 100755 tests/qemu-iotests/tests/nbd-tls-iothread
 create mode 100644 tests/qemu-iotests/tests/nbd-tls-iothread.out

-- 
2.45.1

[PATCH v3 2/2] iotests: test NBD+TLS+iothread

[Eric Blake]

Prevent regressions when using NBD with TLS in the presence of
iothreads, adding coverage the fix to qio channels made in the
previous patch.

The shell function pick_unused_port() was copied from
nbdkit.git/tests/functions.sh.in, where it had all authors from Red
Hat, agreeing to the resulting relicensing from 2-clause BSD to GPLv2.

CC: qemu-sta...@nongnu.org
CC: "Richard W.M. Jones" 
Signed-off-by: Eric Blake 
---
 tests/qemu-iotests/tests/nbd-tls-iothread | 168 ++
 tests/qemu-iotests/tests/nbd-tls-iothread.out |  54 ++
 2 files changed, 222 insertions(+)
 create mode 100755 tests/qemu-iotests/tests/nbd-tls-iothread
 create mode 100644 tests/qemu-iotests/tests/nbd-tls-iothread.out

diff --git a/tests/qemu-iotests/tests/nbd-tls-iothread 
b/tests/qemu-iotests/tests/nbd-tls-iothread
new file mode 100755
index 000..a2fb07206e5
--- /dev/null
+++ b/tests/qemu-iotests/tests/nbd-tls-iothread
@@ -0,0 +1,168 @@
+#!/usr/bin/env bash
+# group: rw quick
+#
+# Test of NBD+TLS+iothread
+#
+# Copyright (C) 2024 Red Hat, Inc.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+#
+
+# creator
+owner=ebl...@redhat.com
+
+seq=`basename $0`
+echo "QA output created by $seq"
+
+status=1# failure is the default!
+
+_cleanup()
+{
+_cleanup_qemu
+_cleanup_test_img
+rm -f "$dst_image"
+tls_x509_cleanup
+}
+trap "_cleanup; exit \$status" 0 1 2 3 15
+
+# get standard environment, filters and checks
+cd ..
+. ./common.rc
+. ./common.filter
+. ./common.qemu
+. ./common.tls
+. ./common.nbd
+
+_supported_fmt qcow2  # Hardcoded to qcow2 command line and QMP below
+_supported_proto file
+
+# pick_unused_port
+#
+# Picks and returns an "unused" port, setting the global variable
+# $port.
+#
+# This is inherently racy, but we need it because qemu does not currently
+# permit NBD+TLS over a Unix domain socket
+pick_unused_port ()
+{
+if ! (ss --version) >/dev/null 2>&1; then
+_notrun "ss utility required, skipped this test"
+fi
+
+# Start at a random port to make it less likely that two parallel
+# tests will conflict.
+port=$(( 5 + (RANDOM%15000) ))
+while ss -ltn | grep -sqE ":$port\b"; do
+((port++))
+if [ $port -eq 65000 ]; then port=5; fi
+done
+echo picked unused port
+}
+
+tls_x509_init
+
+size=1G
+DST_IMG="$TEST_DIR/dst.qcow2"
+
+echo
+echo "== preparing TLS creds and spare port =="
+
+pick_unused_port
+tls_x509_create_root_ca "ca1"
+tls_x509_create_server "ca1" "server1"
+tls_x509_create_client "ca1" "client1"
+tls_obj_base=tls-creds-x509,id=tls0,verify-peer=true,dir="${tls_dir}"
+
+echo
+echo "== preparing image =="
+
+_make_test_img $size
+$QEMU_IMG create -f qcow2 "$DST_IMG" $size | _filter_img_create
+
+echo
+echo === Starting Src QEMU ===
+echo
+
+_launch_qemu -machine q35 \
+-object iothread,id=iothread0 \
+-object "${tls_obj_base}"/client1,endpoint=client \
+-device '{"driver":"pcie-root-port", "id":"root0", "multifunction":true,
+  "bus":"pcie.0"}' \
+-device '{"driver":"virtio-scsi-pci", "id":"virtio_scsi_pci0",
+  "bus":"root0", "iothread":"iothread0"}' \
+-device '{"driver":"scsi-hd", "id":"image1", "drive":"drive_image1",
+  "bus":"virtio_scsi_pci0.0"}' \
+-blockdev '{"driver":"file", "cache":{"direct":true, "no-flush":false},
+"filename":"'"$TEST_IMG"'", "node-name":"drive_sys1"}' \
+-blockdev '{"driver":"qcow2", "node-name":"drive_image1",
+"file":"drive_sys1"}'
+h1=$QEMU_HANDLE
+_send_qemu_cmd $h1 '{"execute": "qmp_capabilities"}' 'return'
+
+echo
+echo === Starting Dst VM2 ===
+echo
+
+_launch_qemu -machine q35 \
+-object iothread,id=iothread0 \
+-object "${tls_obj_base}"/server1,endpoint=server \
+-dev

Re: [PULL 0/2] NBD patches for 2024-05-30

[Eric Blake]

On Thu, May 30, 2024 at 07:22:16AM GMT, Eric Blake wrote:
> The following changes since commit 3b2fe44bb7f605f179e5e7feb2c13c2eb3abbb80:
> 
>   Merge tag 'pull-request-2024-05-29' of https://gitlab.com/thuth/qemu into 
> staging (2024-05-29 08:38:20 -0700)
> 
> are available in the Git repository at:
> 
>   https://repo.or.cz/qemu/ericb.git tags/pull-nbd-2024-05-30
> 
> for you to fetch changes up to 109c59fa3a1cf6be88c2a39b4699a0041c64821f:
> 
>   iotests: test NBD+TLS+iothread (2024-05-30 07:18:42 -0500)
> 
> 
> NBD patches for 2024-05-30
> 
> - Fix AioContext assertion with NBD+TLS
> 
> ----
> Eric Blake (2):
>   qio: Inherit follow_coroutine_ctx across TLS
>   iotests: test NBD+TLS+iothread

Patch 2/2 only works on machines with /home/eblake; I will fix that in
a v2 pull request.

(And this should spur me to write a local commit hook that makes sure
I don't leak stuff like that in future commits...)

> 
>  io/channel-tls.c  |  26 ++--
>  io/channel-websock.c  |   1 +
>  tests/qemu-iotests/tests/nbd-tls-iothread | 168 
> ++
>  tests/qemu-iotests/tests/nbd-tls-iothread.out |  54 +
>  4 files changed, 238 insertions(+), 11 deletions(-)
>  create mode 100755 tests/qemu-iotests/tests/nbd-tls-iothread
>  create mode 100644 tests/qemu-iotests/tests/nbd-tls-iothread.out
> 
> -- 
> 2.45.1
> 
> 

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

Re: [PATCH v2 2/2] iotests: test NBD+TLS+iothread

[Eric Blake]

On Fri, May 31, 2024 at 04:36:20PM GMT, Kevin Wolf wrote:
> Am 18.05.2024 um 04:50 hat Eric Blake geschrieben:
> > Prevent regressions when using NBD with TLS in the presence of
> > iothreads, adding coverage the fix to qio channels made in the
> > previous patch.
> > 
> > CC: qemu-sta...@nongnu.org
> > Signed-off-by: Eric Blake 
> 
> > diff --git a/tests/qemu-iotests/tests/nbd-tls-iothread.out 
> > b/tests/qemu-iotests/tests/nbd-tls-iothread.out
> > new file mode 100644
> > index 000..b5e12dcbe7a
> > --- /dev/null
> > +++ b/tests/qemu-iotests/tests/nbd-tls-iothread.out

> > +Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1073741824
> > +Formatting 
> > '/home/eblake/qemu/build/tests/qemu-iotests/scratch/qcow2-file-nbd-tls-iothread/dst.qcow2',
> >  fmt=qcow2 cluster_size=65536 extended_l2=off compression_type=zlib 
> > size=1073741824 lazy_refcounts=off refcount_bits=16
> 
> /home/eblake is suboptimal in reference output. :-)

Indeed. Will fix, which means I need a v2 pull request.


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

Re: [PATCH v2 1/4] qom: allow to mark objects (including devices) as deprecated.

[Eric Blake]

On Thu, May 30, 2024 at 01:27:14PM GMT, Gerd Hoffmann wrote:
> Add deprecation_note field (string) to ObjectClass.
> Add deprecated bool to ObjectTypeInfo, report in 'qom-list-types'.
> Print the note when listing devices via '-device help'.

In the subject line, I suggest s/allow to mark/allow marking/

> 
> Signed-off-by: Gerd Hoffmann 
> ---
>  include/qom/object.h  | 1 +
>  qom/qom-qmp-cmds.c| 4 
>  system/qdev-monitor.c | 5 +
>  qapi/qom.json | 4 +++-
>  4 files changed, 13 insertions(+), 1 deletion(-)
> 

Reviewed-by: Eric Blake 

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

[PULL 0/2] NBD patches for 2024-05-30

[Eric Blake]

The following changes since commit 3b2fe44bb7f605f179e5e7feb2c13c2eb3abbb80:

  Merge tag 'pull-request-2024-05-29' of https://gitlab.com/thuth/qemu into 
staging (2024-05-29 08:38:20 -0700)

are available in the Git repository at:

  https://repo.or.cz/qemu/ericb.git tags/pull-nbd-2024-05-30

for you to fetch changes up to 109c59fa3a1cf6be88c2a39b4699a0041c64821f:

  iotests: test NBD+TLS+iothread (2024-05-30 07:18:42 -0500)


NBD patches for 2024-05-30

- Fix AioContext assertion with NBD+TLS


Eric Blake (2):
  qio: Inherit follow_coroutine_ctx across TLS
  iotests: test NBD+TLS+iothread

 io/channel-tls.c  |  26 ++--
 io/channel-websock.c  |   1 +
 tests/qemu-iotests/tests/nbd-tls-iothread | 168 ++
 tests/qemu-iotests/tests/nbd-tls-iothread.out |  54 +
 4 files changed, 238 insertions(+), 11 deletions(-)
 create mode 100755 tests/qemu-iotests/tests/nbd-tls-iothread
 create mode 100644 tests/qemu-iotests/tests/nbd-tls-iothread.out

-- 
2.45.1

[PULL 2/2] iotests: test NBD+TLS+iothread

[Eric Blake]

Prevent regressions when using NBD with TLS in the presence of
iothreads, adding coverage the fix to qio channels made in the
previous patch.

The shell function pick_unused_port() was copied from
nbdkit.git/tests/functions.sh.in, where it had all authors from Red
Hat, agreeing to the resulting relicensing from 2-clause BSD to GPLv2.

CC: qemu-sta...@nongnu.org
CC: "Richard W.M. Jones" 
Signed-off-by: Eric Blake 
Message-ID: <20240518025246.791593-6-ebl...@redhat.com>
---
 tests/qemu-iotests/tests/nbd-tls-iothread | 168 ++
 tests/qemu-iotests/tests/nbd-tls-iothread.out |  54 ++
 2 files changed, 222 insertions(+)
 create mode 100755 tests/qemu-iotests/tests/nbd-tls-iothread
 create mode 100644 tests/qemu-iotests/tests/nbd-tls-iothread.out

diff --git a/tests/qemu-iotests/tests/nbd-tls-iothread 
b/tests/qemu-iotests/tests/nbd-tls-iothread
new file mode 100755
index 000..26ccff8ddb7
--- /dev/null
+++ b/tests/qemu-iotests/tests/nbd-tls-iothread
@@ -0,0 +1,168 @@
+#!/usr/bin/env bash
+# group: rw quick
+#
+# Test of NBD+TLS+iothread
+#
+# Copyright (C) 2024 Red Hat, Inc.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+#
+
+# creator
+owner=ebl...@redhat.com
+
+seq=`basename $0`
+echo "QA output created by $seq"
+
+status=1# failure is the default!
+
+_cleanup()
+{
+_cleanup_qemu
+_cleanup_test_img
+rm -f "$dst_image"
+tls_x509_cleanup
+}
+trap "_cleanup; exit \$status" 0 1 2 3 15
+
+# get standard environment, filters and checks
+cd ..
+. ./common.rc
+. ./common.filter
+. ./common.qemu
+. ./common.tls
+. ./common.nbd
+
+_supported_fmt qcow2  # Hardcoded to qcow2 command line and QMP below
+_supported_proto file
+
+# pick_unused_port
+#
+# Picks and returns an "unused" port, setting the global variable
+# $port.
+#
+# This is inherently racy, but we need it because qemu does not currently
+# permit NBD+TLS over a Unix domain socket
+pick_unused_port ()
+{
+if ! (ss --version) >/dev/null 2>&1; then
+_notrun "ss utility required, skipped this test"
+fi
+
+# Start at a random port to make it less likely that two parallel
+# tests will conflict.
+port=$(( 5 + (RANDOM%15000) ))
+while ss -ltn | grep -sqE ":$port\b"; do
+((port++))
+if [ $port -eq 65000 ]; then port=5; fi
+done
+echo picked unused port
+}
+
+tls_x509_init
+
+size=1G
+DST_IMG="$TEST_DIR/dst.qcow2"
+
+echo
+echo "== preparing TLS creds and spare port =="
+
+pick_unused_port
+tls_x509_create_root_ca "ca1"
+tls_x509_create_server "ca1" "server1"
+tls_x509_create_client "ca1" "client1"
+tls_obj_base=tls-creds-x509,id=tls0,verify-peer=true,dir="${tls_dir}"
+
+echo
+echo "== preparing image =="
+
+_make_test_img $size
+$QEMU_IMG create -f qcow2 "$DST_IMG" $size
+
+echo
+echo === Starting Src QEMU ===
+echo
+
+_launch_qemu -machine q35 \
+-object iothread,id=iothread0 \
+-object "${tls_obj_base}"/client1,endpoint=client \
+-device '{"driver":"pcie-root-port", "id":"root0", "multifunction":true,
+  "bus":"pcie.0"}' \
+-device '{"driver":"virtio-scsi-pci", "id":"virtio_scsi_pci0",
+  "bus":"root0", "iothread":"iothread0"}' \
+-device '{"driver":"scsi-hd", "id":"image1", "drive":"drive_image1",
+  "bus":"virtio_scsi_pci0.0"}' \
+-blockdev '{"driver":"file", "cache":{"direct":true, "no-flush":false},
+"filename":"'"$TEST_IMG"'", "node-name":"drive_sys1"}' \
+-blockdev '{"driver":"qcow2", "node-name":"drive_image1",
+"file":"drive_sys1"}'
+h1=$QEMU_HANDLE
+_send_qemu_cmd $h1 '{"execute": "qmp_capabilities"}' 'return'
+
+echo
+echo === Starting Dst VM2 ===
+echo
+
+_launch_qemu -machine q35 \
+-object iothread,id=iothread0 \
+-object

[PULL 1/2] qio: Inherit follow_coroutine_ctx across TLS

[Eric Blake]

Since qemu 8.2, the combination of NBD + TLS + iothread crashes on an
assertion failure:

qemu-kvm: ../io/channel.c:534: void qio_channel_restart_read(void *): Assertion 
`qemu_get_current_aio_context() == qemu_coroutine_get_aio_context(co)' failed.

It turns out that when we removed AioContext locking, we did so by
having NBD tell its qio channels that it wanted to opt in to
qio_channel_set_follow_coroutine_ctx(); but while we opted in on the
main channel, we did not opt in on the TLS wrapper channel.
qemu-iotests has coverage of NBD+iothread and NBD+TLS, but apparently
no coverage of NBD+TLS+iothread, or we would have noticed this
regression sooner.  (I'll add that in the next patch)

But while we could manually opt in to the TLS channel in nbd/server.c
(a one-line change), it is more generic if all qio channels that wrap
other channels inherit the follow status, in the same way that they
inherit feature bits.

CC: Stefan Hajnoczi 
CC: Daniel P. Berrangé 
CC: qemu-sta...@nongnu.org
Fixes: https://issues.redhat.com/browse/RHEL-34786
Fixes: 06e0f098 ("io: follow coroutine AioContext in qio_channel_yield()", 
v8.2.0)
Signed-off-by: Eric Blake 
Reviewed-by: Stefan Hajnoczi 
Reviewed-by: Daniel P. Berrangé 
Message-ID: <20240518025246.791593-5-ebl...@redhat.com>
---
 io/channel-tls.c | 26 +++---
 io/channel-websock.c |  1 +
 2 files changed, 16 insertions(+), 11 deletions(-)

diff --git a/io/channel-tls.c b/io/channel-tls.c
index 1d9c9c72bfb..67b9760 100644
--- a/io/channel-tls.c
+++ b/io/channel-tls.c
@@ -69,37 +69,40 @@ qio_channel_tls_new_server(QIOChannel *master,
const char *aclname,
Error **errp)
 {
-QIOChannelTLS *ioc;
+QIOChannelTLS *tioc;
+QIOChannel *ioc;

-ioc = QIO_CHANNEL_TLS(object_new(TYPE_QIO_CHANNEL_TLS));
+tioc = QIO_CHANNEL_TLS(object_new(TYPE_QIO_CHANNEL_TLS));
+ioc = QIO_CHANNEL(tioc);

-ioc->master = master;
+tioc->master = master;
+ioc->follow_coroutine_ctx = master->follow_coroutine_ctx;
 if (qio_channel_has_feature(master, QIO_CHANNEL_FEATURE_SHUTDOWN)) {
-qio_channel_set_feature(QIO_CHANNEL(ioc), 
QIO_CHANNEL_FEATURE_SHUTDOWN);
+qio_channel_set_feature(ioc, QIO_CHANNEL_FEATURE_SHUTDOWN);
 }
 object_ref(OBJECT(master));

-ioc->session = qcrypto_tls_session_new(
+tioc->session = qcrypto_tls_session_new(
 creds,
 NULL,
 aclname,
 QCRYPTO_TLS_CREDS_ENDPOINT_SERVER,
 errp);
-if (!ioc->session) {
+if (!tioc->session) {
 goto error;
 }

 qcrypto_tls_session_set_callbacks(
-ioc->session,
+tioc->session,
 qio_channel_tls_write_handler,
 qio_channel_tls_read_handler,
-ioc);
+tioc);

-trace_qio_channel_tls_new_server(ioc, master, creds, aclname);
-return ioc;
+trace_qio_channel_tls_new_server(tioc, master, creds, aclname);
+return tioc;

  error:
-object_unref(OBJECT(ioc));
+object_unref(OBJECT(tioc));
 return NULL;
 }

@@ -116,6 +119,7 @@ qio_channel_tls_new_client(QIOChannel *master,
 ioc = QIO_CHANNEL(tioc);

 tioc->master = master;
+ioc->follow_coroutine_ctx = master->follow_coroutine_ctx;
 if (qio_channel_has_feature(master, QIO_CHANNEL_FEATURE_SHUTDOWN)) {
 qio_channel_set_feature(ioc, QIO_CHANNEL_FEATURE_SHUTDOWN);
 }
diff --git a/io/channel-websock.c b/io/channel-websock.c
index a12acc27cf2..de39f0d182d 100644
--- a/io/channel-websock.c
+++ b/io/channel-websock.c
@@ -883,6 +883,7 @@ qio_channel_websock_new_server(QIOChannel *master)
 ioc = QIO_CHANNEL(wioc);

 wioc->master = master;
+ioc->follow_coroutine_ctx = master->follow_coroutine_ctx;
 if (qio_channel_has_feature(master, QIO_CHANNEL_FEATURE_SHUTDOWN)) {
 qio_channel_set_feature(ioc, QIO_CHANNEL_FEATURE_SHUTDOWN);
 }
-- 
2.45.1

Re: [PATCH v2 2/2] iotests: test NBD+TLS+iothread

[Eric Blake]

Adding a bit of self-review (in case you want to amend this before
pushing, instead of waiting for me to get back online),

On Fri, May 17, 2024 at 09:50:15PM GMT, Eric Blake wrote:
> Prevent regressions when using NBD with TLS in the presence of
> iothreads, adding coverage the fix to qio channels made in the
> previous patch.
> 
> CC: qemu-sta...@nongnu.org
> Signed-off-by: Eric Blake 
> ---
>  tests/qemu-iotests/tests/nbd-tls-iothread | 170 ++
>  tests/qemu-iotests/tests/nbd-tls-iothread.out |  54 ++
>  2 files changed, 224 insertions(+)
>  create mode 100755 tests/qemu-iotests/tests/nbd-tls-iothread
>  create mode 100644 tests/qemu-iotests/tests/nbd-tls-iothread.out
> 
> diff --git a/tests/qemu-iotests/tests/nbd-tls-iothread 
> b/tests/qemu-iotests/tests/nbd-tls-iothread
> new file mode 100755
> index 000..a737224a90e
> --- /dev/null
> +++ b/tests/qemu-iotests/tests/nbd-tls-iothread
> @@ -0,0 +1,170 @@
> +#!/usr/bin/env bash
> +# group: rw quick
> +#
> +# Test of NBD+TLS+iothread
> +#
> +# Copyright (C) 2024 Red Hat, Inc.
> +#
> +# This program is free software; you can redistribute it and/or modify
> +# it under the terms of the GNU General Public License as published by
> +# the Free Software Foundation; either version 2 of the License, or
> +# (at your option) any later version.
> +#
> +# This program is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +# GNU General Public License for more details.
> +#
> +# You should have received a copy of the GNU General Public License
> +# along with this program.  If not, see <http://www.gnu.org/licenses/>.
> +#
> +
> +# creator
> +owner=ebl...@redhat.com
> +
> +seq=`basename $0`
> +echo "QA output created by $seq"
> +
> +status=1# failure is the default!
> +
> +_cleanup()
> +{
> +_cleanup_qemu
> +_cleanup_test_img
> +rm -f "$dst_image"
> +tls_x509_cleanup
> +}
> +trap "_cleanup; exit \$status" 0 1 2 3 15
> +
> +# get standard environment, filters and checks
> +cd ..
> +. ./common.rc
> +. ./common.filter
> +. ./common.qemu
> +. ./common.tls
> +. ./common.nbd
> +
> +_supported_fmt qcow2  # Hardcoded to qcow2 command line and QMP below
> +_supported_proto file
> +_require_command QEMU_NBD

This line can probably be dropped.  I originally included it thinking
I might reuse common.nbd's nbd_server_start_tcp_socket to pick an
unused port via a throwaway qemu-nbd, then kill the qemu-nbd process
before starting up the two qemu processes.  But in the end, using ss
to probe a port's use seems a bit more elegant than a throwaway
qemu-nbd process, although it may make CI testing harder by dragging
in another dependency that is less universal.

> +
> +# pick_unused_port
> +# Copied from nbdkit/tests/functions.sh.in with compatible 2-clause BSD 
> license

I'm not sure if I have to include the license text verbatim in this
file, and/or have this function moved to a helper utility file.  The
original source file that I borrowed pick_unused_port from has:

# Copyright Red Hat
#
# Redistribution and use in source and binary forms, with or without
# modification, are permitted provided that the following conditions are
# met:
#
# * Redistributions of source code must retain the above copyright
# notice, this list of conditions and the following disclaimer.
#
# * Redistributions in binary form must reproduce the above copyright
# notice, this list of conditions and the following disclaimer in the
# documentation and/or other materials provided with the distribution.
#
# * Neither the name of Red Hat nor the names of its contributors may be
# used to endorse or promote products derived from this software without
# specific prior written permission.
#
# THIS SOFTWARE IS PROVIDED BY RED HAT AND CONTRIBUTORS ''AS IS'' AND
# ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO,
# THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A
# PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL RED HAT OR
# CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
# SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
# LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF
# USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND
# ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY,
# OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT
# OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
# SUCH DAMAGE.


> +#
> +# Picks and returns an "unused" port, setting the global variable
> +# $port.
> +#
> +# This is inherently racy, but

Re: [PATCH v2 0/2] Fix NBD+TLS regression in presence of iothread

[Eric Blake]

On Fri, May 17, 2024 at 09:50:13PM GMT, Eric Blake wrote:
> In v2:
> - correct list email address
> - add iotest
> - add R-b
> 
> I'm offline next week, and have been communicating with Stefan who may
> want to push this through his block tree instead of waiting for me to
> get back.

I also meant to add that I did test that the iotest 2/2 fails unless
1/2 is applied.

> 
> Eric Blake (2):
>   qio: Inherit follow_coroutine_ctx across TLS
>   iotests: test NBD+TLS+iothread
> 
>  io/channel-tls.c  |  26 +--
>  io/channel-websock.c  |   1 +
>  tests/qemu-iotests/tests/nbd-tls-iothread | 170 ++
>  tests/qemu-iotests/tests/nbd-tls-iothread.out |  54 ++
>  4 files changed, 240 insertions(+), 11 deletions(-)
>  create mode 100755 tests/qemu-iotests/tests/nbd-tls-iothread
>  create mode 100644 tests/qemu-iotests/tests/nbd-tls-iothread.out
> 
> -- 
> 2.45.0
> 
> 

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

[PATCH v2 1/2] qio: Inherit follow_coroutine_ctx across TLS

[Eric Blake]

Since qemu 8.2, the combination of NBD + TLS + iothread crashes on an
assertion failure:

qemu-kvm: ../io/channel.c:534: void qio_channel_restart_read(void *): Assertion 
`qemu_get_current_aio_context() == qemu_coroutine_get_aio_context(co)' failed.

It turns out that when we removed AioContext locking, we did so by
having NBD tell its qio channels that it wanted to opt in to
qio_channel_set_follow_coroutine_ctx(); but while we opted in on the
main channel, we did not opt in on the TLS wrapper channel.
qemu-iotests has coverage of NBD+iothread and NBD+TLS, but apparently
no coverage of NBD+TLS+iothread, or we would have noticed this
regression sooner.  (I'll add that in the next patch)

But while we could manually opt in to the TLS channel in nbd/server.c
(a one-line change), it is more generic if all qio channels that wrap
other channels inherit the follow status, in the same way that they
inherit feature bits.

CC: Stefan Hajnoczi 
CC: Daniel P. Berrangé 
CC: qemu-sta...@nongnu.org
Fixes: https://issues.redhat.com/browse/RHEL-34786
Fixes: 06e0f098 ("io: follow coroutine AioContext in qio_channel_yield()", 
v8.2.0)
Signed-off-by: Eric Blake 
Reviewed-by: Stefan Hajnoczi 
Reviewed-by: Daniel P. Berrangé 

---
 io/channel-tls.c | 26 +++---
 io/channel-websock.c |  1 +
 2 files changed, 16 insertions(+), 11 deletions(-)

diff --git a/io/channel-tls.c b/io/channel-tls.c
index 1d9c9c72bfb..67b9760 100644
--- a/io/channel-tls.c
+++ b/io/channel-tls.c
@@ -69,37 +69,40 @@ qio_channel_tls_new_server(QIOChannel *master,
const char *aclname,
Error **errp)
 {
-QIOChannelTLS *ioc;
+QIOChannelTLS *tioc;
+QIOChannel *ioc;

-ioc = QIO_CHANNEL_TLS(object_new(TYPE_QIO_CHANNEL_TLS));
+tioc = QIO_CHANNEL_TLS(object_new(TYPE_QIO_CHANNEL_TLS));
+ioc = QIO_CHANNEL(tioc);

-ioc->master = master;
+tioc->master = master;
+ioc->follow_coroutine_ctx = master->follow_coroutine_ctx;
 if (qio_channel_has_feature(master, QIO_CHANNEL_FEATURE_SHUTDOWN)) {
-qio_channel_set_feature(QIO_CHANNEL(ioc), 
QIO_CHANNEL_FEATURE_SHUTDOWN);
+qio_channel_set_feature(ioc, QIO_CHANNEL_FEATURE_SHUTDOWN);
 }
 object_ref(OBJECT(master));

-ioc->session = qcrypto_tls_session_new(
+tioc->session = qcrypto_tls_session_new(
 creds,
 NULL,
 aclname,
 QCRYPTO_TLS_CREDS_ENDPOINT_SERVER,
 errp);
-if (!ioc->session) {
+if (!tioc->session) {
 goto error;
 }

 qcrypto_tls_session_set_callbacks(
-ioc->session,
+tioc->session,
 qio_channel_tls_write_handler,
 qio_channel_tls_read_handler,
-ioc);
+tioc);

-trace_qio_channel_tls_new_server(ioc, master, creds, aclname);
-return ioc;
+trace_qio_channel_tls_new_server(tioc, master, creds, aclname);
+return tioc;

  error:
-object_unref(OBJECT(ioc));
+object_unref(OBJECT(tioc));
 return NULL;
 }

@@ -116,6 +119,7 @@ qio_channel_tls_new_client(QIOChannel *master,
 ioc = QIO_CHANNEL(tioc);

 tioc->master = master;
+ioc->follow_coroutine_ctx = master->follow_coroutine_ctx;
 if (qio_channel_has_feature(master, QIO_CHANNEL_FEATURE_SHUTDOWN)) {
 qio_channel_set_feature(ioc, QIO_CHANNEL_FEATURE_SHUTDOWN);
 }
diff --git a/io/channel-websock.c b/io/channel-websock.c
index a12acc27cf2..de39f0d182d 100644
--- a/io/channel-websock.c
+++ b/io/channel-websock.c
@@ -883,6 +883,7 @@ qio_channel_websock_new_server(QIOChannel *master)
 ioc = QIO_CHANNEL(wioc);

 wioc->master = master;
+ioc->follow_coroutine_ctx = master->follow_coroutine_ctx;
 if (qio_channel_has_feature(master, QIO_CHANNEL_FEATURE_SHUTDOWN)) {
 qio_channel_set_feature(ioc, QIO_CHANNEL_FEATURE_SHUTDOWN);
 }
-- 
2.45.0

[PATCH v2 2/2] iotests: test NBD+TLS+iothread

[Eric Blake]

Prevent regressions when using NBD with TLS in the presence of
iothreads, adding coverage the fix to qio channels made in the
previous patch.

CC: qemu-sta...@nongnu.org
Signed-off-by: Eric Blake 
---
 tests/qemu-iotests/tests/nbd-tls-iothread | 170 ++
 tests/qemu-iotests/tests/nbd-tls-iothread.out |  54 ++
 2 files changed, 224 insertions(+)
 create mode 100755 tests/qemu-iotests/tests/nbd-tls-iothread
 create mode 100644 tests/qemu-iotests/tests/nbd-tls-iothread.out

diff --git a/tests/qemu-iotests/tests/nbd-tls-iothread 
b/tests/qemu-iotests/tests/nbd-tls-iothread
new file mode 100755
index 000..a737224a90e
--- /dev/null
+++ b/tests/qemu-iotests/tests/nbd-tls-iothread
@@ -0,0 +1,170 @@
+#!/usr/bin/env bash
+# group: rw quick
+#
+# Test of NBD+TLS+iothread
+#
+# Copyright (C) 2024 Red Hat, Inc.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+#
+
+# creator
+owner=ebl...@redhat.com
+
+seq=`basename $0`
+echo "QA output created by $seq"
+
+status=1# failure is the default!
+
+_cleanup()
+{
+_cleanup_qemu
+_cleanup_test_img
+rm -f "$dst_image"
+tls_x509_cleanup
+}
+trap "_cleanup; exit \$status" 0 1 2 3 15
+
+# get standard environment, filters and checks
+cd ..
+. ./common.rc
+. ./common.filter
+. ./common.qemu
+. ./common.tls
+. ./common.nbd
+
+_supported_fmt qcow2  # Hardcoded to qcow2 command line and QMP below
+_supported_proto file
+_require_command QEMU_NBD
+
+# pick_unused_port
+# Copied from nbdkit/tests/functions.sh.in with compatible 2-clause BSD license
+#
+# Picks and returns an "unused" port, setting the global variable
+# $port.
+#
+# This is inherently racy, but we need it because qemu does not currently
+# permit NBD+TLS over a Unix domain socket
+pick_unused_port ()
+{
+if ! (ss --version) >/dev/null 2>&1; then
+_notrun "ss utility required, skipped this test"
+fi
+
+# Start at a random port to make it less likely that two parallel
+# tests will conflict.
+port=$(( 5 + (RANDOM%15000) ))
+while ss -ltn | grep -sqE ":$port\b"; do
+((port++))
+if [ $port -eq 65000 ]; then port=5; fi
+done
+echo picked unused port
+}
+
+tls_x509_init
+
+size=1G
+DST_IMG="$TEST_DIR/dst.qcow2"
+
+echo
+echo "== preparing TLS creds and spare port =="
+
+pick_unused_port
+tls_x509_create_root_ca "ca1"
+tls_x509_create_server "ca1" "server1"
+tls_x509_create_client "ca1" "client1"
+tls_obj_base=tls-creds-x509,id=tls0,verify-peer=true,dir="${tls_dir}"
+
+echo
+echo "== preparing image =="
+
+_make_test_img $size
+$QEMU_IMG create -f qcow2 "$DST_IMG" $size
+
+echo
+echo === Starting Src QEMU ===
+echo
+
+_launch_qemu -machine q35 \
+-object iothread,id=iothread0 \
+-object "${tls_obj_base}"/client1,endpoint=client \
+-device '{"driver":"pcie-root-port", "id":"root0", "multifunction":true,
+  "bus":"pcie.0"}' \
+-device '{"driver":"virtio-scsi-pci", "id":"virtio_scsi_pci0",
+  "bus":"root0", "iothread":"iothread0"}' \
+-device '{"driver":"scsi-hd", "id":"image1", "drive":"drive_image1",
+  "bus":"virtio_scsi_pci0.0"}' \
+-blockdev '{"driver":"file", "cache":{"direct":true, "no-flush":false},
+"filename":"'"$TEST_IMG"'", "node-name":"drive_sys1"}' \
+-blockdev '{"driver":"qcow2", "node-name":"drive_image1",
+"file":"drive_sys1"}'
+h1=$QEMU_HANDLE
+_send_qemu_cmd $h1 '{"execute": "qmp_capabilities"}' 'return'
+
+echo
+echo === Starting Dst VM2 ===
+echo
+
+_launch_qemu -machine q35 \
+-object iothread,id=iothread0 \
+-object "${tls_obj_base}"/server1,endpoint=server \
+-device '{"driver":"pcie-root-port", "id":"root0", "multifunction":true,
+  "

[PATCH v2 0/2] Fix NBD+TLS regression in presence of iothread

[Eric Blake]

In v2:
- correct list email address
- add iotest
- add R-b

I'm offline next week, and have been communicating with Stefan who may
want to push this through his block tree instead of waiting for me to
get back.

Eric Blake (2):
  qio: Inherit follow_coroutine_ctx across TLS
  iotests: test NBD+TLS+iothread

 io/channel-tls.c  |  26 +--
 io/channel-websock.c  |   1 +
 tests/qemu-iotests/tests/nbd-tls-iothread | 170 ++
 tests/qemu-iotests/tests/nbd-tls-iothread.out |  54 ++
 4 files changed, 240 insertions(+), 11 deletions(-)
 create mode 100755 tests/qemu-iotests/tests/nbd-tls-iothread
 create mode 100644 tests/qemu-iotests/tests/nbd-tls-iothread.out

-- 
2.45.0

Re: [PATCH 01/14] include/hw: add helpers for defining versioned machine types

[Eric Blake]

On Wed, May 01, 2024 at 07:27:46PM +0100, Daniel P. Berrangé wrote:
> The various targets which define versioned machine types have
> a bunch of obfuscated macro code for defining unique function
> and variable names using string concatenation.
> 
> This addes a couple of helpers to improve the clarity of such
> code macro.
> 
> Signed-off-by: Daniel P. Berrangé 
> ---
>  include/hw/boards.h | 166 
>  1 file changed, 166 insertions(+)
> 
> diff --git a/include/hw/boards.h b/include/hw/boards.h
> index 2fa800f11a..47ca450fca 100644
> --- a/include/hw/boards.h
> +++ b/include/hw/boards.h
> @@ -414,6 +414,172 @@ struct MachineState {
>  struct NumaState *numa_state;
>  };
>  
> +/*
> + * The macros which follow are intended to facilitate the
> + * definition of versioned machine types, using a somewhat
> + * similar pattern across targets:
> + *
> + *  #define DEFINE_VIRT_MACHINE_IMPL(latest, ...) \
> + *  static void MACHINE_VER_SYM(class_init, virt, __VA_ARGS__)( \
> + *  ObjectClass *oc, \
> + *  void *data) \
> + *  { \
> + *  MachineClass *mc = MACHINE_CLASS(oc); \
> + *  MACHINE_VER_SYM(options, virt, __VA_ARGS__)(mc); \

Nice to include example usage of the macros.  __VA_ARGS__ is getting
expanded quite a few times here, but I don't see that as being too
much of a problem.

> + *  // Normal 2 digit eg 'virt-9.0'
> + *  #define DEFINE_VIRT_MACHINE(major, minor) \
> + *  DEFINE_VIRT_MACHINE_IMPL(false, major, minor)
> + *
> + *  // Bugfix 3 digit  eg 'virt-9.0.1'

Inconsistent on whether you are using one or two spaces before 'eg'.

> +
> +#define _MACHINE_VER_PICK(x1, x2, x3, x4, x5, x6, ...) x6

This helper macro is powerful; it may be worth a short comment, maybe
along the lines of:

/* Helper macro used to pick the right macro name based on the number
 * of extra arguments passed to the containing macro; usage:
 *
 *  _MACHINE_VER_PICK(__VA_ARGS__, \
 *MACRO_FOR_5_ARGS, \
 *MACRO_FOR_4_ARGS, \
 *MACRO_FOR_3_ARGS, \
 *MACRO_FOR_2_ARGS)(optional prefix args, __VA_ARGS__)
 */

But once understood, I see it comes in handy in several places below.

Reviewed-by: Eric Blake 

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

[PULL 2/2] nbd/server: Mark negotiation functions as coroutine_fn

[Eric Blake]

nbd_negotiate() is already marked coroutine_fn.  And given the fix in
the previous patch to have nbd_negotiate_handle_starttls not create
and wait on a g_main_loop (as that would violate coroutine
constraints), it is worth marking the rest of the related static
functions reachable only during option negotiation as also being
coroutine_fn.

Suggested-by: Vladimir Sementsov-Ogievskiy 
Signed-off-by: Eric Blake 
Message-ID: <20240408160214.1200629-6-ebl...@redhat.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy 
[eblake: drop one spurious coroutine_fn marking]
Signed-off-by: Eric Blake 
---
 nbd/server.c | 102 +--
 1 file changed, 59 insertions(+), 43 deletions(-)

diff --git a/nbd/server.c b/nbd/server.c
index 98ae0e16326..892797bb111 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -195,8 +195,9 @@ static inline void set_be_option_rep(NBDOptionReply *rep, 
uint32_t option,

 /* Send a reply header, including length, but no payload.
  * Return -errno on error, 0 on success. */
-static int nbd_negotiate_send_rep_len(NBDClient *client, uint32_t type,
-  uint32_t len, Error **errp)
+static coroutine_fn int
+nbd_negotiate_send_rep_len(NBDClient *client, uint32_t type,
+   uint32_t len, Error **errp)
 {
 NBDOptionReply rep;

@@ -211,15 +212,15 @@ static int nbd_negotiate_send_rep_len(NBDClient *client, 
uint32_t type,

 /* Send a reply header with default 0 length.
  * Return -errno on error, 0 on success. */
-static int nbd_negotiate_send_rep(NBDClient *client, uint32_t type,
-  Error **errp)
+static coroutine_fn int
+nbd_negotiate_send_rep(NBDClient *client, uint32_t type, Error **errp)
 {
 return nbd_negotiate_send_rep_len(client, type, 0, errp);
 }

 /* Send an error reply.
  * Return -errno on error, 0 on success. */
-static int G_GNUC_PRINTF(4, 0)
+static coroutine_fn int G_GNUC_PRINTF(4, 0)
 nbd_negotiate_send_rep_verr(NBDClient *client, uint32_t type,
 Error **errp, const char *fmt, va_list va)
 {
@@ -259,7 +260,7 @@ nbd_sanitize_name(const char *name)

 /* Send an error reply.
  * Return -errno on error, 0 on success. */
-static int G_GNUC_PRINTF(4, 5)
+static coroutine_fn int G_GNUC_PRINTF(4, 5)
 nbd_negotiate_send_rep_err(NBDClient *client, uint32_t type,
Error **errp, const char *fmt, ...)
 {
@@ -275,7 +276,7 @@ nbd_negotiate_send_rep_err(NBDClient *client, uint32_t type,
 /* Drop remainder of the current option, and send a reply with the
  * given error type and message. Return -errno on read or write
  * failure; or 0 if connection is still live. */
-static int G_GNUC_PRINTF(4, 0)
+static coroutine_fn int G_GNUC_PRINTF(4, 0)
 nbd_opt_vdrop(NBDClient *client, uint32_t type, Error **errp,
   const char *fmt, va_list va)
 {
@@ -288,7 +289,7 @@ nbd_opt_vdrop(NBDClient *client, uint32_t type, Error 
**errp,
 return ret;
 }

-static int G_GNUC_PRINTF(4, 5)
+static coroutine_fn int G_GNUC_PRINTF(4, 5)
 nbd_opt_drop(NBDClient *client, uint32_t type, Error **errp,
  const char *fmt, ...)
 {
@@ -302,7 +303,7 @@ nbd_opt_drop(NBDClient *client, uint32_t type, Error **errp,
 return ret;
 }

-static int G_GNUC_PRINTF(3, 4)
+static coroutine_fn int G_GNUC_PRINTF(3, 4)
 nbd_opt_invalid(NBDClient *client, Error **errp, const char *fmt, ...)
 {
 int ret;
@@ -319,8 +320,9 @@ nbd_opt_invalid(NBDClient *client, Error **errp, const char 
*fmt, ...)
  * If @check_nul, require that no NUL bytes appear in buffer.
  * Return -errno on I/O error, 0 if option was completely handled by
  * sending a reply about inconsistent lengths, or 1 on success. */
-static int nbd_opt_read(NBDClient *client, void *buffer, size_t size,
-bool check_nul, Error **errp)
+static coroutine_fn int
+nbd_opt_read(NBDClient *client, void *buffer, size_t size,
+ bool check_nul, Error **errp)
 {
 if (size > client->optlen) {
 return nbd_opt_invalid(client, errp,
@@ -343,7 +345,8 @@ static int nbd_opt_read(NBDClient *client, void *buffer, 
size_t size,
 /* Drop size bytes from the unparsed payload of the current option.
  * Return -errno on I/O error, 0 if option was completely handled by
  * sending a reply about inconsistent lengths, or 1 on success. */
-static int nbd_opt_skip(NBDClient *client, size_t size, Error **errp)
+static coroutine_fn int
+nbd_opt_skip(NBDClient *client, size_t size, Error **errp)
 {
 if (size > client->optlen) {
 return nbd_opt_invalid(client, errp,
@@ -366,8 +369,9 @@ static int nbd_opt_skip(NBDClient *client, size_t size, 
Error **errp)
  * Return -errno on I/O error, 0 if option was completely handled by
  * sending a reply about inconsistent lengths, or 1 on success.
  */
-static int nbd_opt_read_name(NBDClient *client, char **name, uint32_t *length,
- Error **errp)
+stati

[PULL 1/2] nbd/server: do not poll within a coroutine context

[Eric Blake]

From: Zhu Yangyang 

Coroutines are not supposed to block. Instead, they should yield.

The client performs TLS upgrade outside of an AIOContext, during
synchronous handshake; this still requires g_main_loop.  But the
server responds to TLS upgrade inside a coroutine, so a nested
g_main_loop is wrong.  Since the two callbacks no longer share more
than the setting of data.complete and data.error, it's just as easy to
use static helpers instead of trying to share a common code path.  It
is also possible to add assertions that no other code is interfering
with the eventual path to qio reaching the callback, whether or not it
required a yield or main loop.

Fixes: f95910f ("nbd: implement TLS support in the protocol negotiation")
Signed-off-by: Zhu Yangyang 
[eblake: move callbacks to their use point, add assertions]
Signed-off-by: Eric Blake 
Message-ID: <20240408160214.1200629-5-ebl...@redhat.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy 
---
 nbd/nbd-internal.h | 10 --
 nbd/client.c   | 28 
 nbd/common.c   | 11 ---
 nbd/server.c   | 28 +++-
 4 files changed, 47 insertions(+), 30 deletions(-)

diff --git a/nbd/nbd-internal.h b/nbd/nbd-internal.h
index dfa02f77ee4..91895106a95 100644
--- a/nbd/nbd-internal.h
+++ b/nbd/nbd-internal.h
@@ -72,16 +72,6 @@ static inline int nbd_write(QIOChannel *ioc, const void 
*buffer, size_t size,
 return qio_channel_write_all(ioc, buffer, size, errp) < 0 ? -EIO : 0;
 }

-struct NBDTLSHandshakeData {
-GMainLoop *loop;
-bool complete;
-Error *error;
-};
-
-
-void nbd_tls_handshake(QIOTask *task,
-   void *opaque);
-
 int nbd_drop(QIOChannel *ioc, size_t size, Error **errp);

 #endif
diff --git a/nbd/client.c b/nbd/client.c
index 29ffc609a4b..c89c7504673 100644
--- a/nbd/client.c
+++ b/nbd/client.c
@@ -596,13 +596,31 @@ static int nbd_request_simple_option(QIOChannel *ioc, int 
opt, bool strict,
 return 1;
 }

+/* Callback to learn when QIO TLS upgrade is complete */
+struct NBDTLSClientHandshakeData {
+bool complete;
+Error *error;
+GMainLoop *loop;
+};
+
+static void nbd_client_tls_handshake(QIOTask *task, void *opaque)
+{
+struct NBDTLSClientHandshakeData *data = opaque;
+
+qio_task_propagate_error(task, >error);
+data->complete = true;
+if (data->loop) {
+g_main_loop_quit(data->loop);
+}
+}
+
 static QIOChannel *nbd_receive_starttls(QIOChannel *ioc,
 QCryptoTLSCreds *tlscreds,
 const char *hostname, Error **errp)
 {
 int ret;
 QIOChannelTLS *tioc;
-struct NBDTLSHandshakeData data = { 0 };
+struct NBDTLSClientHandshakeData data = { 0 };

 ret = nbd_request_simple_option(ioc, NBD_OPT_STARTTLS, true, errp);
 if (ret <= 0) {
@@ -619,18 +637,20 @@ static QIOChannel *nbd_receive_starttls(QIOChannel *ioc,
 return NULL;
 }
 qio_channel_set_name(QIO_CHANNEL(tioc), "nbd-client-tls");
-data.loop = g_main_loop_new(g_main_context_default(), FALSE);
 trace_nbd_receive_starttls_tls_handshake();
 qio_channel_tls_handshake(tioc,
-  nbd_tls_handshake,
+  nbd_client_tls_handshake,
   ,
   NULL,
   NULL);

 if (!data.complete) {
+data.loop = g_main_loop_new(g_main_context_default(), FALSE);
 g_main_loop_run(data.loop);
+assert(data.complete);
+g_main_loop_unref(data.loop);
 }
-g_main_loop_unref(data.loop);
+
 if (data.error) {
 error_propagate(errp, data.error);
 object_unref(OBJECT(tioc));
diff --git a/nbd/common.c b/nbd/common.c
index 3247c1d618a..589a748cfe6 100644
--- a/nbd/common.c
+++ b/nbd/common.c
@@ -47,17 +47,6 @@ int nbd_drop(QIOChannel *ioc, size_t size, Error **errp)
 }


-void nbd_tls_handshake(QIOTask *task,
-   void *opaque)
-{
-struct NBDTLSHandshakeData *data = opaque;
-
-qio_task_propagate_error(task, >error);
-data->complete = true;
-g_main_loop_quit(data->loop);
-}
-
-
 const char *nbd_opt_lookup(uint32_t opt)
 {
 switch (opt) {
diff --git a/nbd/server.c b/nbd/server.c
index c3484cc1ebc..98ae0e16326 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -748,6 +748,23 @@ static int nbd_negotiate_handle_info(NBDClient *client, 
Error **errp)
 return rc;
 }

+/* Callback to learn when QIO TLS upgrade is complete */
+struct NBDTLSServerHandshakeData {
+bool complete;
+Error *error;
+Coroutine *co;
+};
+
+static void nbd_server_tls_handshake(QIOTask *task, void *opaque)
+{
+struct NBDTLSServerHandshakeData *data = opaque;
+
+qio_task_propagate_error(task, >error);
+data->complete = true;
+if (!qemu_coroutine_entered(data->co)) {
+aio_co_wake(data->

[PULL 0/2] NBD patches for 2024-04-25

[Eric Blake]

The following changes since commit 5da72194df36535d773c8bdc951529ecd5e31707:

  Merge tag 'pull-tcg-20240424' of https://gitlab.com/rth7680/qemu into staging 
(2024-04-24 15:51:49 -0700)

are available in the Git repository at:

  https://repo.or.cz/qemu/ericb.git tags/pull-nbd-2024-04-25

for you to fetch changes up to 4fa333e08dd96395a99ea8dd9e4c73a29dd23344:

  nbd/server: Mark negotiation functions as coroutine_fn (2024-04-25 12:59:19 
-0500)


NBD patches for 2024-04-25

- Avoid calling poll() within coroutine


Eric Blake (1):
  nbd/server: Mark negotiation functions as coroutine_fn

Zhu Yangyang (1):
  nbd/server: do not poll within a coroutine context

 nbd/nbd-internal.h |  10 -
 nbd/client.c   |  28 ++--
 nbd/common.c   |  11 -
 nbd/server.c   | 128 +
 4 files changed, 105 insertions(+), 72 deletions(-)

-- 
2.44.0

Re: [PATCH v3 09/13] block/gluster: Use URI parsing code from glib

[Eric Blake]

On Thu, Apr 18, 2024 at 12:10:52PM +0200, Thomas Huth wrote:
> Since version 2.66, glib has useful URI parsing functions, too.
> Use those instead of the QEMU-internal ones to be finally able
> to get rid of the latter.
> 
> Since g_uri_get_path() returns a const pointer, we also need to
> tweak the parameter of parse_volume_options() (where we use the
> result of g_uri_get_path() as input).
> 
> Reviewed-by: Eric Blake 
> Reviewed-by: Daniel P. Berrangé 
> Signed-off-by: Thomas Huth 
> ---
>  block/gluster.c | 71 -
>  1 file changed, 35 insertions(+), 36 deletions(-)
> 

> @@ -364,57 +363,57 @@ static int 
> qemu_gluster_parse_uri(BlockdevOptionsGluster *gconf,
>  QAPI_LIST_PREPEND(gconf->server, gsconf);
>  
>  /* transport */
> -if (!uri->scheme || !strcmp(uri->scheme, "gluster")) {
> +uri_scheme = g_uri_get_scheme(uri);
> +if (!uri_scheme || !strcmp(uri_scheme, "gluster")) {
>  gsconf->type = SOCKET_ADDRESS_TYPE_INET;

It may be worth a mention in the commit message that we are aware that
this provides a positive user-visible change as a side-effect: namely,
by virtue of using glib's parser (which normalizes the scheme to
lowercase) instead of our own (which did not), we now accept
GLUSTER:// URIs in addition to the usual gluster:// spelling.  Similar
comments to all the other affected patches in the series.

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

Re: [PATCH v3 04/13] tests: Update our CI to use CentOS Stream 9 instead of 8

[Eric Blake]

On Thu, Apr 18, 2024 at 12:10:47PM +0200, Thomas Huth wrote:
> RHEL 9 (and thus also the derivatives) are available since two years
> now, so according to QEMU's support policy, we can drop the active

Grammar suggestion:

RHEL 9 (and thus also the derivatives) have been available for two years now,

> support for the previous major version 8 now.
> 
> Another reason for doing this is that Centos Stream 8 will go EOL soon:
> 
> https://blog.centos.org/2023/04/end-dates-are-coming-for-centos-stream-8-and-centos-linux-7/
> 
>   "After May 31, 2024, CentOS Stream 8 will be archived
>and no further updates will be provided."
> 
> Thus upgrade our CentOS Stream container to major version 9 now.
> 
> Reviewed-by: Daniel P. Berrangé 
> Signed-off-by: Thomas Huth 

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

Re: [PATCH] tests/unit: Remove debug statements in test-nested-aio-poll.c

[Eric Blake]

On Fri, Apr 19, 2024 at 10:58:19AM +0200, Philippe Mathieu-Daudé wrote:
> We are running this test since almost a year; it is

Grammar suggestion:

We have been running this test for almost a year;

> safe to remove its debug statements, which clutter
> CI jobs output:
> 
>   ▶  88/100 /nested-aio-poll  OK
>   io_read 0x16bb26158
>   io_poll_true 0x16bb26158
>   > io_poll_ready
>   io_read 0x16bb26164
>   < io_poll_ready
>   io_poll_true 0x16bb26158
>   io_poll_false 0x16bb26164
>   > io_poll_ready
>   io_poll_false 0x16bb26164
>   io_poll_false 0x16bb26164
>   io_poll_false 0x16bb26164
>   io_poll_false 0x16bb26164
>   io_poll_false 0x16bb26164
>   io_poll_false 0x16bb26164
>   io_poll_false 0x16bb26164
>   io_poll_false 0x16bb26164
>   io_poll_false 0x16bb26164
>   io_read 0x16bb26164
>   < io_poll_ready
>   88/100 qemu:unit / test-nested-aio-pollOK
> 
> Signed-off-by: Philippe Mathieu-Daudé 
> ---
>  tests/unit/test-nested-aio-poll.c | 7 ---
>  1 file changed, 7 deletions(-)

Reviewed-by: Eric Blake 

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

Re: [PATCH v3 01/13] tests: Remove Ubuntu 20.04 container

[Eric Blake]

On Thu, Apr 18, 2024 at 12:10:44PM +0200, Thomas Huth wrote:
> Since Ubuntu 22.04 is now available since two years, we can stop

Grammar suggestion:

Since Ubuntu 22.04 has now been available for more than two years,

> actively supporting the previous LTS version of Ubuntu now.
> 
> Reviewed-by: Philippe Mathieu-Daudé 
> Signed-off-by: Thomas Huth 
> ---

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

MAINTAINERS tweak [was: [PATCH for-9.1 0/9] Switch to glib URI parsing code]

[Eric Blake]

[Trying Peter Lieven's alternate address...]

On Thu, Mar 28, 2024 at 03:05:57PM +0100, Thomas Huth wrote:
> In the QEMU 9.1 development cycle, we can drop the support for
> Ubuntu 20.04 and CentOS 8 since the following major versions of
> these distributions are available since 2 years already.

Every time I've replied to any message in this thread, I've gotten a
response:

| +Your message to p...@kamp.de couldn't be delivered.
| kamp.de couldn't confirm that your message was sent from a trusted location.
| eblake  Office 365  pl
| Action Required Recipient
| SPF validation error
| 
| How to Fix It
| Your organization's email admin will have to diagnose and fix your domain's 
email settings. Please forward this message to your
| +email admin.
| 
| 
| 
| More Info for Email Admins
| Status code: 550 5.7.23
| 
| This error occurs when Sender Policy Framework (SPF) validation for the 
sender's domain fails. If you're the sender's email
| +admin, make sure the SPF records for your domain at your domain registrar 
are set up correctly. Office 365 supports only one
| +SPF record (a TXT record that defines SPF) for your domain. Include the 
following domain name: spf.protection.outlook.com. If
| +you have a hybrid configuration (some mailboxes in the cloud, and some 
mailboxes on premises) or if you're an Exchange Online
| +Protection standalone customer, add the outbound IP address of your 
on-premises servers to the TXT record.

Red Hat IT says that it is unlikely to be Red Hat's SPF settings, and
suspects that it is instead something caused by whatever Peter is
using to bounce mail from his alias Peter Lieven  to his
Office 365 account.  As I appear to be unable to contact Peter (even
my use of direct email, bypassing the list, and using a personal
account instead of my Red Hat email) about this issue, I'm wondering
if Peter is still an active contributor to the project.

But while typing this email, to see if RBD, iSCSI, and NFS need a new
entry in MAINTAINERS, I did a search through the list archives, where
the last email I found from Peter was
https://lists.gnu.org/archive/html/qemu-devel/2023-01/msg00574.html,
which asked to update MAINTAINERS to his new address, and that has not
made it in so far...

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

Re: [PATCH] dma-helpers: Fix iovec alignment

[Eric Blake]

On Fri, Apr 12, 2024 at 10:06:17AM +0200, Stefan Fritsch wrote:
> Commit 99868af3d0 changed the hardcoded constant BDRV_SECTOR_SIZE to a
> dynamic field 'align' but introduced a bug. qemu_iovec_discard_back()
> is now passed the wanted iov length instead of the actually required
> amount that should be removed from the end of the iov.
> 
> The bug can likely only be hit in uncommon configurations, e.g. with
> icount enabled or when reading from disk directly to device memory.
> 
> Fixes: 99868af3d0a75cf6 ("dma-helpers: explicitly pass alignment into DMA 
> helpers")
> Signed-off-by: Stefan Fritsch 
> ---
>  system/dma-helpers.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)

Wow, that bug has been latent for a while (2016, v2.8.0).  As such, I
don't think it's worth holding up 9.0 for, but it is definitely worth
cc'ing qemu-stable (done now).

> 
> diff --git a/system/dma-helpers.c b/system/dma-helpers.c
> index 9b221cf94e..c9677fd39b 100644
> --- a/system/dma-helpers.c
> +++ b/system/dma-helpers.c
> @@ -174,8 +174,7 @@ static void dma_blk_cb(void *opaque, int ret)
>  }
>  
>  if (!QEMU_IS_ALIGNED(dbs->iov.size, dbs->align)) {
> -qemu_iovec_discard_back(>iov,
> -QEMU_ALIGN_DOWN(dbs->iov.size, dbs->align));
> +qemu_iovec_discard_back(>iov, dbs->iov.size % dbs->align);

Before the regression, it was:

-if (dbs->iov.size & ~BDRV_SECTOR_MASK) {
-qemu_iovec_discard_back(>iov, dbs->iov.size & ~BDRV_SECTOR_MASK);
+if (!QEMU_IS_ALIGNED(dbs->iov.size, dbs->align)) {
+qemu_iovec_discard_back(>iov,

If dbs->align is always a power of two, we can use '& (dbs->align -
1)' to avoid a hardware division, to match the original code; bug
QEMU_IS_ALIGNED does not require a power of two, so your choice of '%
dbs->align' seems reasonable.

Reviewed-by: Eric Blake 

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

Re: [PATCH v2 13/13] util/uri: Remove the old URI parsing code

[Eric Blake]

On Fri, Apr 12, 2024 at 03:24:15PM +0200, Thomas Huth wrote:
> Now that we switched all consumers of the URI code to use the URI
> parsing functions from glib instead, we can remove our internal
> URI parsing code since it is not used anymore.
> 
> Signed-off-by: Thomas Huth 
> ---
>  include/qemu/uri.h |   99 ---
>  util/uri.c | 1466 
>  util/meson.build   |2 +-
>  3 files changed, 1 insertion(+), 1566 deletions(-)
>  delete mode 100644 include/qemu/uri.h
>  delete mode 100644 util/uri.c

Nice.

Reviewed-by: Eric Blake 

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

Re: [PATCH v2 12/13] block/ssh: Use URI parsing code from glib

[Eric Blake]

On Fri, Apr 12, 2024 at 03:24:14PM +0200, Thomas Huth wrote:
> Since version 2.66, glib has useful URI parsing functions, too.
> Use those instead of the QEMU-internal ones to be finally able
> to get rid of the latter.
> 
> Reviewed-by: Richard W.M. Jones 
> Signed-off-by: Thomas Huth 
> ---
>  block/ssh.c | 75 -
>  1 file changed, 40 insertions(+), 35 deletions(-)
> 

>  
> -if (g_strcmp0(uri->scheme, "ssh") != 0) {
> +if (g_strcmp0(g_uri_get_scheme(uri), "ssh") != 0) {

Yet another case-sensitive spot to consider.

>  
> -qdict_put_str(options, "path", uri->path);
> -
> -/* Pick out any query parameters that we understand, and ignore
> - * the rest.
> - */
> -for (i = 0; i < qp->n; ++i) {
> -if (strcmp(qp->p[i].name, "host_key_check") == 0) {
> -qdict_put_str(options, "host_key_check", qp->p[i].value);
> +qdict_put_str(options, "path", uri_path);
> +
> +uri_query = g_uri_get_query(uri);
> +if (uri_query) {
> +g_uri_params_iter_init(, uri_query, -1, "&", G_URI_PARAMS_NONE);
> +while (g_uri_params_iter_next(, _name, _value, )) {
> +if (!qp_name || !qp_value || gerror) {
> +warn_report("Failed to parse SSH URI parameters '%s'.",
> +uri_query);
> +break;
> +}
> +/*
> + * Pick out the query parameters that we understand, and ignore
> + * (or rather warn about) the rest.
> + */
> +if (g_str_equal(qp_name, "host_key_check")) {
> +qdict_put_str(options, "host_key_check", qp_value);
> +} else {
> +warn_report("Unsupported parameter '%s' in URI.", qp_name);

Do we want the trailing '.' in warn_report?

The warning is new; it was not in the old code, nor mentioned in the
commit message.  It seems like a good idea, but we should be more
intentional if we intend to make that change.

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

Re: [PATCH v2 11/13] block/nfs: Use URI parsing code from glib

[Eric Blake]

On Fri, Apr 12, 2024 at 03:24:13PM +0200, Thomas Huth wrote:
> Since version 2.66, glib has useful URI parsing functions, too.
> Use those instead of the QEMU-internal ones to be finally able
> to get rid of the latter.
> 
> Signed-off-by: Thomas Huth 
> ---
>  block/nfs.c | 110 ++--
>  1 file changed, 54 insertions(+), 56 deletions(-)
> 

>  }
> -if (g_strcmp0(uri->scheme, "nfs") != 0) {
> +if (!g_str_equal(g_uri_get_scheme(uri), "nfs")) {

Another case where we should be considering whether g_ascii_strcasecmp
is better, as a separate patch.

> -for (i = 0; i < qp->n; i++) {
> -uint64_t val;
> -if (!qp->p[i].value) {
> -error_setg(errp, "Value for NFS parameter expected: %s",
> -   qp->p[i].name);
> -goto out;
> -}
> -if (parse_uint_full(qp->p[i].value, 0, )) {
> -error_setg(errp, "Illegal value for NFS parameter: %s",

Pre-existing,...

> +
> +uri_query = g_uri_get_query(uri);
> +if (uri_query) {
> +g_uri_params_iter_init(, uri_query, -1, "&", G_URI_PARAMS_NONE);
> +while (g_uri_params_iter_next(, _name, _value, )) {
> +uint64_t val;
> +if (!qp_name || gerror) {
> +error_setg(errp, "Failed to parse NFS parameter");
> +return -EINVAL;
> +}
> +if (!qp_value) {
> +error_setg(errp, "Value for NFS parameter expected: %s",
> +   qp_name);
> +return -EINVAL;
> +}
> +if (parse_uint_full(qp_value, 0, )) {
> +error_setg(errp, "Illegal value for NFS parameter: %s",

...but since we're touching it, I prefer 'Invalid' over 'Illegal' (any
error message implying that you broke a law when you passed in bad
data is a bit aggressive).  Not a show-stopper to leave it alone.

Reviewed-by: Eric Blake 

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

Re: [PATCH v2 10/13] block/nbd: Use URI parsing code from glib

[Eric Blake]

On Fri, Apr 12, 2024 at 03:24:12PM +0200, Thomas Huth wrote:
> Since version 2.66, glib has useful URI parsing functions, too.
> Use those instead of the QEMU-internal ones to be finally able
> to get rid of the latter. The g_uri_get_host() also takes care
> of removing the square brackets from IPv6 addresses, so we can
> drop that part of the QEMU code now, too.
> 
> Reviewed-by: Richard W.M. Jones 
> Signed-off-by: Thomas Huth 
> ---
>  block/nbd.c | 76 ++---
>  1 file changed, 37 insertions(+), 39 deletions(-)
> 

>  
>  /* transport */
> -if (!g_strcmp0(uri->scheme, "nbd")) {
> +uri_scheme = g_uri_get_scheme(uri);
> +if (!g_strcmp0(uri_scheme, "nbd")) {
>  is_unix = false;

As with gluster, we should probably be using g_ascii_strcasecmp() here
to match the RFC; again, worth a separate patch.

Reviewed-by: Eric Blake 

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

Re: [PATCH v2 09/13] block/gluster: Use URI parsing code from glib

[Eric Blake]

On Fri, Apr 12, 2024 at 09:40:18AM -0500, Eric Blake wrote:
> > @@ -364,57 +363,57 @@ static int 
> > qemu_gluster_parse_uri(BlockdevOptionsGluster *gconf,
> >  QAPI_LIST_PREPEND(gconf->server, gsconf);
> >  
> >  /* transport */
> > -if (!uri->scheme || !strcmp(uri->scheme, "gluster")) {
> > +uri_scheme = g_uri_get_scheme(uri);
> > +if (!uri_scheme || !strcmp(uri_scheme, "gluster")) {
> 
> Pre-existing, but per RFC 3986, we should probably be using strcasecmp
> for scheme comparisons (I'm not sure if g_uri_parse guarantees a
> lower-case return, even when the user passed in upper case).  That can
> be a separate patch.

Even beter, g_ascii_strcasecmp() (since strcasecmp can be
locale-specific which is generally not what we need here)


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

Re: [PATCH v2 09/13] block/gluster: Use URI parsing code from glib

[Eric Blake]

On Fri, Apr 12, 2024 at 03:24:11PM +0200, Thomas Huth wrote:
> Since version 2.66, glib has useful URI parsing functions, too.
> Use those instead of the QEMU-internal ones to be finally able
> to get rid of the latter.
> 
> Signed-off-by: Thomas Huth 
> ---
>  block/gluster.c | 71 -
>  1 file changed, 35 insertions(+), 36 deletions(-)
> 
> diff --git a/block/gluster.c b/block/gluster.c
> index cc74af06dc..1c9505f8bb 100644
> --- a/block/gluster.c
> +++ b/block/gluster.c
> @@ -17,7 +17,6 @@
>  #include "qapi/error.h"
>  #include "qapi/qmp/qdict.h"
>  #include "qapi/qmp/qerror.h"
> -#include "qemu/uri.h"
>  #include "qemu/error-report.h"
>  #include "qemu/module.h"
>  #include "qemu/option.h"
> @@ -289,9 +288,9 @@ static void glfs_clear_preopened(glfs_t *fs)
>  }
>  }
>  
> -static int parse_volume_options(BlockdevOptionsGluster *gconf, char *path)
> +static int parse_volume_options(BlockdevOptionsGluster *gconf, const char 
> *path)

Is it worth mentioning in the commit message that this includes a
const-correctness tweak?

> @@ -364,57 +363,57 @@ static int 
> qemu_gluster_parse_uri(BlockdevOptionsGluster *gconf,
>  QAPI_LIST_PREPEND(gconf->server, gsconf);
>  
>  /* transport */
> -if (!uri->scheme || !strcmp(uri->scheme, "gluster")) {
> +uri_scheme = g_uri_get_scheme(uri);
> +if (!uri_scheme || !strcmp(uri_scheme, "gluster")) {

Pre-existing, but per RFC 3986, we should probably be using strcasecmp
for scheme comparisons (I'm not sure if g_uri_parse guarantees a
lower-case return, even when the user passed in upper case).  That can
be a separate patch.

Reviewed-by: Eric Blake 

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

Re: [PATCH v5 2/2] nbd/server: Mark negotiation functions as coroutine_fn

[Eric Blake]

On Wed, Apr 10, 2024 at 10:05:28AM +0300, Vladimir Sementsov-Ogievskiy wrote:
> > > > @@ -755,7 +764,8 @@ struct NBDTLSServerHandshakeData {
> > > >Coroutine *co;
> > > >};
> > > > 
> > > > -static void nbd_server_tls_handshake(QIOTask *task, void *opaque)
> > > > +static coroutine_fn void
> > > 
> > > This is not, that's a callback for tls handshake, which is not coroutine 
> > > context as I understand.
> > 
> > The call chain is:
> > 
> > nbd_negotiate() - coroutine_fn before this patch
> >nbd_negotiate_options() - marked coroutine_fn here
> >  nbd_negotiate_handle_starttls() - marked coroutine_fn here
> >qio_channel_tls_handshake() - in io/channel-tls.c; not marked 
> > coroutine_fn, but
> >  works both in or out of coroutine 
> > context
> >  ...
> >  nbd_server_tls_handshake() - renamed in 1/2 of this series
> > 
> > > without this hunk:
> > 
> > I can drop that particular marking.  There are cases where the
> > callback is reached without having done the qemu_coroutine_yield() in
> > nbd_negotiate_handle_starttls; but there are also cases where the IO
> > took enough time that the coroutine yielded and it is that callback
> > that reawakens it.
> 
> The key thing for me is that in this case (when coroutine yielded), 
> nbd_server_tls_handshake() would finally be called from glib IO handler, set 
> in
> 
>qio_channel_tls_handshake()
>   qio_channel_tls_handshake_task()
>  qio_channel_add_watch_full()
> g_source_set_callback(source, (GSourceFunc)func, user_data, 
> notify);   <<<
> 
> , which would definitely not be a coroutine context.
> 
> 
> Do I understand correctly, that "coroutine_fn" means "only call from 
> coroutine context"[1], or does it mean "could be called from coroutine 
> context"[2]?

I'm always fuzzy on that distinction myself.  But reading the docs helps...


> 
> The comment in osdep.h says:
> 
>  * Mark a function that executes in coroutine context 
> |}
>  *
> |
>  * Functions that execute in coroutine context cannot be called directly from 
> |
>  * normal functions. ...
> 
> So I assume, we mean [1].

...[1] sounds more appropriate.  Adding the marker is more of a
promise that "I've audited that this function does not block and is
therefore safe for a function in coroutine context to use", but
sometimes additionally implies "this function assumes a coroutine is
present; if you are not in a coroutine, things might break".  But with
a bit of thought, it is obvious that a coroutine can call functions
that are not marked with coroutine_fn: any non-blocking syscall fits
in this category (since we don't have control over system headers to
add coroutine_fn annotations to those).  So on that grounds, it is
okay for a function marked coroutine_fn to call another function not
marked coroutine_fn - it just makes the auditing harder to ensure that
you aren't violating your promise of no non-blocking calls.

It's too close to 9.0-rc3 for my comfort to include this patch series.
Even though this bug can cause wedged migrations, I'd feel more
comfortable with preparing the pull request for this series (including
your fix for dropping this one annotation) for 9.1 and for qemu-stable
once the release is complete.

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

Re: [PATCH v5 2/2] nbd/server: Mark negotiation functions as coroutine_fn

[Eric Blake]

On Tue, Apr 09, 2024 at 09:30:39AM +0300, Vladimir Sementsov-Ogievskiy wrote:
> On 08.04.24 19:00, Eric Blake wrote:
> > nbd_negotiate() is already marked coroutine_fn.  And given the fix in
> > the previous patch to have nbd_negotiate_handle_starttls not create
> > and wait on a g_main_loop (as that would violate coroutine
> > constraints), it is worth marking the rest of the related static
> > functions reachable only during option negotiation as also being
> > coroutine_fn.
> > 
> > Suggested-by: Vladimir Sementsov-Ogievskiy 
> > Signed-off-by: Eric Blake 
> > ---
> >   nbd/server.c | 102 +--
> >   1 file changed, 59 insertions(+), 43 deletions(-)
> > 
> > diff --git a/nbd/server.c b/nbd/server.c
> > index 98ae0e16326..1857fba51c1 100644
> 
> [..]
> 
> >   {
> >   int rc;
> >   g_autofree char *name = NULL;
> > @@ -755,7 +764,8 @@ struct NBDTLSServerHandshakeData {
> >   Coroutine *co;
> >   };
> > 
> > -static void nbd_server_tls_handshake(QIOTask *task, void *opaque)
> > +static coroutine_fn void
> 
> This is not, that's a callback for tls handshake, which is not coroutine 
> context as I understand.

The call chain is:

nbd_negotiate() - coroutine_fn before this patch
  nbd_negotiate_options() - marked coroutine_fn here
nbd_negotiate_handle_starttls() - marked coroutine_fn here
  qio_channel_tls_handshake() - in io/channel-tls.c; not marked 
coroutine_fn, but
works both in or out of coroutine context
...
nbd_server_tls_handshake() - renamed in 1/2 of this series

> without this hunk:

I can drop that particular marking.  There are cases where the
callback is reached without having done the qemu_coroutine_yield() in
nbd_negotiate_handle_starttls; but there are also cases where the IO
took enough time that the coroutine yielded and it is that callback
that reawakens it.

> 
> 
> Reviewed-by: Vladimir Sementsov-Ogievskiy 

Thanks.

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

[PATCH v5 2/2] nbd/server: Mark negotiation functions as coroutine_fn

[Eric Blake]

nbd_negotiate() is already marked coroutine_fn.  And given the fix in
the previous patch to have nbd_negotiate_handle_starttls not create
and wait on a g_main_loop (as that would violate coroutine
constraints), it is worth marking the rest of the related static
functions reachable only during option negotiation as also being
coroutine_fn.

Suggested-by: Vladimir Sementsov-Ogievskiy 
Signed-off-by: Eric Blake 
---
 nbd/server.c | 102 +--
 1 file changed, 59 insertions(+), 43 deletions(-)

diff --git a/nbd/server.c b/nbd/server.c
index 98ae0e16326..1857fba51c1 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -195,8 +195,9 @@ static inline void set_be_option_rep(NBDOptionReply *rep, 
uint32_t option,

 /* Send a reply header, including length, but no payload.
  * Return -errno on error, 0 on success. */
-static int nbd_negotiate_send_rep_len(NBDClient *client, uint32_t type,
-  uint32_t len, Error **errp)
+static coroutine_fn int
+nbd_negotiate_send_rep_len(NBDClient *client, uint32_t type,
+   uint32_t len, Error **errp)
 {
 NBDOptionReply rep;

@@ -211,15 +212,15 @@ static int nbd_negotiate_send_rep_len(NBDClient *client, 
uint32_t type,

 /* Send a reply header with default 0 length.
  * Return -errno on error, 0 on success. */
-static int nbd_negotiate_send_rep(NBDClient *client, uint32_t type,
-  Error **errp)
+static coroutine_fn int
+nbd_negotiate_send_rep(NBDClient *client, uint32_t type, Error **errp)
 {
 return nbd_negotiate_send_rep_len(client, type, 0, errp);
 }

 /* Send an error reply.
  * Return -errno on error, 0 on success. */
-static int G_GNUC_PRINTF(4, 0)
+static coroutine_fn int G_GNUC_PRINTF(4, 0)
 nbd_negotiate_send_rep_verr(NBDClient *client, uint32_t type,
 Error **errp, const char *fmt, va_list va)
 {
@@ -259,7 +260,7 @@ nbd_sanitize_name(const char *name)

 /* Send an error reply.
  * Return -errno on error, 0 on success. */
-static int G_GNUC_PRINTF(4, 5)
+static coroutine_fn int G_GNUC_PRINTF(4, 5)
 nbd_negotiate_send_rep_err(NBDClient *client, uint32_t type,
Error **errp, const char *fmt, ...)
 {
@@ -275,7 +276,7 @@ nbd_negotiate_send_rep_err(NBDClient *client, uint32_t type,
 /* Drop remainder of the current option, and send a reply with the
  * given error type and message. Return -errno on read or write
  * failure; or 0 if connection is still live. */
-static int G_GNUC_PRINTF(4, 0)
+static coroutine_fn int G_GNUC_PRINTF(4, 0)
 nbd_opt_vdrop(NBDClient *client, uint32_t type, Error **errp,
   const char *fmt, va_list va)
 {
@@ -288,7 +289,7 @@ nbd_opt_vdrop(NBDClient *client, uint32_t type, Error 
**errp,
 return ret;
 }

-static int G_GNUC_PRINTF(4, 5)
+static coroutine_fn int G_GNUC_PRINTF(4, 5)
 nbd_opt_drop(NBDClient *client, uint32_t type, Error **errp,
  const char *fmt, ...)
 {
@@ -302,7 +303,7 @@ nbd_opt_drop(NBDClient *client, uint32_t type, Error **errp,
 return ret;
 }

-static int G_GNUC_PRINTF(3, 4)
+static coroutine_fn int G_GNUC_PRINTF(3, 4)
 nbd_opt_invalid(NBDClient *client, Error **errp, const char *fmt, ...)
 {
 int ret;
@@ -319,8 +320,9 @@ nbd_opt_invalid(NBDClient *client, Error **errp, const char 
*fmt, ...)
  * If @check_nul, require that no NUL bytes appear in buffer.
  * Return -errno on I/O error, 0 if option was completely handled by
  * sending a reply about inconsistent lengths, or 1 on success. */
-static int nbd_opt_read(NBDClient *client, void *buffer, size_t size,
-bool check_nul, Error **errp)
+static coroutine_fn int
+nbd_opt_read(NBDClient *client, void *buffer, size_t size,
+ bool check_nul, Error **errp)
 {
 if (size > client->optlen) {
 return nbd_opt_invalid(client, errp,
@@ -343,7 +345,8 @@ static int nbd_opt_read(NBDClient *client, void *buffer, 
size_t size,
 /* Drop size bytes from the unparsed payload of the current option.
  * Return -errno on I/O error, 0 if option was completely handled by
  * sending a reply about inconsistent lengths, or 1 on success. */
-static int nbd_opt_skip(NBDClient *client, size_t size, Error **errp)
+static coroutine_fn int
+nbd_opt_skip(NBDClient *client, size_t size, Error **errp)
 {
 if (size > client->optlen) {
 return nbd_opt_invalid(client, errp,
@@ -366,8 +369,9 @@ static int nbd_opt_skip(NBDClient *client, size_t size, 
Error **errp)
  * Return -errno on I/O error, 0 if option was completely handled by
  * sending a reply about inconsistent lengths, or 1 on success.
  */
-static int nbd_opt_read_name(NBDClient *client, char **name, uint32_t *length,
- Error **errp)
+static coroutine_fn int
+nbd_opt_read_name(NBDClient *client, char **name, uint32_t *length,
+  Error **errp)
 {
 int ret;
 uint32_t len;
@@ -402,8 +406,8 @@ static int nbd_o

[PATCH v5 1/2] nbd/server: do not poll within a coroutine context

[Eric Blake]

From: Zhu Yangyang 

Coroutines are not supposed to block. Instead, they should yield.

The client performs TLS upgrade outside of an AIOContext, during
synchronous handshake; this still requires g_main_loop.  But the
server responds to TLS upgrade inside a coroutine, so a nested
g_main_loop is wrong.  Since the two callbacks no longer share more
than the setting of data.complete and data.error, it's just as easy to
use static helpers instead of trying to share a common code path.  It
is also possible to add assertions that no other code is interfering
with the eventual path to qio reaching the callback, whether or not it
required a yield or main loop.

Fixes: f95910f ("nbd: implement TLS support in the protocol negotiation")
Signed-off-by: Zhu Yangyang 
[eblake: move callbacks to their use point, add assertions]
Signed-off-by: Eric Blake 
---
 nbd/nbd-internal.h | 10 --
 nbd/client.c   | 28 
 nbd/common.c   | 11 ---
 nbd/server.c   | 28 +++-
 4 files changed, 47 insertions(+), 30 deletions(-)

diff --git a/nbd/nbd-internal.h b/nbd/nbd-internal.h
index dfa02f77ee4..91895106a95 100644
--- a/nbd/nbd-internal.h
+++ b/nbd/nbd-internal.h
@@ -72,16 +72,6 @@ static inline int nbd_write(QIOChannel *ioc, const void 
*buffer, size_t size,
 return qio_channel_write_all(ioc, buffer, size, errp) < 0 ? -EIO : 0;
 }

-struct NBDTLSHandshakeData {
-GMainLoop *loop;
-bool complete;
-Error *error;
-};
-
-
-void nbd_tls_handshake(QIOTask *task,
-   void *opaque);
-
 int nbd_drop(QIOChannel *ioc, size_t size, Error **errp);

 #endif
diff --git a/nbd/client.c b/nbd/client.c
index 29ffc609a4b..c89c7504673 100644
--- a/nbd/client.c
+++ b/nbd/client.c
@@ -596,13 +596,31 @@ static int nbd_request_simple_option(QIOChannel *ioc, int 
opt, bool strict,
 return 1;
 }

+/* Callback to learn when QIO TLS upgrade is complete */
+struct NBDTLSClientHandshakeData {
+bool complete;
+Error *error;
+GMainLoop *loop;
+};
+
+static void nbd_client_tls_handshake(QIOTask *task, void *opaque)
+{
+struct NBDTLSClientHandshakeData *data = opaque;
+
+qio_task_propagate_error(task, >error);
+data->complete = true;
+if (data->loop) {
+g_main_loop_quit(data->loop);
+}
+}
+
 static QIOChannel *nbd_receive_starttls(QIOChannel *ioc,
 QCryptoTLSCreds *tlscreds,
 const char *hostname, Error **errp)
 {
 int ret;
 QIOChannelTLS *tioc;
-struct NBDTLSHandshakeData data = { 0 };
+struct NBDTLSClientHandshakeData data = { 0 };

 ret = nbd_request_simple_option(ioc, NBD_OPT_STARTTLS, true, errp);
 if (ret <= 0) {
@@ -619,18 +637,20 @@ static QIOChannel *nbd_receive_starttls(QIOChannel *ioc,
 return NULL;
 }
 qio_channel_set_name(QIO_CHANNEL(tioc), "nbd-client-tls");
-data.loop = g_main_loop_new(g_main_context_default(), FALSE);
 trace_nbd_receive_starttls_tls_handshake();
 qio_channel_tls_handshake(tioc,
-  nbd_tls_handshake,
+  nbd_client_tls_handshake,
   ,
   NULL,
   NULL);

 if (!data.complete) {
+data.loop = g_main_loop_new(g_main_context_default(), FALSE);
 g_main_loop_run(data.loop);
+assert(data.complete);
+g_main_loop_unref(data.loop);
 }
-g_main_loop_unref(data.loop);
+
 if (data.error) {
 error_propagate(errp, data.error);
 object_unref(OBJECT(tioc));
diff --git a/nbd/common.c b/nbd/common.c
index 3247c1d618a..589a748cfe6 100644
--- a/nbd/common.c
+++ b/nbd/common.c
@@ -47,17 +47,6 @@ int nbd_drop(QIOChannel *ioc, size_t size, Error **errp)
 }


-void nbd_tls_handshake(QIOTask *task,
-   void *opaque)
-{
-struct NBDTLSHandshakeData *data = opaque;
-
-qio_task_propagate_error(task, >error);
-data->complete = true;
-g_main_loop_quit(data->loop);
-}
-
-
 const char *nbd_opt_lookup(uint32_t opt)
 {
 switch (opt) {
diff --git a/nbd/server.c b/nbd/server.c
index c3484cc1ebc..98ae0e16326 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -748,6 +748,23 @@ static int nbd_negotiate_handle_info(NBDClient *client, 
Error **errp)
 return rc;
 }

+/* Callback to learn when QIO TLS upgrade is complete */
+struct NBDTLSServerHandshakeData {
+bool complete;
+Error *error;
+Coroutine *co;
+};
+
+static void nbd_server_tls_handshake(QIOTask *task, void *opaque)
+{
+struct NBDTLSServerHandshakeData *data = opaque;
+
+qio_task_propagate_error(task, >error);
+data->complete = true;
+if (!qemu_coroutine_entered(data->co)) {
+aio_co_wake(data->co);
+}
+}

 /* Handle NBD_OPT_STARTTLS. Return NULL to drop connection, or else the
  * new channel for all

[PATCH v5 for-9.0? 0/2] Fix NBD TLS poll in coroutine

[Eric Blake]

v4 was here:
https://lists.gnu.org/archive/html/qemu-devel/2024-04/msg00624.html

Since then: add some asserts [Vladimir], add second patch with more
coroutine_fn annotations [Vladimir]

Eric Blake (1):
  nbd/server: Mark negotiation functions as coroutine_fn

Zhu Yangyang (1):
  nbd/server: do not poll within a coroutine context

 nbd/nbd-internal.h |  10 
 nbd/client.c   |  28 --
 nbd/common.c   |  11 
 nbd/server.c   | 128 -
 4 files changed, 105 insertions(+), 72 deletions(-)

-- 
2.44.0

Re: [PATCH v4] nbd/server: do not poll within a coroutine context

[Eric Blake]

On Mon, Apr 08, 2024 at 11:46:39AM +0300, Vladimir Sementsov-Ogievskiy wrote:
> On 05.04.24 20:44, Eric Blake wrote:
> > From: Zhu Yangyang 
> > 
> > Coroutines are not supposed to block. Instead, they should yield.
> > 
> > The client performs TLS upgrade outside of an AIOContext, during
> > synchronous handshake; this still requires g_main_loop.  But the
> > server responds to TLS upgrade inside a coroutine, so a nested
> > g_main_loop is wrong.  Since the two callbacks no longer share more
> > than the setting of data.complete and data.error, it's just as easy to
> > use static helpers instead of trying to share a common code path.
> > 
> > Fixes: f95910f ("nbd: implement TLS support in the protocol negotiation")
> > Signed-off-by: Zhu Yangyang 
> > [eblake: move callbacks to their use point]
> > Signed-off-by: Eric Blake 
> 
> Reviewed-by: Vladimir Sementsov-Ogievskiy 

I'm debating whether it is worth trying to shove this into 9.0; -rc3
is very late, and the problem is pre-existing, so I'm leaning towards
no.  At which point, it's better to get this right.

> 
> still, some notes below
> 
> > ---
> > 
> > v3: https://lists.gnu.org/archive/html/qemu-devel/2024-04/msg00375.html
> > 
> > in v4, factor even the struct to the .c files, avoiding a union [Vladimir]
> > 
> >   nbd/nbd-internal.h | 10 --
> >   nbd/client.c   | 27 +++
> >   nbd/common.c   | 11 ---
> >   nbd/server.c   | 29 +++--
> >   4 files changed, 46 insertions(+), 31 deletions(-)
> > 

> > +++ b/nbd/client.c
> > @@ -596,13 +596,31 @@ static int nbd_request_simple_option(QIOChannel *ioc, 
> > int opt, bool strict,
> >   return 1;
> >   }
> > 
> > +/* Callback to learn when QIO TLS upgrade is complete */
> > +struct NBDTLSClientHandshakeData {
> > +bool complete;
> > +Error *error;
> > +GMainLoop *loop;
> > +};
> > +
> > +static void nbd_client_tls_handshake(QIOTask *task, void *opaque)
> > +{
> > +struct NBDTLSClientHandshakeData *data = opaque;
> > +
> > +qio_task_propagate_error(task, >error);
> > +data->complete = true;
> > +if (data->loop) {
> > +g_main_loop_quit(data->loop);
> > +}
> > +}
> > +
> >   static QIOChannel *nbd_receive_starttls(QIOChannel *ioc,
> >   QCryptoTLSCreds *tlscreds,
> >   const char *hostname, Error 
> > **errp)
> >   {
> >   int ret;
> >   QIOChannelTLS *tioc;
> > -struct NBDTLSHandshakeData data = { 0 };
> > +struct NBDTLSClientHandshakeData data = { 0 };
> > 
> >   ret = nbd_request_simple_option(ioc, NBD_OPT_STARTTLS, true, errp);
> >   if (ret <= 0) {
> > @@ -619,18 +637,19 @@ static QIOChannel *nbd_receive_starttls(QIOChannel 
> > *ioc,
> >   return NULL;
> >   }
> >   qio_channel_set_name(QIO_CHANNEL(tioc), "nbd-client-tls");
> > -data.loop = g_main_loop_new(g_main_context_default(), FALSE);
> >   trace_nbd_receive_starttls_tls_handshake();
> >   qio_channel_tls_handshake(tioc,
> > -  nbd_tls_handshake,
> > +  nbd_client_tls_handshake,
> > ,
> > NULL,
> > NULL);
> > 
> >   if (!data.complete) {
> > +data.loop = g_main_loop_new(g_main_context_default(), FALSE);
> >   g_main_loop_run(data.loop);
> > +g_main_loop_unref(data.loop);
> 
> probably good to assert(data.complete);

Seems reasonable.

> > +++ b/nbd/server.c
> > @@ -748,6 +748,23 @@ static int nbd_negotiate_handle_info(NBDClient 
> > *client, Error **errp)
> >   return rc;
> >   }
> > 
> > +/* Callback to learn when QIO TLS upgrade is complete */
> > +struct NBDTLSServerHandshakeData {
> > +bool complete;
> > +Error *error;
> > +Coroutine *co;
> > +};
> > +
> > +static void nbd_server_tls_handshake(QIOTask *task, void *opaque)
> > +{
> > +struct NBDTLSServerHandshakeData *data = opaque;
> > +
> > +qio_task_propagate_error(task, >error);
> > +data->complete = true;
> > +if (!qemu_coroutine_entered(data->co)) {
> > +aio_co_wake(data->co);
> > +}
> > +}
> > 
> >   /* Handle NBD_OPT_STARTTLS

[PATCH v4] nbd/server: do not poll within a coroutine context

[Eric Blake]

From: Zhu Yangyang 

Coroutines are not supposed to block. Instead, they should yield.

The client performs TLS upgrade outside of an AIOContext, during
synchronous handshake; this still requires g_main_loop.  But the
server responds to TLS upgrade inside a coroutine, so a nested
g_main_loop is wrong.  Since the two callbacks no longer share more
than the setting of data.complete and data.error, it's just as easy to
use static helpers instead of trying to share a common code path.

Fixes: f95910f ("nbd: implement TLS support in the protocol negotiation")
Signed-off-by: Zhu Yangyang 
[eblake: move callbacks to their use point]
Signed-off-by: Eric Blake 
---

v3: https://lists.gnu.org/archive/html/qemu-devel/2024-04/msg00375.html

in v4, factor even the struct to the .c files, avoiding a union [Vladimir]

 nbd/nbd-internal.h | 10 --
 nbd/client.c   | 27 +++
 nbd/common.c   | 11 ---
 nbd/server.c   | 29 +++--
 4 files changed, 46 insertions(+), 31 deletions(-)

diff --git a/nbd/nbd-internal.h b/nbd/nbd-internal.h
index dfa02f77ee4..91895106a95 100644
--- a/nbd/nbd-internal.h
+++ b/nbd/nbd-internal.h
@@ -72,16 +72,6 @@ static inline int nbd_write(QIOChannel *ioc, const void 
*buffer, size_t size,
 return qio_channel_write_all(ioc, buffer, size, errp) < 0 ? -EIO : 0;
 }

-struct NBDTLSHandshakeData {
-GMainLoop *loop;
-bool complete;
-Error *error;
-};
-
-
-void nbd_tls_handshake(QIOTask *task,
-   void *opaque);
-
 int nbd_drop(QIOChannel *ioc, size_t size, Error **errp);

 #endif
diff --git a/nbd/client.c b/nbd/client.c
index 29ffc609a4b..c7141d7a098 100644
--- a/nbd/client.c
+++ b/nbd/client.c
@@ -596,13 +596,31 @@ static int nbd_request_simple_option(QIOChannel *ioc, int 
opt, bool strict,
 return 1;
 }

+/* Callback to learn when QIO TLS upgrade is complete */
+struct NBDTLSClientHandshakeData {
+bool complete;
+Error *error;
+GMainLoop *loop;
+};
+
+static void nbd_client_tls_handshake(QIOTask *task, void *opaque)
+{
+struct NBDTLSClientHandshakeData *data = opaque;
+
+qio_task_propagate_error(task, >error);
+data->complete = true;
+if (data->loop) {
+g_main_loop_quit(data->loop);
+}
+}
+
 static QIOChannel *nbd_receive_starttls(QIOChannel *ioc,
 QCryptoTLSCreds *tlscreds,
 const char *hostname, Error **errp)
 {
 int ret;
 QIOChannelTLS *tioc;
-struct NBDTLSHandshakeData data = { 0 };
+struct NBDTLSClientHandshakeData data = { 0 };

 ret = nbd_request_simple_option(ioc, NBD_OPT_STARTTLS, true, errp);
 if (ret <= 0) {
@@ -619,18 +637,19 @@ static QIOChannel *nbd_receive_starttls(QIOChannel *ioc,
 return NULL;
 }
 qio_channel_set_name(QIO_CHANNEL(tioc), "nbd-client-tls");
-data.loop = g_main_loop_new(g_main_context_default(), FALSE);
 trace_nbd_receive_starttls_tls_handshake();
 qio_channel_tls_handshake(tioc,
-  nbd_tls_handshake,
+  nbd_client_tls_handshake,
   ,
   NULL,
   NULL);

 if (!data.complete) {
+data.loop = g_main_loop_new(g_main_context_default(), FALSE);
 g_main_loop_run(data.loop);
+g_main_loop_unref(data.loop);
 }
-g_main_loop_unref(data.loop);
+
 if (data.error) {
 error_propagate(errp, data.error);
 object_unref(OBJECT(tioc));
diff --git a/nbd/common.c b/nbd/common.c
index 3247c1d618a..589a748cfe6 100644
--- a/nbd/common.c
+++ b/nbd/common.c
@@ -47,17 +47,6 @@ int nbd_drop(QIOChannel *ioc, size_t size, Error **errp)
 }


-void nbd_tls_handshake(QIOTask *task,
-   void *opaque)
-{
-struct NBDTLSHandshakeData *data = opaque;
-
-qio_task_propagate_error(task, >error);
-data->complete = true;
-g_main_loop_quit(data->loop);
-}
-
-
 const char *nbd_opt_lookup(uint32_t opt)
 {
 switch (opt) {
diff --git a/nbd/server.c b/nbd/server.c
index c3484cc1ebc..ea13cf0e766 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -748,6 +748,23 @@ static int nbd_negotiate_handle_info(NBDClient *client, 
Error **errp)
 return rc;
 }

+/* Callback to learn when QIO TLS upgrade is complete */
+struct NBDTLSServerHandshakeData {
+bool complete;
+Error *error;
+Coroutine *co;
+};
+
+static void nbd_server_tls_handshake(QIOTask *task, void *opaque)
+{
+struct NBDTLSServerHandshakeData *data = opaque;
+
+qio_task_propagate_error(task, >error);
+data->complete = true;
+if (!qemu_coroutine_entered(data->co)) {
+aio_co_wake(data->co);
+}
+}

 /* Handle NBD_OPT_STARTTLS. Return NULL to drop connection, or else the
  * new channel for all further (now-encrypted) communication. */
@@ -756,7 +773,7 @@ static Q

Re: [PATCH v3] nbd/server: do not poll within a coroutine context

[Eric Blake]

On Fri, Apr 05, 2024 at 05:10:16PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> On 04.04.24 04:42, Eric Blake wrote:
> > From: Zhu Yangyang 
> > 
> > Coroutines are not supposed to block. Instead, they should yield.
> > 
> > The client performs TLS upgrade outside of an AIOContext, during
> > synchronous handshake; this still requires g_main_loop.  But the
> > server responds to TLS upgrade inside a coroutine, so a nested
> > g_main_loop is wrong.  Since the two callbacks no longer share more
> > than the setting of data.complete and data.error, it's just as easy to
> > use static helpers instead of trying to share a common code path.
> > 
> > Fixes: f95910f ("nbd: implement TLS support in the protocol negotiation")
> > Signed-off-by: Zhu Yangyang 
> > [eblake: move callbacks to their use point]
> > Signed-off-by: Eric Blake 
> > ---
> > 
> > After looking at this more, I'm less convinced that there is enough
> > common code here to even be worth trying to share in common.c.  This
> > takes the essence of the v2 patch, but refactors it a bit.
> 
> Maybe, do the complete split, and make separate structure definitions in 
> client.c and server.c, and don't make shared NBDTLSHandshakeData with union? 
> Finally, it's just a simple opaque-structure for static callback function, 
> seems good to keep it in .c as well.

Sure, v4 coming up along those lines.

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

[PATCH v3] nbd/server: do not poll within a coroutine context

[Eric Blake]

From: Zhu Yangyang 

Coroutines are not supposed to block. Instead, they should yield.

The client performs TLS upgrade outside of an AIOContext, during
synchronous handshake; this still requires g_main_loop.  But the
server responds to TLS upgrade inside a coroutine, so a nested
g_main_loop is wrong.  Since the two callbacks no longer share more
than the setting of data.complete and data.error, it's just as easy to
use static helpers instead of trying to share a common code path.

Fixes: f95910f ("nbd: implement TLS support in the protocol negotiation")
Signed-off-by: Zhu Yangyang 
[eblake: move callbacks to their use point]
Signed-off-by: Eric Blake 
---

After looking at this more, I'm less convinced that there is enough
common code here to even be worth trying to share in common.c.  This
takes the essence of the v2 patch, but refactors it a bit.

v2: https://lists.gnu.org/archive/html/qemu-devel/2024-04/msg00019.html

 nbd/nbd-internal.h | 20 ++--
 nbd/client.c   | 21 +
 nbd/common.c   | 11 ---
 nbd/server.c   | 21 -
 4 files changed, 43 insertions(+), 30 deletions(-)

diff --git a/nbd/nbd-internal.h b/nbd/nbd-internal.h
index dfa02f77ee4..087c6bfc002 100644
--- a/nbd/nbd-internal.h
+++ b/nbd/nbd-internal.h
@@ -63,6 +63,16 @@
 #define NBD_SET_TIMEOUT _IO(0xab, 9)
 #define NBD_SET_FLAGS   _IO(0xab, 10)

+/* Used in NBD_OPT_STARTTLS handling */
+struct NBDTLSHandshakeData {
+bool complete;
+Error *error;
+union {
+GMainLoop *loop;
+Coroutine *co;
+} u;
+};
+
 /* nbd_write
  * Writes @size bytes to @ioc. Returns 0 on success.
  */
@@ -72,16 +82,6 @@ static inline int nbd_write(QIOChannel *ioc, const void 
*buffer, size_t size,
 return qio_channel_write_all(ioc, buffer, size, errp) < 0 ? -EIO : 0;
 }

-struct NBDTLSHandshakeData {
-GMainLoop *loop;
-bool complete;
-Error *error;
-};
-
-
-void nbd_tls_handshake(QIOTask *task,
-   void *opaque);
-
 int nbd_drop(QIOChannel *ioc, size_t size, Error **errp);

 #endif
diff --git a/nbd/client.c b/nbd/client.c
index 29ffc609a4b..c9dc5265404 100644
--- a/nbd/client.c
+++ b/nbd/client.c
@@ -596,6 +596,18 @@ static int nbd_request_simple_option(QIOChannel *ioc, int 
opt, bool strict,
 return 1;
 }

+/* Callback to learn when QIO TLS upgrade is complete */
+static void nbd_client_tls_handshake(QIOTask *task, void *opaque)
+{
+struct NBDTLSHandshakeData *data = opaque;
+
+qio_task_propagate_error(task, >error);
+data->complete = true;
+if (data->u.loop) {
+g_main_loop_quit(data->u.loop);
+}
+}
+
 static QIOChannel *nbd_receive_starttls(QIOChannel *ioc,
 QCryptoTLSCreds *tlscreds,
 const char *hostname, Error **errp)
@@ -619,18 +631,19 @@ static QIOChannel *nbd_receive_starttls(QIOChannel *ioc,
 return NULL;
 }
 qio_channel_set_name(QIO_CHANNEL(tioc), "nbd-client-tls");
-data.loop = g_main_loop_new(g_main_context_default(), FALSE);
 trace_nbd_receive_starttls_tls_handshake();
 qio_channel_tls_handshake(tioc,
-  nbd_tls_handshake,
+  nbd_client_tls_handshake,
   ,
   NULL,
   NULL);

 if (!data.complete) {
-g_main_loop_run(data.loop);
+data.u.loop = g_main_loop_new(g_main_context_default(), FALSE);
+g_main_loop_run(data.u.loop);
+g_main_loop_unref(data.u.loop);
 }
-g_main_loop_unref(data.loop);
+
 if (data.error) {
 error_propagate(errp, data.error);
 object_unref(OBJECT(tioc));
diff --git a/nbd/common.c b/nbd/common.c
index 3247c1d618a..589a748cfe6 100644
--- a/nbd/common.c
+++ b/nbd/common.c
@@ -47,17 +47,6 @@ int nbd_drop(QIOChannel *ioc, size_t size, Error **errp)
 }


-void nbd_tls_handshake(QIOTask *task,
-   void *opaque)
-{
-struct NBDTLSHandshakeData *data = opaque;
-
-qio_task_propagate_error(task, >error);
-data->complete = true;
-g_main_loop_quit(data->loop);
-}
-
-
 const char *nbd_opt_lookup(uint32_t opt)
 {
 switch (opt) {
diff --git a/nbd/server.c b/nbd/server.c
index c3484cc1ebc..d16726a6326 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -748,6 +748,17 @@ static int nbd_negotiate_handle_info(NBDClient *client, 
Error **errp)
 return rc;
 }

+/* Callback to learn when QIO TLS upgrade is complete */
+static void nbd_server_tls_handshake(QIOTask *task, void *opaque)
+{
+struct NBDTLSHandshakeData *data = opaque;
+
+qio_task_propagate_error(task, >error);
+data->complete = true;
+if (!qemu_coroutine_entered(data->u.co)) {
+aio_co_wake(data->u.co);
+}
+}

 /* Handle NBD_OPT_STARTTLS. Return NULL to drop connection, or

Re: [PATCH 06/19] block/stream: fix -Werror=maybe-uninitialized false-positives

[Eric Blake]

On Wed, Apr 03, 2024 at 01:24:11PM +0400, Marc-André Lureau wrote:
> > > Unfortunately, it doesn't work in all cases. It seems to have issues
> > > with some guards:
> > > ../block/stream.c: In function ‘stream_run’:
> > > ../block/stream.c:216:12: error: ‘ret’ may be used uninitialized
> > > [-Werror=maybe-uninitialized]
> > >216 | if (ret < 0) {
> > >

That one looks like:

int ret;
WITH_GRAPH_RDLOCK_GUARD() {
  ret = ...;
}
if (copy) {
  ret = ...;
}
if (ret < 0)

I suspect the compiler is seeing the uncertainty possible from the
second conditional, and letting it take priority over the certainty
that the tweaked macro provided for the first conditional.

> > >
> >
> > So, updated macro helps in some cases, but doesn't help here? Intersting, 
> > why.
> >
> > > What should we do? change the macros + cherry-pick the missing
> > > false-positives, or keep this series as is?

An uglier macro, with sufficient comments as to why it is ugly (in
order to let us have fewer false positives where we have to add
initializers) may be less churn in the code base, but I'm not
necessarily sold on the ugly macro.  Let's see if anyone else
expresses an opinion.


> > >
> > >
> >
> > I think marco + missing is better. No reason to add dead-initializations in 
> > cases where new macros helps.
> 
> Ok
> 
> > Still, would be good to understand, what's the difference, why it help on 
> > some cases and not help in another.
> 
> I don't know, it's like if the analyzer was lazy for this particular
> case, although there is nothing much different from other usages.
> 
> If I replace:
> for (... *var2 = (void *)true; var2;
> with:
> for (... *var2 = (void *)true; var2 || true;
> 
> then it doesn't warn..

but it also doesn't work.  We want the body to execute exactly once,
not infloop.


> 
> Interestingly as well, if I change:
> for (... *var2 = (void *)true; var2; var2 = NULL)
> for:
> for (... *var2 = GML_OBJ_(); var2; var2 = NULL)
> 
> GML_OBJ_() simply being &(GraphLockable) { }), an empty compound
> literal, then it doesn't work, in all usages.

So the compiler is not figuring out that the compound literal is
sufficient for an unconditional one time through the for loop body.

What's worse, different compiler versions will change behavior over
time.  Making the code ugly to pacify a particular compiler, when that
compiler might improve in the future, is a form of chasing windmills.

> 
> All in all, I am not sure the trick of using var2 is really reliable either.

And that's my biggest argument for not making the macro not more
complex than it already is.

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

Re: [PATCH 06/19] block/stream: fix -Werror=maybe-uninitialized false-positives

[Eric Blake]

On Tue, Apr 02, 2024 at 12:58:43PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> > > Again, same false-positives, because of WITH_GRAPH_RDLOCK_GUARD()..
> > > 
> > > Didn't you try to change WITH_ macros somehow, so that compiler believe 
> > > in our good intentions?
> > > 
> > 
> > 
> > #define WITH_QEMU_LOCK_GUARD_(x, var) \
> >  for (g_autoptr(QemuLockable) var = \
> >  qemu_lockable_auto_lock(QEMU_MAKE_LOCKABLE_NONNULL((x))); \
> >   var; \
> >   qemu_lockable_auto_unlock(var), var = NULL)
> > 
> > I can't think of a clever way to rewrite this. The compiler probably
> > thinks the loop may not run, due to the "var" condition. But how to
> > convince it otherwise? it's hard to introduce another variable too..
> 
> 
> hmm. maybe like this?
> 
> #define WITH_QEMU_LOCK_GUARD_(x, var) \
> for (g_autoptr(QemuLockable) var = \
> qemu_lockable_auto_lock(QEMU_MAKE_LOCKABLE_NONNULL((x))), \
>  var2 = (void *)(true); \
>  var2; \
>  qemu_lockable_auto_unlock(var), var2 = NULL)
> 
> 
> probably, it would be simpler for compiler to understand the logic this way. 
> Could you check?

Wouldn't that attach __attribute__((cleanup(xxx))) to var2, at which
point we could cause the compiler to call xxx((void*)(true)) if the
user does an early return inside the lock guard, with disastrous
consequences?  Or is the __attribute__ applied only to the first out
of two declarations in a list?

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

Re: [PATCH v2 1/1] nbd/server: do not poll within a coroutine context

[Eric Blake]

dle_starttls(NBDClient *client,
>  
>  qio_channel_set_name(QIO_CHANNEL(tioc), "nbd-server-tls");
>  trace_nbd_negotiate_handle_starttls_handshake();
> -data.loop = g_main_loop_new(g_main_context_default(), FALSE);
> +data.co = qemu_coroutine_self();
>  qio_channel_tls_handshake(tioc,
> -  nbd_tls_handshake,
> +  nbd_server_tls_handshake,
>,
>NULL,
>NULL);
>  
> -    if (!data.complete) {
> -g_main_loop_run(data.loop);
> +while (!data.complete) {
> +qemu_coroutine_yield();
>  }
> -g_main_loop_unref(data.loop);
> +
>  if (data.error) {
>  object_unref(OBJECT(tioc));
>  error_propagate(errp, data.error);
> -- 
> 2.33.0
>

Thanks for the updated patch - it looks like we are heading in a good direction.

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

Re: [PATCH for-9.1 6/9] block/nbd: Use URI parsing code from glib

[Eric Blake]

Adjusting cc list to add upstream NBD and drop developers unrelated to
this part of the qemu series...

On Thu, Mar 28, 2024 at 02:13:42PM +, Richard W.M. Jones wrote:
> On Thu, Mar 28, 2024 at 03:06:03PM +0100, Thomas Huth wrote:
> > Since version 2.66, glib has useful URI parsing functions, too.
> > Use those instead of the QEMU-internal ones to be finally able
> > to get rid of the latter. The g_uri_get_host() also takes care
> > of removing the square brackets from IPv6 addresses, so we can
> > drop that part of the QEMU code now, too.
> > 

> >  
> >  if (is_unix) {
> >  /* nbd+unix:///export?socket=path */
> > -if (uri->server || uri->port || strcmp(qp->p[0].name, "socket")) {
> > +const char *uri_socket = g_hash_table_lookup(qp, "socket");
> > +if (uri_server || uri_port != -1 || !uri_socket) {
> >  ret = -EINVAL;
> >  goto out;
> >  }

The spec for NBD URIs is at:

https://github.com/NetworkBlockDevice/nbd/blob/master/doc/uri.md

Should any of this spec mention case-insensitive concerns, such as
whether 'NBD://' may be equivalent to 'nbd://', and whether
'nbd+unix:///?socket=x' is equivalent to 'nbd+unix:///?Socket=x'?
Right now, I think that most implementations of NBD servers and
clients happen to use case-sensitive parsing; but glib provides the
option to do case-insensitive query parsing.

If I read https://docs.gtk.org/glib/type_func.Uri.parse_params.html
correctly, passing G_URI_PARAMS_CASE_INSENSITIVE (which you did not
do) would mean that 'nbd+unix:///?socket=ignore=/for/real'
would result in this g_hash_table_lookup finding only "Socket", not
"socket".  Maybe it is worth an explicit addition to the NBD URI spec
to mention that we intend to be case-sensitive (in the parts where it
can be; I'm not sure if the schema part must be handled
case-insensitively without re-reading the RFCs), and therefore that
'Socket=' does NOT have the same meaning as 'socket='.

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

Re: [PATCH for-9.1 6/9] block/nbd: Use URI parsing code from glib

[Eric Blake]

On Thu, Mar 28, 2024 at 03:06:03PM +0100, Thomas Huth wrote:
> Since version 2.66, glib has useful URI parsing functions, too.
> Use those instead of the QEMU-internal ones to be finally able
> to get rid of the latter. The g_uri_get_host() also takes care
> of removing the square brackets from IPv6 addresses, so we can
> drop that part of the QEMU code now, too.
> 
> Signed-off-by: Thomas Huth 
> ---
>  block/nbd.c | 66 ++---
>  1 file changed, 38 insertions(+), 28 deletions(-)
> 
> diff --git a/block/nbd.c b/block/nbd.c
> index ef05f7cdfd..95b507f872 100644
> --- a/block/nbd.c
> +++ b/block/nbd.c
> @@ -31,7 +31,6 @@
>  #include "qemu/osdep.h"
>  
>  #include "trace.h"
> -#include "qemu/uri.h"
>  #include "qemu/option.h"
>  #include "qemu/cutils.h"
>  #include "qemu/main-loop.h"
> @@ -1514,30 +1513,34 @@ static void nbd_client_close(BlockDriverState *bs)
>  
>  static int nbd_parse_uri(const char *filename, QDict *options)
>  {
> -URI *uri;
> +GUri *uri;

Is it worth using 'g_autoptr(GUri) uri = NULL;' here, to simplify cleanup later?

>  const char *p;
> -QueryParams *qp = NULL;
> +GHashTable *qp = NULL;

Presumably would be easier if qp is also auto-free.

> +int qp_n;
>  int ret = 0;
>  bool is_unix;
> +const char *uri_scheme, *uri_query, *uri_server;
> +int uri_port;
>  
> -uri = uri_parse(filename);
> +uri = g_uri_parse(filename, G_URI_FLAGS_NONE, NULL);

The glib API is fairly close to what we have in qemu, making this a
nice switchover.

>  /* nbd[+tcp]://host[:port]/export */
> -if (!uri->server) {
> +if (!uri_server) {
>  ret = -EINVAL;
>  goto out;
>  }
>  
> -/* strip braces from literal IPv6 address */
> -if (uri->server[0] == '[') {
> -host = qstring_from_substr(uri->server, 1,
> -   strlen(uri->server) - 1);
> -} else {
> -host = qstring_from_str(uri->server);
> -}
> -
>  qdict_put_str(options, "server.type", "inet");
> -qdict_put(options, "server.host", host);
> +qdict_put_str(options, "server.host", uri_server);
>  
> -port_str = g_strdup_printf("%d", uri->port ?: NBD_DEFAULT_PORT);
> +port_str = g_strdup_printf("%d", uri_port != -1 ? uri_port
> +: NBD_DEFAULT_PORT);
>  qdict_put_str(options, "server.port", port_str);

If a user requests nbd://hostname:0/export, this now sets server.port
to "0" instead of "10809".  Is that an intentional change?  No one
actually passes an explicit ":0" port on purpose, but we do have to
worry about malicious URIs.

>  g_free(port_str);
>  }
>  
>  out:
>  if (qp) {
> -query_params_free(qp);
> +g_hash_table_destroy(qp);
>  }
> -uri_free(uri);
> +g_uri_unref(uri);
>  return ret;

It may be possible to eliminate the out label altogether, if
GHashTable has the appropriate auto-free magic.

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

Re: [PATCH 00/19] -Werror=maybe-uninitialized fixes

[Eric Blake]

On Thu, Mar 28, 2024 at 02:20:33PM +0400, marcandre.lur...@redhat.com wrote:
> From: Marc-André Lureau 
> 
> Hi,
> 
> Depending on -Doptimization=, GCC (13.2.1 here) produces different
> maybe-uninitialized warnings:
> - g: produces -Werror=maybe-uninitialized errors
> - 0: clean build
> - 1: produces -Werror=maybe-uninitialized errors
> - 2: clean build
> - 3: produces few -Werror=maybe-uninitialized errors
> - s: produces -Werror=maybe-uninitialized errors
> 
> Most are false-positive, because prior LOCK_GUARD should guarantee an
> initialization path. Few of them are a bit trickier. Finally, I found
> a potential related memory leak.
> 
> thanks

Couple of subject lines are inconsistent; I suggest:

> 
> Marc-André Lureau (19):
>   util/coroutine: fix -Werror=maybe-uninitialized false-positive
>   util/timer: with -Werror=maybe-uninitialized false-positive

s/with/fix/

>   hw/qxl: fix -Werror=maybe-uninitialized false-positives
>   nbd: with -Werror=maybe-uninitialized false-positive

s/with/fix/

>   block/mirror: fix -Werror=maybe-uninitialized false-positive
>   block/stream: fix -Werror=maybe-uninitialized false-positives
>   hw/ahci: fix -Werror=maybe-uninitialized false-positive
>   hw/vhost-scsi: fix -Werror=maybe-uninitialized
>   hw/sdhci: fix -Werror=maybe-uninitialized false-positive
>   hw/rdma: fix -Werror=maybe-uninitialized false-positive
>   migration/block: fix -Werror=maybe-uninitialized false-positive
>   migration: fix -Werror=maybe-uninitialized false-positives
>   hw/virtio-blk: fix -Werror=maybe-uninitialized false-positive
>   plugins: fix -Werror=maybe-uninitialized false-positive
>   migration: fix -Werror=maybe-uninitialized false-positive
>   tests: fix -Werror=maybe-uninitialized
>   hw/nvme: fix -Werror=maybe-uninitialized
>   hw/virtio: fix -Werror=maybe-uninitialized
>   RFC: hw/virtio: a potential leak fix
> 

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

Re: [PATCH 04/19] nbd: with -Werror=maybe-uninitialized false-positive

[Eric Blake]

On Thu, Mar 28, 2024 at 02:20:37PM +0400, marcandre.lur...@redhat.com wrote:
> From: Marc-André Lureau 
> 
> ../nbd/client-connection.c:419:8: error: ‘wait_co’ may be used uninitialized 
> [-Werror=maybe-uninitialized]
> 
> Signed-off-by: Marc-André Lureau 
> ---
>  nbd/client-connection.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Reviewed-by: Eric Blake 

> 
> diff --git a/nbd/client-connection.c b/nbd/client-connection.c
> index f9da67c87e..b11e266807 100644
> --- a/nbd/client-connection.c
> +++ b/nbd/client-connection.c
> @@ -410,7 +410,7 @@ nbd_co_establish_connection(NBDClientConnection *conn, 
> NBDExportInfo *info,
>   */
>  void nbd_co_establish_connection_cancel(NBDClientConnection *conn)
>  {
> -Coroutine *wait_co;
> +Coroutine *wait_co = NULL;
>  
>  WITH_QEMU_LOCK_GUARD(>mutex) {
>  wait_co = g_steal_pointer(>wait_co);
> -- 
> 2.44.0
> 

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

Re: [PATCH v1] coroutine: avoid inserting duplicate coroutine to co_queue_wakeup

[Eric Blake]

On Mon, Mar 25, 2024 at 05:18:50PM +0800, zhuyangyang via wrote:
> If g_main_loop_run()/aio_poll() is called in the coroutine context,
> the pending coroutine may be woken up repeatedly, and the co_queue_wakeup
> may be disordered.
> 
> When the poll() syscall exited in g_main_loop_run()/aio_poll(), it means
> some listened events is completed. Therefore, the completion callback
> function is dispatched.
> 
> If this callback function needs to invoke aio_co_enter(), it will only
> wake up the coroutine (because we are already in coroutine context),
> which may cause that the data on this listening event_fd/socket_fd
> is not read/cleared. When the next poll () exits, it will be woken up again
> and inserted into the wakeup queue again.
> 
> For example, if TLS is enabled in NBD, the server will call g_main_loop_run()
> in the coroutine, and repeatedly wake up the io_read event on a socket.
> The call stack is as follows:
> 
> aio_co_enter()
> aio_co_wake()
> qio_channel_restart_read()
> aio_dispatch_handler()
> aio_dispatch_handlers()
> aio_dispatch()
> aio_ctx_dispatch()
> g_main_context_dispatch()
> g_main_loop_run()
> nbd_negotiate_handle_starttls()
> nbd_negotiate_options()
> nbd_negotiate()
> nbd_co_client_start()
> coroutine_trampoline()

zhuyangyang, do you have a reliable reproduction setup for how you
were able to trigger this?  Obviously, it only happens when TLS is
enabled (we aren't creating a g_main_loop_run for any other NBD
command), and only when the server is first starting to serve a
client; is this a case where you were hammering a long-running qemu
process running an NBD server with multiple clients trying to
reconnect to the server all near the same time?

If we can come up with a reliable formula for reproducing the
corrupted coroutine list, it would make a great iotest addition
alongside the existing qemu-iotests 233 for ensuring that NBD TLS
traffic is handled correctly in both server and client.

> 
> Signed-off-by: zhuyangyang 

Side note: this appears to be your first qemu contribution (based on
'git shortlog --author zhuyangyang').  While I am not in a position to
presume how you would like your name Anglicized, I will point out that
the prevailing style is to separate given name from family name (just
because your username at work has no spaces does not mean that your
S-o-b has to follow suit).  It is also permissible to list your name
in native characters alongside or in place of the Anglicized version;
for example, 'git log --author="Stefano Dong"' shows this technique.

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

Re: [PATCH v1] coroutine: avoid inserting duplicate coroutine to co_queue_wakeup

[Eric Blake]

On Mon, Mar 25, 2024 at 11:50:41AM -0400, Stefan Hajnoczi wrote:
> On Mon, Mar 25, 2024 at 05:18:50PM +0800, zhuyangyang wrote:
> > If g_main_loop_run()/aio_poll() is called in the coroutine context,
> > the pending coroutine may be woken up repeatedly, and the co_queue_wakeup
> > may be disordered.
> 
> aio_poll() must not be called from coroutine context:
> 
>   bool no_coroutine_fn aio_poll(AioContext *ctx, bool blocking);
>^^^
> 
> Coroutines are not supposed to block. Instead, they should yield.
> 
> > When the poll() syscall exited in g_main_loop_run()/aio_poll(), it means
> > some listened events is completed. Therefore, the completion callback
> > function is dispatched.
> > 
> > If this callback function needs to invoke aio_co_enter(), it will only
> > wake up the coroutine (because we are already in coroutine context),
> > which may cause that the data on this listening event_fd/socket_fd
> > is not read/cleared. When the next poll () exits, it will be woken up again
> > and inserted into the wakeup queue again.
> > 
> > For example, if TLS is enabled in NBD, the server will call 
> > g_main_loop_run()
> > in the coroutine, and repeatedly wake up the io_read event on a socket.
> > The call stack is as follows:
> > 
> > aio_co_enter()
> > aio_co_wake()
> > qio_channel_restart_read()
> > aio_dispatch_handler()
> > aio_dispatch_handlers()
> > aio_dispatch()
> > aio_ctx_dispatch()
> > g_main_context_dispatch()
> > g_main_loop_run()
> > nbd_negotiate_handle_starttls()
> 
> This code does not look like it was designed to run in coroutine
> context. Two options:
> 
> 1. Don't run it in coroutine context (e.g. use a BH instead). This
>avoids blocking the coroutine but calling g_main_loop_run() is still
>ugly, in my opinion.
> 
> 2. Get rid of data.loop and use coroutine APIs instead:
> 
>while (!data.complete) {
>qemu_coroutine_yield();
>}
> 
>and update nbd_tls_handshake() to call aio_co_wake(data->co) instead
>of g_main_loop_quit(data->loop).
> 
>This requires auditing the code to check whether the event loop might
>invoke something that interferes with
>nbd_negotiate_handle_starttls(). Typically this means monitor
>commands or fd activity that could change the state of this
>connection while it is yielded. This is where the real work is but
>hopefully it will not be that hard to figure out.

I agree that 1) is ugly.  So far, I've added some temporary
assertions, to see when qio_channel_tls_handshake is reached; it looks
like nbd/server.c is calling it from within coroutine context, but
nbd/client.c is calling it from the main loop.  The qio code handles
either, but now I'm stuck in trying to get client.c into having the
right coroutine context; the TLS handshake is done before the usual
BlockDriverState *bs object is available, so I'm not even sure what
aio context, if any, I should be using.  But on my first try, I'm
hitting:

qemu-img: ../util/async.c:707: aio_co_enter: Assertion `self != co' failed.

so I obviously got something wrong.

It may be possible to use block_gen_c to turn nbd_receive_negotiate
and nbd_receive_export_list into co_wrapper functions, if I can audit
that all of their code goes through qio (and is therefore
coroutine-safe), but that work is still ongoing.

At any rate, qemu-iotest 233 should be good for testing that changes
in this area work; I've also been testing with libnbd (test
interop/interop-qemu-nbd-tls-certs hits qemu's server.c) and nbdkit
(test tests/test-tls-psk.sh hits qemu's client.c).

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

Re: [PATCH for-9.1 v2 04/11] vhost-user-server: don't abort if we can't set fd non-blocking

[Eric Blake]

On Tue, Mar 26, 2024 at 02:39:29PM +0100, Stefano Garzarella wrote:
> In vhost-user-server we set all fd received from the other peer
> in non-blocking mode. For some of them (e.g. memfd, shm_open, etc.)
> if we fail, it's not really a problem, because we don't use these
> fd with blocking operations, but only to map memory.
> 
> In these cases a failure is not bad, so let's just report a warning
> instead of panicking if we fail to set some fd in non-blocking mode.
> 
> This for example occurs in macOS where setting shm_open() fd
> non-blocking is failing (errno: 25).

What is errno 25 on MacOS?

> 
> Signed-off-by: Stefano Garzarella 
> ---
>  util/vhost-user-server.c | 6 +-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/util/vhost-user-server.c b/util/vhost-user-server.c
> index 3bfb1ad3ec..064999f0b7 100644
> --- a/util/vhost-user-server.c
> +++ b/util/vhost-user-server.c
> @@ -66,7 +66,11 @@ static void vmsg_unblock_fds(VhostUserMsg *vmsg)
>  {
>  int i;
>  for (i = 0; i < vmsg->fd_num; i++) {
> -qemu_socket_set_nonblock(vmsg->fds[i]);
> +int ret = qemu_socket_try_set_nonblock(vmsg->fds[i]);
> +if (ret) {

Should this be 'if (ret < 0)'?

> +warn_report("Failed to set fd %d nonblock for request %d: %s",
> +vmsg->fds[i], vmsg->request, strerror(-ret));
> +}

This now ignores all errors even on pre-existing fds where we NEED
non-blocking, rather than just the specific (expected) error we are
seeing on MacOS.  Should this code be a bit more precise about
checking that -ret == EXXX for the expected errno value we are
ignoring for the specific fds where non-blocking is not essential?

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

Re: [PATCH for-9.1 v2 03/11] libvhost-user: mask F_INFLIGHT_SHMFD if memfd is not supported

[Eric Blake]

On Tue, Mar 26, 2024 at 02:39:28PM +0100, Stefano Garzarella wrote:
> libvhost-user will panic when receiving VHOST_USER_GET_INFLIGHT_FD
> message if MFD_ALLOW_SEALING is not defined, since it's not able
> to create a memfd.
> 
> VHOST_USER_GET_INFLIGHT_FD is used only if
> VHOST_USER_PROTOCOL_F_INFLIGHT_SHMFD is negotiated. So, let's mask
> that feature if the backend is not able to properly handle these
> messages.
> 
> Signed-off-by: Stefano Garzarella 
> ---
>  subprojects/libvhost-user/libvhost-user.c | 10 ++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/subprojects/libvhost-user/libvhost-user.c 
> b/subprojects/libvhost-user/libvhost-user.c
> index a11afd1960..1c361ffd51 100644
> --- a/subprojects/libvhost-user/libvhost-user.c
> +++ b/subprojects/libvhost-user/libvhost-user.c
> @@ -1674,6 +1674,16 @@ vu_get_protocol_features_exec(VuDev *dev, VhostUserMsg 
> *vmsg)
>  features |= dev->iface->get_protocol_features(dev);
>  }
>  
> +/*
> + * If MFD_ALLOW_SEALING is not defined, we are not able to handle
> + * VHOST_USER_GET_INFLIGHT_FD messages, since we can't create a memfd.
> + * Those messages are used only if VHOST_USER_PROTOCOL_F_INFLIGHT_SHMFD
> + * is negotiated. A device implementation can enable it, so let's mask
> + * it to avoid a runtime panic.
> + */
> +#ifndef MFD_ALLOW_SEALING
> +features &= ~(1ULL << VHOST_USER_PROTOCOL_F_INFLIGHT_SHMFD);
> +#endif

Masking the feature out of advertisement is obviously correct. But
should we also fix the code for handling
VHOST_USER_PROTOCOL_F_INFLIGHT_SHMFD to return an error to any client
that requests it in error when the feature was not advertised, instead
of panicking?

>  vmsg_set_reply_u64(vmsg, features);
>  return true;
>  }
> -- 
> 2.44.0
> 

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

Re: [PATCH for-9.1 v2 02/11] libvhost-user: fail vu_message_write() if sendmsg() is failing

[Eric Blake]

On Tue, Mar 26, 2024 at 02:39:27PM +0100, Stefano Garzarella wrote:
> In vu_message_write() we use sendmsg() to send the message header,
> then a write() to send the payload.
> 
> If sendmsg() fails we should avoid sending the payload, since we
> were unable to send the header.
> 
> Discovered before fixing the issue with the previous patch, where
> sendmsg() failed on macOS due to wrong parameters, but the frontend
> still sent the payload which the backend incorrectly interpreted
> as a wrong header.
> 
> Signed-off-by: Stefano Garzarella 
> ---
>  subprojects/libvhost-user/libvhost-user.c | 5 +
>  1 file changed, 5 insertions(+)
> 
> diff --git a/subprojects/libvhost-user/libvhost-user.c 
> b/subprojects/libvhost-user/libvhost-user.c
> index 22bea0c775..a11afd1960 100644
> --- a/subprojects/libvhost-user/libvhost-user.c
> +++ b/subprojects/libvhost-user/libvhost-user.c
> @@ -639,6 +639,11 @@ vu_message_write(VuDev *dev, int conn_fd, VhostUserMsg 
> *vmsg)
>  rc = sendmsg(conn_fd, , 0);
>  } while (rc < 0 && (errno == EINTR || errno == EAGAIN));
>  
> +if (rc <= 0) {

Is rejecting a 0 return value correct?  Technically, a 0 return means
a successful write of 0 bytes - but then again, it is unwise to
attempt to send an fd or other auxilliary ddata without at least one
regular byte, so we should not be attempting a write of 0 bytes.  So I
guess this one is okay, although I might have used < instead of <=.

> +vu_panic(dev, "Error while writing: %s", strerror(errno));
> +return false;
> +}

At any rate, noticing the error is the correct thing to do.

> +
>  if (vmsg->size) {
>  do {
>  if (vmsg->data) {
> -- 
> 2.44.0
> 

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

Re: [PATCH for-9.1 v2 01/11] libvhost-user: set msg.msg_control to NULL when it is empty

[Eric Blake]

On Tue, Mar 26, 2024 at 02:39:26PM +0100, Stefano Garzarella wrote:
> On some OS (e.g. macOS) sendmsg() returns -1 (errno EINVAL) if
> the `struct msghdr` has the field `msg_controllen` set to 0, but
> `msg_control` is not NULL.
> 
> Signed-off-by: Stefano Garzarella 
> ---
>  subprojects/libvhost-user/libvhost-user.c | 1 +
>  1 file changed, 1 insertion(+)

Reviewed-by: Eric Blake 

> 
> diff --git a/subprojects/libvhost-user/libvhost-user.c 
> b/subprojects/libvhost-user/libvhost-user.c
> index a879149fef..22bea0c775 100644
> --- a/subprojects/libvhost-user/libvhost-user.c
> +++ b/subprojects/libvhost-user/libvhost-user.c
> @@ -632,6 +632,7 @@ vu_message_write(VuDev *dev, int conn_fd, VhostUserMsg 
> *vmsg)
>  memcpy(CMSG_DATA(cmsg), vmsg->fds, fdsize);
>  } else {
>  msg.msg_controllen = 0;
> +msg.msg_control = NULL;
>  }
>  
>  do {
> -- 
> 2.44.0
> 

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

Re: [PATCH v1] coroutine: avoid inserting duplicate coroutine to co_queue_wakeup

[Eric Blake]

I've seen (and agree with) Stefan's reply that a more thorough audit
is needed, but am still providing a preliminary review based on what I
see here.

On Mon, Mar 25, 2024 at 05:18:50PM +0800, zhuyangyang via wrote:
> If g_main_loop_run()/aio_poll() is called in the coroutine context,
> the pending coroutine may be woken up repeatedly, and the co_queue_wakeup
> may be disordered.

coroutine context should not be doing anything that can block; you may
have uncovered a bigger bug that we need to address.

> 
> When the poll() syscall exited in g_main_loop_run()/aio_poll(), it means
> some listened events is completed. Therefore, the completion callback
> function is dispatched.
> 
> If this callback function needs to invoke aio_co_enter(), it will only
> wake up the coroutine (because we are already in coroutine context),
> which may cause that the data on this listening event_fd/socket_fd
> is not read/cleared. When the next poll () exits, it will be woken up again
> and inserted into the wakeup queue again.
> 
> For example, if TLS is enabled in NBD, the server will call g_main_loop_run()
> in the coroutine, and repeatedly wake up the io_read event on a socket.
> The call stack is as follows:
> 
> aio_co_enter()
> aio_co_wake()
> qio_channel_restart_read()
> aio_dispatch_handler()
> aio_dispatch_handlers()
> aio_dispatch()
> aio_ctx_dispatch()
> g_main_context_dispatch()
> g_main_loop_run()
> nbd_negotiate_handle_starttls()
> nbd_negotiate_options()
> nbd_negotiate()
> nbd_co_client_start()
> coroutine_trampoline()
> 
> Signed-off-by: zhuyangyang 
> ---
>  util/async.c | 13 -
>  1 file changed, 12 insertions(+), 1 deletion(-)
> 
> diff --git a/util/async.c b/util/async.c
> index 0467890052..25fc1e6083 100644
> --- a/util/async.c
> +++ b/util/async.c
> @@ -705,7 +705,18 @@ void aio_co_enter(AioContext *ctx, Coroutine *co)
>  if (qemu_in_coroutine()) {
>  Coroutine *self = qemu_coroutine_self();
>  assert(self != co);
> -QSIMPLEQ_INSERT_TAIL(>co_queue_wakeup, co, co_queue_next);
> +/*
> + * If the Coroutine *co is already in the co_queue_wakeup, this
> + * repeated insertion will causes the loss of other queue element

s/causes/cause/

> + * or infinite loop.
> + * For examplex:

s/examplex/example/

> + * Head->a->b->c->NULL, after insert_tail(head, b) => 
> Head->a->b->NULL
> + * Head->a-b>->NULL, after insert_tail(head, b) => Head->a->b->b...

s/b>->/b->/

> + */
> +if (!co->co_queue_next.sqe_next &&
> +self->co_queue_wakeup.sqh_last != >co_queue_next.sqe_next) {
> +QSIMPLEQ_INSERT_TAIL(>co_queue_wakeup, co, co_queue_next);
> +    }
>  } else {
>  qemu_aio_coroutine_enter(ctx, co);
>  }

Intuitively, attacking the symptoms (avoiding bogus list insertion
when it is already on the list) makes sense; but I want to make sure
we attack the root cause.

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

Re: [PATCH for-9.0] mirror: Don't call job_pause_point() under graph lock

[Eric Blake]

On Wed, Mar 13, 2024 at 04:30:00PM +0100, Kevin Wolf wrote:
> Calling job_pause_point() while holding the graph reader lock
> potentially results in a deadlock: bdrv_graph_wrlock() first drains
> everything, including the mirror job, which pauses it. The job is only
> unpaused at the end of the drain section, which is when the graph writer
> lock has been successfully taken. However, if the job happens to be
> paused at a pause point where it still holds the reader lock, the writer
> lock can't be taken as long as the job is still paused.
> 
> Mark job_pause_point() as GRAPH_UNLOCKED and fix mirror accordingly.
> 
> Cc: qemu-sta...@nongnu.org
> Buglink: https://issues.redhat.com/browse/RHEL-28125
> Fixes: 004915a96a7a40e942ac85e6d22518cbcd283506
> Signed-off-by: Kevin Wolf 
> ---
>  include/qemu/job.h |  2 +-
>  block/mirror.c | 10 ++
>  2 files changed, 7 insertions(+), 5 deletions(-)

Reviewed-by: Eric Blake 

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

Re: [PATCH 08/16] block/qcow2: Fix missing ERRP_GUARD() for error_prepend()

[Eric Blake]

On Thu, Feb 29, 2024 at 12:37:15AM +0800, Zhao Liu wrote:
> From: Zhao Liu 
> 
> As the comment in qapi/error, passing @errp to error_prepend() requires
> ERRP_GUARD():
> 

> 
> In block/qcow2.c, there're 2 functions passing @errp to error_prepend()

s/there're/there are/

> without ERRP_GUARD():
>  - qcow2_co_create()
>  - qcow2_co_truncate()
> 
> Their @errp parameters are so widely sourced that it is necessary to
> protect their @errp with ERRP_GUARD().
> 
> Therefore, to avoid the issue like [1] said, add missing ERRP_GUARD() at
> their beginning.
> 
> [1]: Issue description in the commit message of commit ae7c80a7bd73
>  ("error: New macro ERRP_GUARD()").
> 
> Cc: Kevin Wolf 
> Cc: Hanna Reitz 
> Signed-off-by: Zhao Liu 
> ---
>  block/qcow2.c | 2 ++
>  1 file changed, 2 insertions(+)

Reviewed-by: Eric Blake 


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

Re: [PATCH 03/16] block: Fix missing ERRP_GUARD() for error_prepend()

[Eric Blake]

On Thu, Feb 29, 2024 at 12:37:10AM +0800, Zhao Liu wrote:
> From: Zhao Liu 
> 
> As the comment in qapi/error, passing @errp to error_prepend() requires
> ERRP_GUARD():
> 

> 
> In block.c, there're 4 functions passing @errp to error_prepend()
> without ERRP_GUARD():
>  - bdrv_co_create_opts_simple()
>  - parse_json_filename()
>  - bdrv_open_backing_file()
>  - bdrv_append_temp_snapshot()
> 
> bdrv_co_create_opts_simple(), as an implementation of
> BolckDriver.bdrv_co_create_opts(), its @errp parameter is so widely

BlockDriver

> sourced that it is necessary to protect it with ERRP_GUARD().
> 
> Though the @errp parameters passed to parse_json_filename(),
> bdrv_open_backing_file() and bdrv_append_temp_snapshot() points to their
> callers' local_err, to follow the requirement of @errp, also add missing
> ERRP_GUARD() at their beginning.
> 
> [1]: Issue description in the commit message of commit ae7c80a7bd73
>  ("error: New macro ERRP_GUARD()").
> 
> Cc: Kevin Wolf 
> Cc: Hanna Reitz 
> Signed-off-by: Zhao Liu 
> ---
>  block.c | 4 
>  1 file changed, 4 insertions(+)

Reviewed-by: Eric Blake 

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

Re: [PATCH 2/2] docs/devel/writing-monitor-commands: Minor improvements

[Eric Blake]

On Tue, Feb 27, 2024 at 12:56:17PM +0100, Markus Armbruster wrote:
> Avoid "JSON" when talking about the QAPI schema syntax.  Capitalize
> QEMU.  Don't claim all HMP commands live in monitor/hmp-cmds.c (this
> was never true).  Fix punctuation and drop inappropriate "the" here
> and there.
> 
> Signed-off-by: Markus Armbruster 
> ---
>  docs/devel/writing-monitor-commands.rst | 32 -
>  1 file changed, 15 insertions(+), 17 deletions(-)
> 

Reviewed-by: Eric Blake 

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

Re: [PATCH 1/2] docs/devel/writing-monitor-commands: Repair a decade of rot

[Eric Blake]

On Tue, Feb 27, 2024 at 12:56:16PM +0100, Markus Armbruster wrote:
> The tutorial doesn't match reality since at least 2013.  Repairing it
> involves fixing the following issues:
> 
> * Update for commit 6d327171551 (aio / timers: Remove alarm timers):
>   replace the broken examples.  Instead of having one for returning a
>   struct and another for returning a list of structs, do just one for
>   the latter.  This resolves the FIXME added in commit
>   e218052f928 (aio / timers: De-document -clock) back in 2014.
> 
> * Update for commit 895a2a80e0e (qapi: Use 'struct' instead of 'type'
>   in schema).
> 
> * Update for commit 3313b6124b5 (qapi: add qapi2texi script): add
>   required documentation to the schema snippets, and drop section
>   "Command Documentation".
> 
> * Update for commit a3c45b3e629 (qapi: New special feature flag
>   "unstable"): supply the required feature, deemphasize the x- prefix.
> 
> * Update for commit dd98234c059 (qapi: introduce x-query-roms QMP
>   command): rephrase from "add new command" to "examine existing
>   command".
> 
> * Update for commit 9492718b7c0 (qapi misc: Elide redundant has_FOO in
>   generated C): hello-world's message argument no longer comes with a
>   has_message, add a second argument that does.
> 
> * Update for moved and renamed files.
> 
> While there, update QMP version output to current output.

Nice cleanups.

> 
> Signed-off-by: Markus Armbruster 
> ---
>  docs/devel/writing-monitor-commands.rst | 460 ++--
>  1 file changed, 181 insertions(+), 279 deletions(-)

>  
> -The "type" keyword defines a new QAPI type. Its "data" member contains the
> -type's members. In this example our members are the "clock-name" and the
> -"next-deadline" one, which is optional.
> +The "struct" keyword defines a new QAPI type. Its "data" member
> +contains the type's members. In this example our members are
> +"filename" and "bootindex".  The latter is optional.

Is there any rhyme or reason behind one vs. two spaces after sentences here?

>  
>  The HMP command
>  ~~~
>  
> -Here's the HMP counterpart of the query-alarm-clock command::
> +Here's the HMP counterpart of the query-option-roms command::
>  
> - void hmp_info_alarm_clock(Monitor *mon)
> + void hmp_info_option_roms(Monitor *mon, const QDict *qdict)
>   {
> - QemuAlarmClock *clock;
>   Error *err = NULL;
> + OptionRomInfoList *info_list, *tail;
> + OptionRomInfo *info;
>  
> - clock = qmp_query_alarm_clock();
> + info_list = qmp_query_option_roms();
>   if (hmp_handle_error(mon, err)) {
> - return;
> +  return;

Was the change to TAB intentional?

>   }
>  
> - monitor_printf(mon, "Alarm clock method in use: '%s'\n", 
> clock->clock_name);
> - if (clock->has_next_deadline) {
> - monitor_printf(mon, "Next alarm will fire in %" PRId64 " 
> nanoseconds\n",
> -clock->next_deadline);
> + for (tail = info_list; tail; tail = tail->next) {
> +  info = tail->value;
> +  monitor_printf(mon, "%s", info->filename);
> +  if (info->has_bootindex) {
> +  monitor_printf(mon, " %" PRId64, info->bootindex);
> +  }
> +  monitor_printf(mon, "\n");
>   }

More TABs.

>  Writing a debugging aid returning unstructured text
>  ---
...
> -The ``HumanReadableText`` struct is intended to be used for all
> -commands, under the ``x-`` name prefix that are returning unstructured
> -text targeted at humans. It should never be used for commands outside
> -the ``x-`` name prefix, as those should be using structured QAPI types.
> +The ``HumanReadableText`` struct is defined in qapi/common.json as a
> +struct with a string member. It is intended to be used for all
> +commands that are returning unstructured text targeted at
> +humans. These should all have feature 'unstable'.  Note that the
> +feature's documentation states why the command is unstable.  WE

We

> +commonly use a ``x-`` command name prefix to make lack of stability
> +obvious to human users.
>  

Cleanups are trivial enough that I'm fine with you making them before
including:

Reviewed-by: Eric Blake 

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

Re: [PATCH 4/4] qapi/char: Deprecate backend type "memory"

[Eric Blake]

On Sat, Feb 03, 2024 at 09:02:28AM +0100, Markus Armbruster wrote:
> It's an alias for "ringbuf" we kept for backward compatibility; see
> commit 3a1da42eb35 (qapi: Rename ChardevBackend member "memory" to
> "ringbuf").  Deprecation is long overdue.
> 
> Signed-off-by: Markus Armbruster 
> ---
>  docs/about/deprecated.rst | 8 
>  qapi/char.json| 8 +---
>  2 files changed, 13 insertions(+), 3 deletions(-)

Reviewed-by: Eric Blake 

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

Re: [PATCH 3/4] qapi/char: Make backend types properly conditional

[Eric Blake]

On Sat, Feb 03, 2024 at 09:02:27AM +0100, Markus Armbruster wrote:
> Character backends are actually QOM types.  When a backend's
> compile-time conditional QOM type is not compiled in, creation fails
> with "'FOO' is not a valid char driver name".  Okay, except
> introspecting chardev-add with query-qmp-schema doesn't work then: the
> backend type is there even though the QOM type isn't.
> 
> A management application can work around this issue by using
> qom-list-types instead.
> 
> Fix the issue anyway: add the conditionals to the QAPI schema.
> 
> Signed-off-by: Markus Armbruster 
> ---
>  qapi/char.json | 20 
>  1 file changed, 12 insertions(+), 8 deletions(-)

Reviewed-by: Eric Blake 

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

Re: [PATCH 2/4] tests/unit/test-char: Fix qemu_socket(), make_udp_socket() check

[Eric Blake]

On Sat, Feb 03, 2024 at 09:02:26AM +0100, Markus Armbruster wrote:
> qemu_socket() and make_udp_socket() return a file descriptor on
> success, -1 on failure.  The check misinterprets 0 as failure.  Fix
> that.
> 
> Signed-off-by: Markus Armbruster 
> ---
>  tests/unit/test-char.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)

Reviewed-by: Eric Blake 

Might be worth amending the commit message of 1/4 where you called out
this bug to mention it will be fixed in the next patch.

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

Re: [PATCH 1/4] chardev/parallel: Don't close stdin on inappropriate device

[Eric Blake]

On Sat, Feb 03, 2024 at 09:02:25AM +0100, Markus Armbruster wrote:
> The __linux__ version of qemu_chr_open_pp_fd() tries to claim the
> parport device with a PPCLAIM ioctl().  On success, it stores the file
> descriptor in the chardev object, and returns success.  On failure, it
> closes the file descriptor, and returns failure.
> 
> chardev_new() then passes the Chardev to object_unref().  This duly
> calls char_parallel_finalize(), which closes the file descriptor
> stored in the chardev object.  Since qemu_chr_open_pp_fd() didn't
> store it, it's still zero, so this closes standard input.  Ooopsie.
> 
> To demonstate, add a unit test.  With the bug above unfixed, running
> this test closes standard input.  char_hotswap_test() happens to run
> next.  It opens a socket, duly gets file descriptor 0, and since it
> tests for success with > 0 instead of >= 0, it fails.

Two bugs for the price of one!

> 
> The test needs to be conditional exactly like the chardev it tests.
> Since the condition is rather complicated, steal the solution from the
> serial chardev: define HAVE_CHARDEV_PARALLEL in qemu/osdep.h.  This
> also permits simplifying chardev/meson.build a bit.
> 
> The bug fix is easy enough: store the file descriptor, and leave
> closing it to char_parallel_finalize().
> 
> Signed-off-by: Markus Armbruster 
> ---

> +++ b/include/qemu/osdep.h
> @@ -508,11 +508,18 @@ void qemu_anon_ram_free(void *ptr, size_t size);
>  
>  #ifdef _WIN32
>  #define HAVE_CHARDEV_SERIAL 1
> -#elif defined(__linux__) || defined(__sun__) || defined(__FreeBSD__)\
> +#define HAVE_CHARDEV_PARALLEL 1
> +#else
> +#if defined(__linux__) || defined(__sun__) || defined(__FreeBSD__)   \
>  || defined(__NetBSD__) || defined(__OpenBSD__) || defined(__DragonFly__) 
> \
>  || defined(__GLIBC__) || defined(__APPLE__)
>  #define HAVE_CHARDEV_SERIAL 1
>  #endif
> +#if defined(__linux__) || defined(__FreeBSD__) \
> +|| defined(__FreeBSD_kernel__) || defined(__DragonFly__)
> +#define HAVE_CHARDEV_PARALLEL 1
> +#endif
> +#endif

Not for this patch, but I've grown to like a preprocessor style I've
seen in other projects to make it easier to read nested #if:

#ifdef _WIN32
# define HAVE_CHARDEV_SERIAL 1
# define HAVE_CHARDEV_PARALLEL 1
#else
# if defined(__linux__) ... defined(__APPLE__)
#  define HAVE_CHARDEV_SERIAL 1
# endif
# if defined(__linux__) ... defined(__DragonFly__)
#  define HAVE_CHARDEV_PARALLEL 1
# endif
#endif

> +++ b/chardev/meson.build
> @@ -21,11 +21,9 @@ if host_os == 'windows'
>  else
>    chardev_ss.add(files(
>'char-fd.c',
> +'char-parallel.c',
>'char-pty.c',

Indentation looks off.  Otherwise,

Reviewed-by: Eric Blake 

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

Re: [PATCH [repost]] block/blkio: Don't assume size_t is 64 bit

[Eric Blake]

On Mon, Jan 29, 2024 at 06:53:55PM +, Richard W.M. Jones wrote:
> With GCC 14 the code failed to compile on i686 (and was wrong for any
> version of GCC):
> 
> ../block/blkio.c: In function ‘blkio_file_open’:
> ../block/blkio.c:857:28: error: passing argument 3 of ‘blkio_get_uint64’ from 
> incompatible pointer type [-Wincompatible-pointer-types]
>   857 |>mem_region_alignment);
>   |^~~~
>   ||
>   |size_t * {aka unsigned int *}
> In file included from ../block/blkio.c:12:
> /usr/include/blkio.h:49:67: note: expected ‘uint64_t *’ {aka ‘long long 
> unsigned int *’} but argument is of type ‘size_t *’ {aka ‘unsigned int *’}
>49 | int blkio_get_uint64(struct blkio *b, const char *name, uint64_t 
> *value);
>   | 
> ~~^

I wish gcc could point this out even when compiling on a 64-bit
platform where size_t and uint64_t happen to share the same type, by
reasoning about the underlying typedefs being different.  But that's a
bigger task for gcc, and not one for this group.

> 
> Signed-off-by: Richard W.M. Jones 
> ---
>  block/blkio.c | 12 +++-
>  1 file changed, 11 insertions(+), 1 deletion(-)

Reviewed-by: Eric Blake 

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

Re: [PATCH 2/5] qapi: Drop redundant documentation of conditional

[Eric Blake]

On Mon, Jan 29, 2024 at 12:50:05PM +0100, Markus Armbruster wrote:
> Documentation generated for dump-skeys contains
> 
> This command is only supported on s390 architecture.
> 
> and
> 
> If
> ~~
> 
> "TARGET_S390X"
> 
> The former became redundant in commit 901a34a400a (qapi: add 'If:'
> section to generated documentation) added the latter.  Drop the
> former.
> 
> Signed-off-by: Markus Armbruster 
> ---
>  qapi/misc-target.json | 2 --
>  1 file changed, 2 deletions(-)

Reviewed-by: Eric Blake 

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

Re: [PATCH 1/5] qapi: Drop redundant documentation of inherited members

[Eric Blake]

On Mon, Jan 29, 2024 at 12:50:04PM +0100, Markus Armbruster wrote:
> Documentation generated for SchemaInfo looks like
> 
> The members of "SchemaInfoBuiltin" when "meta-type" is ""builtin""
> The members of "SchemaInfoEnum" when "meta-type" is ""enum""
> The members of "SchemaInfoArray" when "meta-type" is ""array""
> The members of "SchemaInfoObject" when "meta-type" is ""object""
> The members of "SchemaInfoAlternate" when "meta-type" is ""alternate""
> The members of "SchemaInfoCommand" when "meta-type" is ""command""
> The members of "SchemaInfoEvent" when "meta-type" is ""event""
> Additional members depend on the value of "meta-type".
> 
> The last line became redundant when commit 88f63467c57 (qapi2texi:
> Generate reference to base type members) added the lines preceding it.
> Drop it.
> 
> BlockdevOptions has the same issue.  Drop
> 
> Remaining options are determined by the block driver.
> 
> Signed-off-by: Markus Armbruster 
> ---
>  qapi/block-core.json | 2 --
>  qapi/introspect.json | 2 --
>  2 files changed, 4 deletions(-)

Reviewed-by: Eric Blake 

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

Re: [PATCH 3/5] qapi: Elide "Potential additional modes" from generated docs

[Eric Blake]

On Mon, Jan 29, 2024 at 12:50:06PM +0100, Markus Armbruster wrote:
> Documentation of BlockExportRemoveMode has
> 
> Potential additional modes to be added in the future:
> 
> hide: Just hide export from new clients, leave existing connections
> as is.  Remove export after all clients are disconnected.
> 
> soft: Hide export from new clients, answer with ESHUTDOWN for all
> further requests from existing clients.
> 
> I think this is useful only for developers.  Elide it from generated
> documentation by turning it into a TODO section.
> 
> This effectively reverts my own commit b71fd73cc45 (Revert "qapi:
> BlockExportRemoveMode: move comments to TODO").  At the time, I was
> about to elide TODO sections from the generated manual, I wasn't sure
> about this one, and decided to avoid change.  And now I've made up my
> mind.
> 
> Signed-off-by: Markus Armbruster 
> ---
>  qapi/block-export.json | 11 ++-
>  1 file changed, 6 insertions(+), 5 deletions(-)

Reviewed-by: Eric Blake 

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

Re: [PATCH] iotests: don't run tests requiring cached writes in '-nocache' mode

[Eric Blake]

On Mon, Dec 11, 2023 at 03:32:23PM +0200, Andrey Drobyshev wrote:
> There're tests whose logic implies running without O_DIRECT set,
> otherwise they fail when running iotests in '-nocache' mode.  For these
> tests let's add _require_no_o_direct() helper which can be put in the
> preabmle and which makes sure '-nocache' isn't set.  Use it to skip

preamble

> running the following tests:
> 
>   * 271: creates files with unaligned sizes, thus producing multiple
> errors like:
> 
> qemu-io: can't open device /path/to/t.qcow2.raw: Cannot get 'write'
> permission without 'resize': Image size is not a multiple of request alignment

I wonder if we can instead tweak the test to use larger sizes such
that all accesses ARE aligned even with O_DIRECT.

/me goes and reads the test...

# Note that the image size is not a multiple of the cluster size
_reset_img 2083k

Ah - we really DO have a test that depends on odd sizing; where
changing it to be more nicely aligned will break other assumptions in
the test.

> 
>   * 308, file-io-error: use fuse exports.  Though fuse does have
> 'direct-io' mode (see https://docs.kernel.org/filesystems/fuse-io.html)
> we aren't using it yet, thus getting errors like:
> 
> qemu-io: can't open device /path/to/t.qcow2.fuse: Could not open
> '/path/to/t.qcow2.fuse': filesystem does not support O_DIRECT

And I agree that this one is beyond our control, so adding skip
support makes sense.

> 
> Signed-off-by: Andrey Drobyshev 
> ---
>  tests/qemu-iotests/271 | 1 +
>  tests/qemu-iotests/308 | 2 ++
>  tests/qemu-iotests/common.rc   | 7 +++
>  tests/qemu-iotests/tests/file-io-error | 1 +
>  4 files changed, 11 insertions(+)

Reviewed-by: Eric Blake 

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

Re: [PATCH] iotests/264: Use iotests.sock_dir for socket creation

[Eric Blake]

On Thu, Jan 25, 2024 at 03:52:37PM +0200, Andrey Drobyshev wrote:
> If socket path is too long (longer than 108 bytes), socket can't be
> opened.  This might lead to failure when test dir path is long enough.
> Make sure socket is created in iotests.sock_dir to avoid such a case.
> 
> This commit basically aligns iotests/264 with the rest of iotests.
> 
> Signed-off-by: Andrey Drobyshev 
> ---
>  tests/qemu-iotests/264 | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)

Reviewed-by: Eric Blake 

> 
> diff --git a/tests/qemu-iotests/264 b/tests/qemu-iotests/264
> index c532ccd809..c6ba2754e2 100755
> --- a/tests/qemu-iotests/264
> +++ b/tests/qemu-iotests/264
> @@ -25,7 +25,8 @@ import os
>  import iotests
>  from iotests import qemu_img_create, file_path, qemu_nbd_popen
>  
> -disk_a, disk_b, nbd_sock = file_path('disk_a', 'disk_b', 'nbd-sock')
> +disk_a, disk_b = file_path('disk_a', 'disk_b')
> +nbd_sock = file_path('nbd-sock', base_dir=iotests.sock_dir)
>  nbd_uri = 'nbd+unix:///?socket=' + nbd_sock
>  wait_limit = 3.0
>  wait_step = 0.2
> -- 
> 2.39.3
> 

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

Re: [PATCH 0/7] qapi qmp: Documentation fixes

[Eric Blake]

On Sat, Jan 20, 2024 at 10:53:20AM +0100, Markus Armbruster wrote:
> Markus Armbruster (7):
>   docs/devel/qapi-code-gen: Fix missing ':' in tagged section docs
>   docs: Replace dangling references to docs/interop/qmp-intro.txt
>   qapi: Fix dangling references to docs/devel/qapi-code-gen.txt
>   docs/interop/bitmaps: Clean up a reference to qemu-qmp-ref
>   qapi: Fix mangled "Returns" sections in documentation
>   qapi: Indent tagged doc comment sections properly
>   qapi: Fix malformed "Since:" section tags (again)

For the series:
Reviewed-by: Eric Blake 

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

Re: [PATCH] iotests/277: Use iotests.sock_dir for socket creation

[Eric Blake]

On Wed, Jan 24, 2024 at 06:22:57PM +0200, Andrey Drobyshev wrote:
> If socket path is too long (longer than 108 bytes), socket can't be
> opened.  This might lead to failure when test dir path is long enough.
> Make sure socket is created in iotests.sock_dir to avoid such a case.
> 
> This commit basically aligns iotests/277 with the rest of iotests.
> 
> Signed-off-by: Andrey Drobyshev 
> ---
>  tests/qemu-iotests/277 | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)

Reviewed-by: Eric Blake 

> 
> diff --git a/tests/qemu-iotests/277 b/tests/qemu-iotests/277
> index 24833e7eb6..4224202ac2 100755
> --- a/tests/qemu-iotests/277
> +++ b/tests/qemu-iotests/277
> @@ -27,7 +27,8 @@ from iotests import file_path, log
>  iotests.script_initialize()
>  
>  
> -nbd_sock, conf_file = file_path('nbd-sock', 'nbd-fault-injector.conf')
> +conf_file = file_path('nbd-fault-injector.conf')
> +nbd_sock = file_path('nbd-sock', base_dir=iotests.sock_dir)
>  
>  
>  def make_conf_file(event):
> -- 
> 2.39.3
> 

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

Re: Re: [PATCH v2 1/4] Include new arbitrary limits if not already defined

[Eric Blake]

On Mon, Jan 22, 2024 at 05:19:19PM +, Peter Maydell wrote:
> On Thu, 18 Jan 2024 at 16:03, Manolo de Medici  
> wrote:
> >
> > qemu uses the PATH_MAX and IOV_MAX constants extensively
> > in the code. Define these constants to sensible values ourselves
> > if the system doesn't define them already.
> >
> > Signed-off-by: Manolo de Medici 
> > ---
> >  include/qemu/osdep.h | 8 
> >  1 file changed, 8 insertions(+)
> >
> > diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h
> > index 9a405bed89..9fb6ac5c64 100644
> > --- a/include/qemu/osdep.h
> > +++ b/include/qemu/osdep.h
> > @@ -363,6 +363,14 @@ void QEMU_ERROR("code path is reachable")
> >  #define TIME_MAX TYPE_MAXIMUM(time_t)
> >  #endif
> >
> > +#ifndef PATH_MAX
> > +#define PATH_MAX 1024
> > +#endif

POSIX requires that _XOPEN_PATH_MAX be defined as 1024, as a bare
minimum for any system implementing X/Open extensions to POSIX, so
this number is reasonable.  It is also small enough that most of our
uses where PATH_MAX is used for stack allocation (rather than heap
allocation) don't explode.  But the /reason/ that GNU Hurd refuses to
define PATH_MAX is /because/ it is an arbitrary limit, and GNU Hurd
goes out of its way to not impose such a small limit on the user.  The
intent is that portable code should be written to malloc() any path
operation in order to deal with ANY size file name thrown at the code,
rather than stack-allocate and risk truncation when the limits chosen
were too small for the user's desires.

I'm not opposed to this patch with a stronger commit message, but the
commit message would do well to show that an attempt was made to audit
all existing uses of PATH_MAX and why we still need them rather than
malloc()ing.  A stronger patch would be one that eliminates the use of
PATH_MAX from the code base, on the grounds that truly portable code
can handle all pathnames that the underlying system already has memory
on hand to throw at the program; note that I am /not/ insisting on
such a stronger code guarantee, but merely that we document our design
decision of why we are unable/unwilling to go that far if the stronger
guarantee turns out to be impractical.

> > +
> > +#ifndef IOV_MAX
> > +#define IOV_MAX 1024
> > +#endif

Here, POSIX only requires _XOPEN_IOV_MAX to be 16 (if you're going to
do sharded scatter-gather I/O, portable code has to cater to systems
that do not tolerate you trying to cram more than 16 shards into one
syscall).  Older Solaris actually had a limit this low, but I can
easily test that Linux has a limit of 1024, and a Google search seems
to concur that the various BSDs have also settled on 1024.  GNU Hurd
obviously supports more than 1024, but capping at this number is
reasonable.  Unlike eradicating PATH_MAX from the code base, this is
one that makes more sense to me to have in place; although it would
still be worth documenting that all known systems that qemu targets
(including GNU Hurd, if your intent is to make that such a system)
support 1024 rather than the smaller minimum of 16 that POSIX
mandates.

> > +
> >  /* Mac OSX has a  bug that incorrectly defines SIZE_MAX with
> >   * the wrong type. Our replacement isn't usable in preprocessor
> >   * expressions, but it is sufficient for our needs. */
> 
> Ccing some people who know more about portability concerns
> than I do...
> 
> thanks
> -- PMM
> 

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

Re: [PATCH 2/2] meson: generate .stp files for tools too

[Eric Blake]

On Mon, Jan 08, 2024 at 05:13:56PM +, Daniel P. Berrangé wrote:
> The qemu-img, qemu-io, qemu-nbd, qemu-storage-daemon tools all have
> support for systemtap tracing built-in, so should be given corresponding
> .stp files to define their probes.
> 
> Signed-off-by: Daniel P. Berrangé 
> ---
>  meson.build | 61 +++--
>  1 file changed, 40 insertions(+), 21 deletions(-)

I'm less familiar with writing meson rules, but I can follow how you
refactored the stap.found() logic to be shared among two different
groups of traceable binaries, with the obvious difference being the
generation of command:[] in a way that uses the correct prefixes per
tracable item.

And since I hit the problem of not being able to see trace output via
qemu-trace-stap on qemu-io, this makes sense.

Reviewed-by: Eric Blake 

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

Re: [PATCH 5/6] nbd/server: only traverse NBDExport->clients from main loop thread

[Eric Blake]

On Wed, Dec 20, 2023 at 08:49:02PM -0500, Stefan Hajnoczi wrote:
> The NBD clients list is currently accessed from both the export
> AioContext and the main loop thread. When the AioContext lock is removed
> there will be nothing protecting the clients list.
> 
> Adding a lock around the clients list is tricky because NBDClient
> structs are refcounted and may be freed from the export AioContext or
> the main loop thread. nbd_export_request_shutdown() -> client_close() ->
> nbd_client_put() is also tricky because the list lock would be held
> while indirectly dropping references to NDBClients.
> 
> A simpler approach is to only allow nbd_client_put() and client_close()
> calls from the main loop thread. Then the NBD clients list is only
> accessed from the main loop thread and no fancy locking is needed.
> 
> nbd_trip() just needs to reschedule itself in the main loop AioContext
> before calling nbd_client_put() and client_close(). This costs more CPU
> cycles per NBD request but is needed for thread-safety when the
> AioContext lock is removed.

Late review (now that this is already in), but this is a bit
misleading.  The CPU penalty is only incurred for NBD_CMD_DISC or
after detection of a protocol error (that is, only when the connection
is being shut down), and not on every NBD request.

Thanks for working on this!

> 
> Note that nbd_client_get() can still be called from either thread, so
> make NBDClient->refcount atomic.
> 
> Signed-off-by: Stefan Hajnoczi 
> ---
>  nbd/server.c | 23 ++++++++---
>  1 file changed, 20 insertions(+), 3 deletions(-)
 

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

Re: [PATCH 12/12] block: remove outdated AioContext locking comments

[Eric Blake]

On Wed, Nov 29, 2023 at 02:55:53PM -0500, Stefan Hajnoczi wrote:
> The AioContext lock no longer exists.
> 
> There is one noteworthy change:
> 
>   - * More specifically, these functions use BDRV_POLL_WHILE(bs), which
>   - * requires the caller to be either in the main thread and hold
>   - * the BlockdriverState (bs) AioContext lock, or directly in the
>   - * home thread that runs the bs AioContext. Calling them from
>   - * another thread in another AioContext would cause deadlocks.
>   + * More specifically, these functions use BDRV_POLL_WHILE(bs), which 
> requires
>   + * the caller to be either in the main thread or directly in the home 
> thread
>   + * that runs the bs AioContext. Calling them from another thread in another
>   + * AioContext would cause deadlocks.
> 
> I am not sure whether deadlocks are still possible. Maybe they have just
> moved to the fine-grained locks that have replaced the AioContext. Since
> I am not sure if the deadlocks are gone, I have kept the substance
> unchanged and just removed mention of the AioContext.

I'd rather text that may be overly conservative than an omission that
could lead to a bug; so I'm okay with your action there.

> 
> Signed-off-by: Stefan Hajnoczi 
> ---

Reviewed-by: Eric Blake 

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

Re: [PATCH 11/12] job: remove outdated AioContext locking comments

[Eric Blake]

On Wed, Nov 29, 2023 at 02:55:52PM -0500, Stefan Hajnoczi wrote:
> The AioContext lock no longer exists.
> 
> Signed-off-by: Stefan Hajnoczi 
> ---
>  include/qemu/job.h | 20 
>  1 file changed, 20 deletions(-)
>

Reviewed-by: Eric Blake 

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

Re: [PATCH 10/12] scsi: remove outdated AioContext lock comment

[Eric Blake]

On Wed, Nov 29, 2023 at 02:55:51PM -0500, Stefan Hajnoczi wrote:
> The SCSI subsystem no longer uses the AioContext lock. Request
> processing runs exclusively in the BlockBackend's AioContext since
> "scsi: only access SCSIDevice->requests from one thread" and hence the
> lock is unnecessary.
> 
> Signed-off-by: Stefan Hajnoczi 
> ---
>  hw/scsi/scsi-disk.c | 1 -
>  1 file changed, 1 deletion(-)
>

Reviewed-by: Eric Blake 

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

Re: [PATCH 09/12] docs: remove AioContext lock from IOThread docs

[Eric Blake]

On Wed, Nov 29, 2023 at 02:55:50PM -0500, Stefan Hajnoczi wrote:
> Encourage the use of locking primitives and stop mentioning the
> AioContext lock since it is being removed.
> 
> Signed-off-by: Stefan Hajnoczi 
> ---
>  docs/devel/multiple-iothreads.txt | 45 +++
>  1 file changed, 15 insertions(+), 30 deletions(-)
> 
> diff --git a/docs/devel/multiple-iothreads.txt 
> b/docs/devel/multiple-iothreads.txt
> index a3e949f6b3..4865196bde 100644
> --- a/docs/devel/multiple-iothreads.txt

Reviewed-by: Eric Blake 

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

Re: [PATCH 08/12] aio: remove aio_context_acquire()/aio_context_release() API

[Eric Blake]

On Wed, Nov 29, 2023 at 02:55:49PM -0500, Stefan Hajnoczi wrote:
> Delete these functions because nothing calls these functions anymore.
> 
> I introduced these APIs in commit 98563fc3ec44 ("aio: add
> aio_context_acquire() and aio_context_release()") in 2014. It's with a
> sigh of relief that I delete these APIs almost 10 years later.
> 
> Thanks to Paolo Bonzini's vision for multi-queue QEMU, we got an
> understanding of where the code needed to go in order to remove the
> limitations that the original dataplane and the IOThread/AioContext
> approach that followed it.
> 
> Emanuele Giuseppe Esposito had the splendid determination to convert
> large parts of the codebase so that they no longer needed the AioContext
> lock. This was a painstaking process, both in the actual code changes
> required and the iterations of code review that Emanuele eeked out of

s/eeked/eked/

> Kevin and me over many months.
> 
> Kevin Wolf tackled multitudes of graph locking conversions to protect
> in-flight I/O from run-time changes to the block graph as well as the
> clang Thread Safety Analysis annotations that allow the compiler to
> check whether the graph lock is being used correctly.
> 
> And me, well, I'm just here to add some pizzazz to the QEMU multi-queue
> block layer :). Thank you to everyone who helped with this effort,
> including Eric Blake, code reviewer extraordinaire, and others who I've
> forgotten to mention.
> 
> Signed-off-by: Stefan Hajnoczi 
> ---
>  include/block/aio.h | 17 -----
>  util/async.c    | 10 --
>  2 files changed, 27 deletions(-)
>

Yay!

Reviewed-by: Eric Blake 

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

Re: [PATCH 07/12] aio-wait: draw equivalence between AIO_WAIT_WHILE() and AIO_WAIT_WHILE_UNLOCKED()

[Eric Blake]

On Wed, Nov 29, 2023 at 02:55:48PM -0500, Stefan Hajnoczi wrote:
> Now that the AioContext lock no longer exists, AIO_WAIT_WHILE() and
> AIO_WAIT_WHILE_UNLOCKED() are equivalent.
> 
> A future patch will get rid of AIO_WAIT_WHILE_UNLOCKED().
> 
> Signed-off-by: Stefan Hajnoczi 
> ---
>  include/block/aio-wait.h | 16 
>  1 file changed, 4 insertions(+), 12 deletions(-)
>

Reviewed-by: Eric Blake 

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

Re: [PATCH 06/12] scsi: remove AioContext locking

[Eric Blake]

On Wed, Nov 29, 2023 at 02:55:47PM -0500, Stefan Hajnoczi wrote:
> The AioContext lock no longer has any effect. Remove it.
> 
> Signed-off-by: Stefan Hajnoczi 
> ---
>  include/hw/virtio/virtio-scsi.h | 14 --
>  hw/scsi/scsi-bus.c  |  2 --
>  hw/scsi/scsi-disk.c | 28 
>  hw/scsi/virtio-scsi.c   | 18 --
>  4 files changed, 4 insertions(+), 58 deletions(-)
>

Reviewed-by: Eric Blake 

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

Re: [PATCH 05/12] block: remove AioContext locking

[Eric Blake]

On Wed, Nov 29, 2023 at 02:55:46PM -0500, Stefan Hajnoczi wrote:
> This is the big patch that removes
> aio_context_acquire()/aio_context_release() from the block layer and
> affected block layer users.
> 
> There isn't a clean way to split this patch and the reviewers are likely
> the same group of people, so I decided to do it in one patch.
> 
> Signed-off-by: Stefan Hajnoczi 
> ---

> +++ b/block.c
> @@ -7585,29 +7433,12 @@ void coroutine_fn bdrv_co_leave(BlockDriverState *bs, 
> AioContext *old_ctx)
>  
>  void coroutine_fn bdrv_co_lock(BlockDriverState *bs)
>  {
> -AioContext *ctx = bdrv_get_aio_context(bs);
> -
> -/* In the main thread, bs->aio_context won't change concurrently */
> -assert(qemu_get_current_aio_context() == qemu_get_aio_context());
> -
> -/*
> - * We're in coroutine context, so we already hold the lock of the main
> - * loop AioContext. Don't lock it twice to avoid deadlocks.
> - */
> -assert(qemu_in_coroutine());

Is this assertion worth keeping in the short term?...

> -if (ctx != qemu_get_aio_context()) {
> -aio_context_acquire(ctx);
> -}
> +/* TODO removed in next patch */
>  }

...I guess I'll see in the next patch.

>  
>  void coroutine_fn bdrv_co_unlock(BlockDriverState *bs)
>  {
> -AioContext *ctx = bdrv_get_aio_context(bs);
> -
> -assert(qemu_in_coroutine());
> -if (ctx != qemu_get_aio_context()) {
> -aio_context_release(ctx);
> -}
> +/* TODO removed in next patch */
>  }

Same comment.

> +++ b/blockdev.c
> @@ -1395,7 +1352,6 @@ static void external_snapshot_action(TransactionAction 
> *action,
>  /* File name of the new image (for 'blockdev-snapshot-sync') */
>  const char *new_image_file;
>  ExternalSnapshotState *state = g_new0(ExternalSnapshotState, 1);
> -AioContext *aio_context;
>  uint64_t perm, shared;
>  
>  /* TODO We'll eventually have to take a writer lock in this function */

I'm guessing removal of the locking gets us one step closer to
implementing this TODO at a later time?  Or is it now a stale comment?
Either way, it doesn't affect this patch.

> +++ b/migration/block.c
> @@ -270,7 +270,6 @@ static int mig_save_device_bulk(QEMUFile *f, 
> BlkMigDevState *bmds)
>  
>  if (bmds->shared_base) {
>  qemu_mutex_lock_iothread();
> -aio_context_acquire(blk_get_aio_context(bb));
...
> @@ -313,22 +311,10 @@ static int mig_save_device_bulk(QEMUFile *f, 
> BlkMigDevState *bmds)
>  block_mig_state.submitted++;
>  blk_mig_unlock();
>  
> -/* We do not know if bs is under the main thread (and thus does
> - * not acquire the AioContext when doing AIO) or rather under
> - * dataplane.  Thus acquire both the iothread mutex and the
> - * AioContext.
> - *
> - * This is ugly and will disappear when we make bdrv_* thread-safe,
> - * without the need to acquire the AioContext.
> - */
> -qemu_mutex_lock_iothread();
> -aio_context_acquire(blk_get_aio_context(bmds->blk));

Will conflict, but with trivial resolution, with your other thread
renaming things to qemu_bql_lock().


> +++ b/tests/unit/test-blockjob.c

> -static void test_complete_in_standby(void)
> -{

> @@ -531,13 +402,5 @@ int main(int argc, char **argv)
>  g_test_add_func("/blockjob/cancel/standby", test_cancel_standby);
>  g_test_add_func("/blockjob/cancel/pending", test_cancel_pending);
>  g_test_add_func("/blockjob/cancel/concluded", test_cancel_concluded);
> -
> -/*
> - * This test is flaky and sometimes fails in CI and otherwise:
> - * don't run unless user opts in via environment variable.
> - */
> -if (getenv("QEMU_TEST_FLAKY_TESTS")) {
> -g_test_add_func("/blockjob/complete_in_standby", 
> test_complete_in_standby);
> -}

Looks like you ripped out this entire test, because it is no longer
viable.  I might have mentioned it in the commit message, or squashed
the removal of this test into the earlier 02/12 patch.

Reviewed-by: Eric Blake 

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

Re: [PATCH v2 2/2] block: stream: Allow users to request only format driver names in backing file format

[Eric Blake]

On Thu, Nov 30, 2023 at 05:06:04PM +0100, Peter Krempa wrote:
> Introduce a new flag 'backing_file_format_no_protocol' for the
> block-commit QMP command which instructs the internals to use 'raw'
> instead of the protocol driver in case when a image is used without a
> dummy 'raw' wrapper.

Same long name as in 1/2; if we come up with a nicer name (my proposal
in 1/2 was backing-mask-protocol; but I'm open to others), we should
keep the two patches consistent.

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

Re: [PATCH v2 1/2] block: commit: Allow users to request only format driver names in backing file format

[Eric Blake]

On Thu, Nov 30, 2023 at 05:06:03PM +0100, Peter Krempa wrote:
> Introduce a new flag 'backing_file_format_no_protocol' for the
> block-commit QMP command which instructs the internals to use 'raw'
> instead of the protocol driver in case when a image is used without a
> dummy 'raw' wrapper.
> 
> The flag is designed such that it can be always asserted by management
> tools even when there isn't any update to backing files.
> 
> The flag will be used by libvirt so that the backing images still
> reference the proper format even when libvirt will stop using the dummy
> raw driver (raw driver with no other config). Libvirt needs this so that
> the images stay compatible with older libvirt versions which didn't
> expect that a protocol driver name can appear in the backing file format
> field.
> 
> Signed-off-by: Peter Krempa 
> Reviewed-by: Vladimir Sementsov-Ogievskiy 
> ---

> +++ b/qapi/block-core.json
> @@ -1810,6 +1810,14 @@
>  # Care should be taken when specifying the string, to specify a
>  # valid filename or protocol.  (Since 2.1)
>  #
> +# @backing-file-format-no-protocol: If true always use a 'format' driver name
> +# for the 'backing file format' field if updating the image header of the
> +# overlay of 'top'. Otherwise the real name of the driver of the backing
> +# image may be used which may be a protocol driver.
> +#
> +# Can be used also when no image header will be updated.
> +# (default: false; since: 9.0)

This is a long name.  What about:

@backing-mask-protocol: If true, replace any protocol mentioned in the
'backing file format' with 'raw', rather than storing the protocol
name as the backing format.  Can be used even when no image header
    will be updated (default false; since 9.0).

or s/mask/hide/ if that sounds better.

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

Re: [PATCH 04/12] graph-lock: remove AioContext locking

[Eric Blake]

On Wed, Nov 29, 2023 at 02:55:45PM -0500, Stefan Hajnoczi wrote:
> Stop acquiring/releasing the AioContext lock in
> bdrv_graph_wrlock()/bdrv_graph_unlock() since the lock no longer has any
> effect.
> 
> The distinction between bdrv_graph_wrunlock() and
> bdrv_graph_wrunlock_ctx() becomes meaningless and they can be collapsed
> into one function.
> 
> Signed-off-by: Stefan Hajnoczi 
> ---

Reviewed-by: Eric Blake 

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

Re: [PATCH 03/12] aio: make aio_context_acquire()/aio_context_release() a no-op

[Eric Blake]

On Wed, Nov 29, 2023 at 02:55:44PM -0500, Stefan Hajnoczi wrote:
> aio_context_acquire()/aio_context_release() has been replaced by
> fine-grained locking to protect state shared by multiple threads. The
> AioContext lock still plays the role of balancing locking in
> AIO_WAIT_WHILE() and many functions in QEMU either require that the
> AioContext lock is held or not held for this reason. In other words, the
> AioContext lock is purely there for consistency with itself and serves
> no real purpose anymore.
> 
> Stop actually acquiring/releasing the lock in
> aio_context_acquire()/aio_context_release() so that subsequent patches
> can remove callers across the codebase incrementally.
> 
> I have performed "make check" and qemu-iotests stress tests across
> x86-64, ppc64le, and aarch64 to confirm that there are no failures as a
> result of eliminating the lock.
> 
> Signed-off-by: Stefan Hajnoczi 
> ---
>  util/async.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/util/async.c b/util/async.c
> index 8f90ddc304..04ee83d220 100644
> --- a/util/async.c
> +++ b/util/async.c
> @@ -725,12 +725,12 @@ void aio_context_unref(AioContext *ctx)
>  
>  void aio_context_acquire(AioContext *ctx)
>  {
> -qemu_rec_mutex_lock(>lock);
> +/* TODO remove this function */
>  }
>  
>  void aio_context_release(AioContext *ctx)
>  {
> -qemu_rec_mutex_unlock(>lock);
> +/* TODO remove this function */
>  }
>  
>  QEMU_DEFINE_STATIC_CO_TLS(AioContext *, my_aiocontext)
> -- 
> 2.42.0

Reviewed-by: Eric Blake 

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

Re: [PATCH 02/12] tests: remove aio_context_acquire() tests

[Eric Blake]

On Wed, Nov 29, 2023 at 02:55:43PM -0500, Stefan Hajnoczi wrote:
> The aio_context_acquire() API is being removed. Drop the test case that
> calls the API.
> 
> Signed-off-by: Stefan Hajnoczi 
> ---
>  tests/unit/test-aio.c | 67 +--
>  1 file changed, 1 insertion(+), 66 deletions(-)

The rest of this series should not hold up 8.2.

Reviewed-by: Eric Blake 

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

Re: [PATCH 01/12] virtio-scsi: replace AioContext lock with tmf_bh_lock

[Eric Blake]

On Wed, Nov 29, 2023 at 02:55:42PM -0500, Stefan Hajnoczi wrote:
> Protect the Task Management Function BH state with a lock. The TMF BH
> runs in the main loop thread. An IOThread might process a TMF at the
> same time as the TMF BH is running. Therefore tmf_bh_list and tmf_bh
> must be protected by a lock.
> 
> Run TMF request completion in the IOThread using aio_wait_bh_oneshot().
> This avoids more locking to protect the virtqueue and SCSI layer state.

Are we trying to get this into 8.2?

> 
> Signed-off-by: Stefan Hajnoczi 
> ---
>  include/hw/virtio/virtio-scsi.h |  3 +-
>  hw/scsi/virtio-scsi.c   | 62 ++---
>  2 files changed, 43 insertions(+), 22 deletions(-)
>

Reviewed-by: Eric Blake 

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

Re: [PATCH 4/4] dma-helpers: don't lock AioContext in dma_blk_cb()

[Eric Blake]

On Thu, Nov 23, 2023 at 02:49:31PM -0500, Stefan Hajnoczi wrote:
> Commit abfcd2760b3e ("dma-helpers: prevent dma_blk_cb() vs
> dma_aio_cancel() race") acquired the AioContext lock inside dma_blk_cb()
> to avoid a race with scsi_device_purge_requests() running in the main
> loop thread.
> 
> The SCSI code no longer calls dma_aio_cancel() from the main loop thread
> while I/O is running in the IOThread AioContext. Therefore it is no
> longer necessary to take this lock to protect DMAAIOCB fields. The
> ->cb() function also does not require the lock because blk_aio_*() and
> friends do not need the AioContext lock.
> 
> Both hw/ide/core.c and hw/ide/macio.c also call dma_blk_io() but don't
> rely on it taking the AioContext lock, so this change is safe.
> 
> Signed-off-by: Stefan Hajnoczi 
> ---
>  system/dma-helpers.c | 7 ++-
>  1 file changed, 2 insertions(+), 5 deletions(-)

Reviewed-by: Eric Blake 

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

Re: [PATCH 3/4] scsi: don't lock AioContext in I/O code path

[Eric Blake]

On Thu, Nov 23, 2023 at 02:49:30PM -0500, Stefan Hajnoczi wrote:
> blk_aio_*() doesn't require the AioContext lock and the SCSI subsystem's
> internal state also does not anymore.
> 
> Signed-off-by: Stefan Hajnoczi 
> ---
>  hw/scsi/scsi-disk.c| 23 ---
>  hw/scsi/scsi-generic.c | 20 +++-
>  2 files changed, 3 insertions(+), 40 deletions(-)
>

Reviewed-by: Eric Blake 

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

Re: [PATCH 2/4] virtio-scsi: don't lock AioContext around virtio_queue_aio_attach_host_notifier()

[Eric Blake]

On Thu, Nov 23, 2023 at 02:49:29PM -0500, Stefan Hajnoczi wrote:
> virtio_queue_aio_attach_host_notifier() does not require the AioContext
> lock. Stop taking the lock and remember add an explicit smp_wmb()

s/remember// ?

> because we were relying on the implicit barrier in the AioContext lock
> before.
> 
> Signed-off-by: Stefan Hajnoczi 
> ---
>  hw/scsi/virtio-scsi-dataplane.c | 8 +---
>  1 file changed, 1 insertion(+), 7 deletions(-)

Reviewed-by: Eric Blake 

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

Re: [PATCH] vmdk: Don't corrupt desc file in vmdk_write_cid

[Eric Blake]

On Fri, Nov 24, 2023 at 11:56:54AM +, Fam wrote:
> From: Fam Zheng 
> 
> If the text description file is larger than DESC_SIZE, we force the last
> byte in the buffer to be 0 and write it out.
> 
> This results in a corruption.
> 
> Try to allocate a big buffer in this case.
> 
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1923
> 
> Signed-off-by: Fam Zheng 
> ---
>  block/vmdk.c   | 28 
>  tests/qemu-iotests/059 |  2 ++
>  tests/qemu-iotests/059.out |  4 
>  3 files changed, 26 insertions(+), 8 deletions(-)

Reviewed-by: Eric Blake 

Are we trying to get this into 8.2, since it is a data corruption?

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

Re: [PATCH 1/4] scsi: only access SCSIDevice->requests from one thread

[Eric Blake]

On Thu, Nov 23, 2023 at 02:49:28PM -0500, Stefan Hajnoczi wrote:
> Stop depending on the AioContext lock and instead access
> SCSIDevice->requests from only one thread at a time:
> - When the VM is running only the BlockBackend's AioContext may access
>   the requests list.
> - When the VM is stopped only the main loop may access the requests
>   list.
> 
> These constraints protect the requests list without the need for locking
> in the I/O code path.
> 
> Note that multiple IOThreads are not supported yet because the code
> assumes all SCSIRequests are executed from a single AioContext. Leave
> that as future work.
> 
> Signed-off-by: Stefan Hajnoczi 
> ---
>  include/hw/scsi/scsi.h |   7 +-
>  hw/scsi/scsi-bus.c | 174 -
>  2 files changed, 124 insertions(+), 57 deletions(-)
>

Reviewed-by: Eric Blake 

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

Re: [PATCH v3] hw/i386: fix short-circuit logic with non-optimizing builds

[Eric Blake]

On Mon, Nov 20, 2023 at 11:20:52AM +0100, Philippe Mathieu-Daudé wrote:
> (Cc'ing Eric)
> 
> On 20/11/23 10:28, Michael S. Tsirkin wrote:
> > On Sun, Nov 19, 2023 at 07:34:58PM -0600, Dan Hoffman wrote:
> > > As far as I can tell, yes. Any optimization level above O0 does not have 
> > > this
> > > issue (on this version of Clang, at least)
> > 
> > Aha, this is with -O0. That makes sense.
> 
> But then, why the other cases aren't problematic?
> 
> $ git grep -E ' (&&|\|\|) !?kvm_enabled'
> hw/arm/boot.c:1228:assert(!(info->secure_board_setup && kvm_enabled()));

This one's obvious; no kvm_*() calls in the assert.

> hw/i386/microvm.c:270:(mms->rtc == ON_OFF_AUTO_AUTO &&
> !kvm_enabled())) {

Ones like this require reading context to see whether the if() block
guarded any kvm_*() calls for the linker to elide - but still a fairly
easy audit.

> > > 
> > >  I'm surprised the order of conditions matters for code elision...
> > > 
> > >  > Signed-off-by: Daniel Hoffman 
> > >  > ---
> > >  >   hw/i386/x86.c | 15 ---
> > >  >   1 file changed, 12 insertions(+), 3 deletions(-)
> > >  >
> > >  > diff --git a/hw/i386/x86.c b/hw/i386/x86.c
> > >  > index b3d054889bb..2b6291ad8d5 100644
> > >  > --- a/hw/i386/x86.c
> > >  > +++ b/hw/i386/x86.c
> > >  > @@ -131,8 +131,12 @@ void x86_cpus_init(X86MachineState *x86ms, int
> > >  default_cpu_version)
> > >  >       /*
> > >  >        * Can we support APIC ID 255 or higher?  With KVM, that 
> > > requires
> > >  >        * both in-kernel lapic and X2APIC userspace API.
> > >  > +     *
> > >  > +     * kvm_enabled() must go first to ensure that kvm_* 
> > > references are
> > >  > +     * not emitted for the linker to consume (kvm_enabled() is
> > >  > +     * a literal `0` in configurations where kvm_* aren't defined)
> > >  >        */
> > >  > -    if (x86ms->apic_id_limit > 255 && kvm_enabled() &&
> > >  > +    if (kvm_enabled() && x86ms->apic_id_limit > 255 &&
> > >  >           (!kvm_irqchip_in_kernel() || !kvm_enable_x2apic())) {

Indeed, if clang -O0 treats 'if (cond1 && 0 && cond2)' differently
than 'if (0 && cond1 && cond2)' for purposes of eliding the code for
cond2, that seems like a blatant missed optimization that we should be
reporting to the clang folks.

While this patch may solve the immediate issue, it does not scale - if
we ever have two separate guards that can both be independently
hard-coded to 0 based on configure-time decisions, but which are both
used as guards in the same expression, it will become impossible to
avoid a '(cond1 && 0 && cond2)' scenario across all 4 possible
configurations of those two guards.

I have no objection to the patch, but it would be nice if the commit
message could point to a clang bug report, if one has been filed.

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

Re: Converting images to stdout

[Eric Blake]

On Thu, Nov 16, 2023 at 06:48:09PM +0100, Alberto Garcia wrote:
> Hi,
> 
> I haven't written here in a while :) but I have something small that I
> would like to discuss.
> 
> Using qemu-img to convert an image and writing the result directly to
> stdout is a question that has already been raised in the past (see
> [1] for an example) and it's clear that it's generally not possible
> because the images need to be seekable.
> 
> While I think that there's almost certainly no generic way to do
> something like that for every combination of input and output formats,
> I do think that it should be relatively easy to produce a qcow2 file
> directly to stdout as long as the input file is on disk.

Indeed, it does seem like this should be easy enough to cobble together.

> 
> I'm interested in this use case, and I think that the method would be
> as simple as this:
> 
> 1. Decide a cluster size for the output qcow2 file.
> 2. Read the input file once to determine which clusters need to be
>allocated in the output file and which ones don't.
> 3. That infomation is enough to determine the number and contents of
>the refcount table, refcount blocks, and L1/L2 tables.
> 4. Write the qcow2 header + metadata + allocated data to stdout.

It may also be possible to write a qcow2 file that uses the external
data bit, so that you are only writing the qcow2 header + metadata,
and reusing the existing input file as the external data.

> 
> Since this would be qcow2-specific I would probably implement this as
> a separate tool instead of adding it directly to qemu-img. This could
> go to the scripts/ directory because I can imagine that such a tool
> could be useful for other people.

Also sounds reasonable.

> 
> Because this would be an external tool it would only support a qcow2
> file with the default options. Other features like compression would
> be out of scope.

Why is compression not viable?  Are you worried that the qcow2
metadata (such as refcounts) becomes too complex?

I've also wondered how easy or hard it would be to write a tool that
can take an existing qcow2 file and defragment and/or compress it
in-place (rather than having to copy it to a second qcow2 file).

> 
> For the same reason the input would be a raw file for simplicity,
> other input files could be presented in raw format using
> qemu-storage-daemon.

Yes, getting seekable input from any other format is easy enough; it's
the streaming output that is the trickier task.

> 
> And that's the general idea, comments and questions are welcome.
> 
> Thanks!
> 
> Berto
> 
> [1] https://lists.nongnu.org/archive/html/qemu-discuss/2020-01/msg00014.html
> 

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

Re: [PATCH 16/16] docs: add uefi variable service documentation and TODO list.

[Eric Blake]

On Wed, Nov 15, 2023 at 04:12:38PM +0100, Gerd Hoffmann wrote:
> Signed-off-by: Gerd Hoffmann 
> ---
>  docs/devel/index-internals.rst |  1 +
>  docs/devel/uefi-vars.rst   | 66 ++
>  hw/uefi/TODO.md| 17 +
>  3 files changed, 84 insertions(+)
>  create mode 100644 docs/devel/uefi-vars.rst
>  create mode 100644 hw/uefi/TODO.md

> +
> +Guest UEFI variable management
> +==
> +
> +Traditional approach for UEFI Variable storage in qemu guests is to

The traditional

> +work as close as possible to physical hardware.  That means provide

providing

> +pflash as storage and leave the management of variables and flash to

leaving

> +the guest.

> +
> +Secure boot support comes with the requirement that the UEFI variable
> +storage must be protected against direct access by the OS.  All update
> +requests must pass the sanity checks.  (Parts of) the firmware must
> +run with a higher priviledge level than the OS so this can be enforced

privilege

> +by the firmware.  On x86 this has been implemented using System
> +Management Mode (SMM) in qemu and kvm, which again is the same
> +approach taken by physical hardware.  Only priviedged code running in

privileged

> +SMM mode is allowed to access flash storage.
> +
> +Communication with the firmware code running in SMM mode works by
> +serializing the requests to a shared buffer, then trapping into SMM
> +mode via SMI.  The SMM code processes the request, stores the reply in
> +the same buffer and returns.
> +
> +Host UEFI variable service
> +==
> +
> +Instead of running the priviledged code inside the guest we can run it

privileged

> +on the host.  The serialization protocol cen be reused.  The

can

> +communication with the host uses a virtual device, which essentially
> +allows to configure the shared buffer location and size and to trap to

s/allows to configure/configures/
s/and to trap/, and traps/

> +the host to process the requests.
> +
> +The ``uefi-vars`` device implements the UEFI virtual device.  It comes
> +in ``uefi-vars-isa`` and ``uefi-vars-sysbus`` flavours.  The device
> +reimplements the handlers needed, specifically
> +``EfiSmmVariableProtocol`` and ``VarCheckPolicyLibMmiHandler``.  It
> +also consumes events (``EfiEndOfDxeEventGroup``,
> +``EfiEventReadyToBoot`` and ``EfiEventExitBootServices``).
> +
> +The advantage of the approach is that we do not need a special
> +prividge level for the firmware to protect itself, i.e. it does not

privilege

> +depend on SMM emulation on x64, which allows to remove a bunch of

s/allows to remove/allows the removal of/

> +complex code for SMM emulation from the linux kernel
> +(CONFIG_KVM_SMM=n).  It also allows to support secure boot on arm

s/to support/support for/

> +without implementing secure world (el3) emulation in kvm.
> +
> +Of course there are also downsides.  The added device increases the
> +attack surface of the host, and we are adding some code duplication
> +because we have to reimplement some edk2 functionality in qemu.
> +

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

Re: [PATCH] qapi/pragma.json: Improve the comment about the lists of QAPI rule exceptions

[Eric Blake]

On Mon, Nov 13, 2023 at 05:29:28PM +0100, Thomas Huth wrote:
> Let's use more inclusive language here.
> 
> Signed-off-by: Thomas Huth 
> ---
>  qapi/pragma.json | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)

Reviewed-by: Eric Blake 

> 
> diff --git a/qapi/pragma.json b/qapi/pragma.json
> index 7f810b0e97..0aa4eeddd3 100644
> --- a/qapi/pragma.json
> +++ b/qapi/pragma.json
> @@ -3,8 +3,8 @@
>  
>  { 'pragma': { 'doc-required': true } }
>  
> -# Whitelists to permit QAPI rule violations; think twice before you
> -# add to them!
> +# Entries in these lists are allowed to violate the QAPI rules (for
> +# historical reasons); think twice before you add to them!
>  { 'pragma': {
>  # Command names containing '_'
>  'command-name-exceptions': [
> -- 
> 2.41.0
> 

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

Re: [PATCH] block-backend: per-device throttling of BLOCK_IO_ERROR reports

[Eric Blake]

On Tue, Nov 07, 2023 at 11:58:42AM +0300, Vladimir Sementsov-Ogievskiy wrote:
> From: Leonid Kaplan 
> 
> BLOCK_IO_ERROR events comes from guest, so we must throttle them.
> We still want per-device throttling, so let's use device id as a key.
> 
> Signed-off-by: Leonid Kaplan 
> Signed-off-by: Vladimir Sementsov-Ogievskiy 
> ---
>  monitor/monitor.c | 10 ++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/monitor/monitor.c b/monitor/monitor.c
> index 01ede1babd..ad0243e9d7 100644
> --- a/monitor/monitor.c
> +++ b/monitor/monitor.c
> @@ -309,6 +309,7 @@ int error_printf_unless_qmp(const char *fmt, ...)
>  static MonitorQAPIEventConf monitor_qapi_event_conf[QAPI_EVENT__MAX] = {
>  /* Limit guest-triggerable events to 1 per second */
>  [QAPI_EVENT_RTC_CHANGE]= { 1000 * SCALE_MS },
> +[QAPI_EVENT_BLOCK_IO_ERROR]= { 1000 * SCALE_MS },
>  [QAPI_EVENT_WATCHDOG]  = { 1000 * SCALE_MS },
>  [QAPI_EVENT_BALLOON_CHANGE]= { 1000 * SCALE_MS },
>  [QAPI_EVENT_QUORUM_REPORT_BAD] = { 1000 * SCALE_MS },
> @@ -498,6 +499,10 @@ static unsigned int qapi_event_throttle_hash(const void 
> *key)
>  hash += g_str_hash(qdict_get_str(evstate->data, "qom-path"));
>  }
>  
> +if (evstate->event == QAPI_EVENT_BLOCK_IO_ERROR) {
> +hash += g_str_hash(qdict_get_str(evstate->data, "device"));

Wouldn't ^= be better than += for combining hashes?

> +}
> +
>  return hash;
>  }
>  
> @@ -525,6 +530,11 @@ static gboolean qapi_event_throttle_equal(const void *a, 
> const void *b)
> qdict_get_str(evb->data, "qom-path"));
>  }
>  
> +if (eva->event == QAPI_EVENT_BLOCK_IO_ERROR) {
> +return !strcmp(qdict_get_str(eva->data, "device"),
> +   qdict_get_str(evb->data, "device"));
> +}
> +

At any rate, the idea makes sense for me.

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