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

[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

[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 <stefanha@redhat.com>
CC: Daniel P. Berrangé <berrange@redhat.com>
CC: qemu-stable@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 <eblake@redhat.com>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@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..67b97000060 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-stable@nongnu.org
Signed-off-by: Eric Blake <eblake@redhat.com>
---
 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 00000000000..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=eblake@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=$(( 50000 + (RANDOM%15000) ))
+    while ss -ltn | grep -sqE ":$port\b"; do
+        ((port++))
+        if [ $port -eq 65000 ]; then port=50000; 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,
+              "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":"'"$DST_IMG"'", "node-name":"drive_sys1"}' \
+    -blockdev '{"driver":"qcow2", "node-name":"drive_image1",
+                "file":"drive_sys1"}' \
+    -incoming defer
+h2=$QEMU_HANDLE
+_send_qemu_cmd $h2 '{"execute": "qmp_capabilities"}' 'return'
+
+echo
+echo === Dst VM: Enable NBD server for incoming storage migration ===
+echo
+
+_send_qemu_cmd $h2 '{"execute": "nbd-server-start", "arguments":
+    {"addr": {"type": "inet", "data": {"host": "127.0.0.1", "port": "'$port'"}},
+              "tls-creds": "tls0"}}' '{"return": {}}' | sed "s/\"$port\"/PORT/g"
+_send_qemu_cmd $h2 '{"execute": "block-export-add", "arguments":
+    {"node-name": "drive_image1", "type": "nbd", "writable": true,
+      "id": "drive_image1"}}' '{"return": {}}'
+
+echo
+echo === Src VM: Mirror to dst NBD for outgoing storage migration ===
+echo
+
+_send_qemu_cmd $h1 '{"execute": "blockdev-add", "arguments":
+    {"node-name": "mirror", "driver": "nbd",
+     "server": {"type": "inet", "host": "127.0.0.1", "port": "'$port'"},
+     "export": "drive_image1", "tls-creds": "tls0",
+     "tls-hostname": "127.0.0.1"}}' '{"return": {}}' | sed "s/\"$port\"/PORT/g"
+_send_qemu_cmd $h1 '{"execute": "blockdev-mirror", "arguments":
+    {"sync": "full", "device": "drive_image1", "target": "mirror",
+     "job-id": "drive_image1_53"}}' '{"return": {}}'
+_timed_wait_for $h1 '"ready"'
+
+echo
+echo === Cleaning up ===
+echo
+
+_send_qemu_cmd $h1 '{"execute":"quit"}' ''
+_send_qemu_cmd $h2 '{"execute":"quit"}' ''
+
+echo "*** done"
+rm -f $seq.full
+status=0
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 00000000000..b5e12dcbe7a
--- /dev/null
+++ b/tests/qemu-iotests/tests/nbd-tls-iothread.out
@@ -0,0 +1,54 @@
+QA output created by nbd-tls-iothread
+
+== preparing TLS creds and spare port ==
+picked unused port
+Generating a self signed certificate...
+Generating a signed certificate...
+Generating a signed certificate...
+
+== preparing image ==
+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
+
+=== Starting Src QEMU ===
+
+{"execute": "qmp_capabilities"}
+{"return": {}}
+
+=== Starting Dst VM2 ===
+
+{"execute": "qmp_capabilities"}
+{"return": {}}
+
+=== Dst VM: Enable NBD server for incoming storage migration ===
+
+{"execute": "nbd-server-start", "arguments":
+    {"addr": {"type": "inet", "data": {"host": "127.0.0.1", "port": PORT}},
+              "tls-creds": "tls0"}}
+{"return": {}}
+{"execute": "block-export-add", "arguments":
+    {"node-name": "drive_image1", "type": "nbd", "writable": true,
+      "id": "drive_image1"}}
+{"return": {}}
+
+=== Src VM: Mirror to dst NBD for outgoing storage migration ===
+
+{"execute": "blockdev-add", "arguments":
+    {"node-name": "mirror", "driver": "nbd",
+     "server": {"type": "inet", "host": "127.0.0.1", "port": PORT},
+     "export": "drive_image1", "tls-creds": "tls0",
+     "tls-hostname": "127.0.0.1"}}
+{"return": {}}
+{"execute": "blockdev-mirror", "arguments":
+    {"sync": "full", "device": "drive_image1", "target": "mirror",
+     "job-id": "drive_image1_53"}}
+{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "created", "id": "drive_image1_53"}}
+{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "running", "id": "drive_image1_53"}}
+{"return": {}}
+{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "ready", "id": "drive_image1_53"}}
+
+=== Cleaning up ===
+
+{"execute":"quit"}
+{"execute":"quit"}
+*** done
-- 
2.45.0

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-stable@nongnu.org
> Signed-off-by: Eric Blake <eblake@redhat.com>
> ---
>  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 00000000000..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=eblake@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 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=$(( 50000 + (RANDOM%15000) ))
> +    while ss -ltn | grep -sqE ":$port\b"; do
> +        ((port++))
> +        if [ $port -eq 65000 ]; then port=50000; fi

