Re: [PATCH v2 2/2] nbd: Add testsuite coverage

[Eric Blake]

On 2/28/20 4:55 AM, Jose E. Marchesi wrote:
> Hi Eric.
>
>      +Using NBD connections in tests
>      +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>      +
>      +If your test requires an NBD server (only useful when poke is compiled
>      +with libnbd), the dg-nbd directive is what you need.  It has one form::
>      +
>      +  /* { dg-nbd foo.data /tmp/pokenbd.[pid]/sock } */
>      +
>      +This serves a previously-created temporary file (use dg-data with the
>      +named file form) over the named Unix socket.  The server at this
>      +socket is provided by nbdkit, and will be shut down gracefully when
>      +the testsuite completes.
>      +
>      +To use the server as an IO space, your test will then follow up with::
>      +
>      +  /* { dg-command "open 
> (\"nbd+unix:///?socket=/tmp/pokenbd.[pid]/sock\")" } */
>
> Does NBDKIT == "yes" imply an unix environment?  If not, we may need to
> add extra logic to skip these tests in non-unix systems (such as mingw
> or cygwin.)

To my knowledge, no one has tried to port libnbd or nbdkit to Cygwin yet 
(although I suspect that porting to Cygwin will be easy, while porting 
to mingw will require some heavy effort since native Windows sockets 
tend to need wrappers).  Cygwin supports Unix sockets, but mingw does 
not.  So you are correct that this may need tweaks down the road (either 
to support whatever mingw actually can do, or to skip on mingw where 
Unix sockets are not available); but given that libnbd and nbdkit are 
not yet build for either platform (at least, not in my quick web 
search), it shouldn't be a problem for now.

>      
>      +# Create a temporary NBD server for a file previously create with the
>
> s/create/created/
>
>           # Uncomment the following couple of lines to run the testsuite with
>      -    # valgring.
>      +    # valgrind.
>
> Spurious change?

Typo fix. But I can separate it out and do a general typo fixup patch 
instead if desired.

>      diff --git a/testsuite/poke.cmd/nbd.pk b/testsuite/poke.cmd/nbd.pk
>      new file mode 100644
>      index 00000000..f7296da9
>      --- /dev/null
>      +++ b/testsuite/poke.cmd/nbd.pk
>      @@ -0,0 +1,8 @@
>      +/* { dg-do run } */
>      +/* { dg-data {c*} {0x10 0x20 0x30 0x40 0x50 0x60 0x70 0x80} nbddata } */
>      +/* { dg-nbd nbddata /tmp/pokenbd.[pid]/sock } */
>      +
>      +/* { dg-command ".nbd nbd+unix:///?socket=/tmp/pokenbd.[pid]/sock" } */
>      +/* { dg-command {.set obase 16} }  */
>      +/* { dg-command { byte[8] @ 0#B } } */
>      +/* { dg-output {[0x10,0x20,0x30,0x40,0x50,0x60,0x70,0x80]} } */
>
> Please call the test nbd-1.pk.  This will avoid renames when we add more
> tests for the .nbd command.

Good idea.  Another reason for me to respin this patch is to follow up 
with my idea of using nbdkit's data server, rather than requiring a 
temporary file.  Also, I noticed that typing '/tmp/pokenbd.[pid]/' is 
long and would only get longer to actually honor $TMPDIR, I may 
implement a helper function [dg-tmpdir] that factors that out.

>      diff --git a/testsuite/poke.pkl/open-3.pk b/testsuite/poke.pkl/open-3.pk
>      new file mode 100644
>      index 00000000..57b45f2e
>      --- /dev/null
>      +++ b/testsuite/poke.pkl/open-3.pk
>      @@ -0,0 +1,8 @@
>      +/* { dg-do run } */
>      +/* { dg-data {c*} {0x10} nbddata } */
>      +/* { dg-nbd nbddata /tmp/pokenbd.[pid]/open-3 } */
>      +
>      +/* { dg-command { .set obase 10 } } */
>      +/* { dg-command "defvar foo = open 
> (\"nbd+unix:///?socket=/tmp/pokenbd.[pid]/open-3\")" } */
>      +/* { dg-command { get_ios == foo } } */
>      +/* { dg-output "1" } */
>
> Very nice :)
> We are gonna also need a few tests mapping stuff (both read and write)
> in a NBD ios.

I'll add some.  Glad to see the ideas for how to test this have worked 
out so far.  And even though patch 1 is ready, I'll feel a bit more 
comfortable posting the next revision of patch 2 before committing to 
the series (I also want to post patches overhauling the ios-dev.h 
.get_c/.put_c into .pread/.pwrite, and if those land first, it's less 
churn on the ios-dev-nbd.c patch).

--
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org