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 :|