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-sta...@nongnu.org > > Signed-off-by: Eric Blake <ebl...@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 :|