Also, common.nbd only probes:
    for ((port = 10809; port <= 10909; port++))
and nbdkit's choice of starting with a random offset is interesting.

> +    done
> +    echo picked unused port

I'm not sure if there is any easy way to make an iotest output the
unfiltered text for debugging...

> +}
> +
> +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,
> +              "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":"'"$DST_IMG"'", "node-name":"drive_sys1"}' \
> +    -blockdev '{"driver":"qcow2", "node-name":"drive_image1",
> +                "file":"drive_sys1"}' \
> +    -incoming defer
> +h2=$QEMU_HANDLE
> +_send_qemu_cmd $h2 '{"execute": "qmp_capabilities"}' 'return'
> +
> +echo
> +echo === Dst VM: Enable NBD server for incoming storage migration ===
> +echo
> +
> +_send_qemu_cmd $h2 '{"execute": "nbd-server-start", "arguments":
> +    {"addr": {"type": "inet", "data": {"host": "127.0.0.1", "port": "'$port'"}},
> +              "tls-creds": "tls0"}}' '{"return": {}}' | sed "s/\"$port\"/PORT/g"

...while still having only filtered text in the .out file to avoid
spurious mismatch due to the intentional non-determinism in the port
picked.

> +_send_qemu_cmd $h2 '{"execute": "block-export-add", "arguments":
> +    {"node-name": "drive_image1", "type": "nbd", "writable": true,
> +      "id": "drive_image1"}}' '{"return": {}}'
> +
> +echo
> +echo === Src VM: Mirror to dst NBD for outgoing storage migration ===
> +echo
> +
> +_send_qemu_cmd $h1 '{"execute": "blockdev-add", "arguments":
> +    {"node-name": "mirror", "driver": "nbd",
> +     "server": {"type": "inet", "host": "127.0.0.1", "port": "'$port'"},
> +     "export": "drive_image1", "tls-creds": "tls0",
> +     "tls-hostname": "127.0.0.1"}}' '{"return": {}}' | sed "s/\"$port\"/PORT/g"
> +_send_qemu_cmd $h1 '{"execute": "blockdev-mirror", "arguments":
> +    {"sync": "full", "device": "drive_image1", "target": "mirror",
> +     "job-id": "drive_image1_53"}}' '{"return": {}}'
> +_timed_wait_for $h1 '"ready"'
> +
> +echo
> +echo === Cleaning up ===
> +echo
> +
> +_send_qemu_cmd $h1 '{"execute":"quit"}' ''
> +_send_qemu_cmd $h2 '{"execute":"quit"}' ''

Since the bug was exposed by this point, I didn't bother to do a clean
shutdown of the mirror job or NBD export.  As is, testing that we shut
down cleanly despite abandoning a job is probably not a bad idea.

> +
> +echo "*** done"
> +rm -f $seq.full
> +status=0
> 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 00000000000..b5e12dcbe7a
> --- /dev/null
> +++ b/tests/qemu-iotests/tests/nbd-tls-iothread.out
> @@ -0,0 +1,54 @@
> +QA output created by nbd-tls-iothread
> +
> +== preparing TLS creds and spare port ==
> +picked unused port
> +Generating a self signed certificate...
> +Generating a signed certificate...
> +Generating a signed certificate...
> +
> +== preparing image ==
> +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
> +
> +=== Starting Src QEMU ===
> +
> +{"execute": "qmp_capabilities"}
> +{"return": {}}
> +
> +=== Starting Dst VM2 ===
> +
> +{"execute": "qmp_capabilities"}
> +{"return": {}}
> +
> +=== Dst VM: Enable NBD server for incoming storage migration ===
> +
> +{"execute": "nbd-server-start", "arguments":
> +    {"addr": {"type": "inet", "data": {"host": "127.0.0.1", "port": PORT}},
> +              "tls-creds": "tls0"}}
> +{"return": {}}
> +{"execute": "block-export-add", "arguments":
> +    {"node-name": "drive_image1", "type": "nbd", "writable": true,
> +      "id": "drive_image1"}}
> +{"return": {}}
> +
> +=== Src VM: Mirror to dst NBD for outgoing storage migration ===
> +
> +{"execute": "blockdev-add", "arguments":
> +    {"node-name": "mirror", "driver": "nbd",
> +     "server": {"type": "inet", "host": "127.0.0.1", "port": PORT},
> +     "export": "drive_image1", "tls-creds": "tls0",
> +     "tls-hostname": "127.0.0.1"}}
> +{"return": {}}
> +{"execute": "blockdev-mirror", "arguments":
> +    {"sync": "full", "device": "drive_image1", "target": "mirror",
> +     "job-id": "drive_image1_53"}}
> +{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "created", "id": "drive_image1_53"}}
> +{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "running", "id": "drive_image1_53"}}
> +{"return": {}}
> +{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "ready", "id": "drive_image1_53"}}
> +
> +=== Cleaning up ===
> +
> +{"execute":"quit"}
> +{"execute":"quit"}
> +*** done
> -- 
> 2.45.0
> 
> 

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

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

[Daniel P. Berrangé]

On Fri, May 17, 2024 at 10:08:08PM -0500, Eric Blake wrote:
> 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-stable@nongnu.org
> > Signed-off-by: Eric Blake <eblake@redhat.com>
> > ---
> >  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

> > +
> > +# 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

I checked most of the relevant history, and this Copyright statement
does indeed appear correct - the code was all written by Richard.

Thus, you could invoke Red Hat's right to re-license and just declare
this copy to be under QEMU's normal GPL license, avoiding the question
of copying the license text.


> > +#
> > +# 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=$(( 50000 + (RANDOM%15000) ))
> > +    while ss -ltn | grep -sqE ":$port\b"; do
> > +        ((port++))
> > +        if [ $port -eq 65000 ]; then port=50000; fi
> 
> Also, common.nbd only probes:
>     for ((port = 10809; port <= 10909; port++))
> and nbdkit's choice of starting with a random offset is interesting.

Yes, a random offset is a nice idea, massively reducing risk of
clashes through (un)lucky concurrent execution.



> > +echo
> > +echo === Cleaning up ===
> > +echo
> > +
> > +_send_qemu_cmd $h1 '{"execute":"quit"}' ''
> > +_send_qemu_cmd $h2 '{"execute":"quit"}' ''
> 
> Since the bug was exposed by this point, I didn't bother to do a clean
> shutdown of the mirror job or NBD export.  As is, testing that we shut
> down cleanly despite abandoning a job is probably not a bad idea.

Yeah, perhaps worthwhile, if you can get something that works reliably.
A reliable partial test is better than an unreliable full test, as we'll
just end up  killing the latter.

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

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

[Kevin Wolf]

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-stable@nongnu.org
> Signed-off-by: Eric Blake <eblake@redhat.com>

> 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 00000000000..b5e12dcbe7a
> --- /dev/null
> +++ b/tests/qemu-iotests/tests/nbd-tls-iothread.out
> @@ -0,0 +1,54 @@
> +QA output created by nbd-tls-iothread
> +
> +== preparing TLS creds and spare port ==
> +picked unused port
> +Generating a self signed certificate...
> +Generating a signed certificate...
> +Generating a signed certificate...
> +
> +== preparing image ==
> +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. :-)

Kevin

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-stable@nongnu.org
> > Signed-off-by: Eric Blake <eblake@redhat.com>
> 
> > 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 00000000000..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 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