Libguestfs - May 2020 - Re: [PATCH nbdkit] tests/test-truncate4.sh: Rewrite to use nbdsh.

This commit works for me, but TBH I'm not clear on what the actual
purpose of this test is supposed to be.

Rich.

The old test relied on issuing an asynchronous command to a qemu-io
subprocess, requiring a sleep to ensure that the command had been
carried out.  ‘sleep 1’ was not long enough on s390x as you can see
from this partial trace:

    + exec
    + qemu-io -f raw nbd:unix:/tmp/tmp.jqOZGj6dnX
    + exec
    + echo 'Reading from connection A, try 1'
    Reading from connection A, try 1
    + echo 'r -P 1 0 1024'
    + sleep 1

Above we issue the command and sleep while it executes.  However:

    nbdkit: debug: accepted connection
    nbdkit: file[1]: debug: truncate: preconnect
    nbdkit: file[1]: debug: file: preconnect
    nbdkit: file[1]: debug: newstyle negotiation: flags: global 0x3
    + echo 'Resizing down'
    Resizing down
    + truncate -s 512 truncate4.data
    nbdkit: file[1]: debug: newstyle negotiation: client flags: 0x3
    nbdkit: file[1]: debug: newstyle negotiation: NBD_OPT_STRUCTURED_REPLY:
client requested structured replies

Here are still connecting to nbdkit from "connection A", long after
the sleep has finished and the truncate has been done.

Instead of increasing the sleep and hoping for the best, I chose to
rewrite the test to use nbdsh.  This allows us to control both
connections from a single process, making the test predicatable and
easier to understand.
---
 tests/test-truncate4.sh | 90 ++++++++++++++++++++++++++---------------
 1 file changed, 58 insertions(+), 32 deletions(-)

diff --git a/tests/test-truncate4.sh b/tests/test-truncate4.sh
index e4be626b..c5dc608c 100755
--- a/tests/test-truncate4.sh
+++ b/tests/test-truncate4.sh
@@ -1,6 +1,6 @@
 #!/usr/bin/env bash
 # nbdkit
-# Copyright (C) 2019 Red Hat Inc.
+# Copyright (C) 2019-2020 Red Hat Inc.
 #
 # Redistribution and use in source and binary forms, with or without
 # modification, are permitted provided that the following conditions are
@@ -31,55 +31,81 @@
 # SUCH DAMAGE.
 
 # Regression test when next_ops->get_size changes between connections.
+#
 # For now, NBD does not support dynamic resize; but the file plugin
 # reads size from the file system for each new connection, at which
 # point the client remembers that size for the life of the connection.
+#
 # We are testing that connection A can still see the tail of a file,
 # even when connection B is opened while the file was temporarily
-# shorter (if the actions of connection B affect the size visible
-# through connection A, we didn't isolate per-connection state).
+# shorter.  If the actions of connection B affect the size visible
+# through connection A, we didn't isolate per-connection state.
 
 source ./functions.sh
 set -e
 set -x
 
-requires qemu-io --version
+requires nbdsh --version
+requires truncate --help
+
+if ! dd --help | grep fullblock; then
+    echo "$0: requires GNU dd with the iflag=fullblock option"
+    exit 77
+fi
 
 sock=`mktemp -u`
-files="truncate4.out truncate4.pid $sock truncate4.data"
+data=truncate4.data
+files="truncate4.pid $sock $data"
 rm -f $files
 cleanup_fn rm -f $files
 
-# Initial file contents: 1k of pattern 1
-truncate -s 1024 truncate4.data
-qemu-io -c 'w -P 1 0 1024' -f raw truncate4.data
+truncate -s 0 $data
 
 # Run nbdkit with file plugin and truncate filter in front.
 start_nbdkit -P truncate4.pid -U $sock \
        --filter=truncate \
-       file truncate4.data \
+       file $data \
        round-up=1024
 
-fail=0
-exec 4>&1 # Save original stdout
-{
-    exec 5>&1 >&4 # Save connection A, set stdout back to
original
-    echo 'Reading from connection A, try 1'
-    echo 'r -P 1 0 1024' >&5
-    sleep 1
-    echo 'Resizing down'
-    truncate -s 512 truncate4.data
-    echo 'Reading from connection B'
-    echo 'r -P 1 0 512' | qemu-io -f raw nbd:unix:$sock >>
truncate4.out
-    echo 'Restoring size'
-    truncate -s 1024 truncate4.data
-    qemu-io -c 'w -P 2 0 1024' -f raw truncate4.data
-    echo 'Reading from connection A, try 2'
-    echo 'r -P 2 512 512' >&5
-    echo 'quit' >&5
-} | qemu-io -f raw nbd:unix:$sock >> truncate4.out || fail=1
-exec 4>&-
-
-cat truncate4.out
-grep 'Pattern verification failed' truncate4.out && fail=1
-exit $fail
+export data sock
+nbdsh -c '
+import os
+
+data = os.environ["data"]
+sock = os.environ["sock"]
+
+def restore_file():
+    # Original test data, 1024 bytes of "TEST" repeated.
+    with open (data, "w") as file:
+        file.write ("TEST"*256)
+
+restore_file ()
+
+print ("Connection A.", flush=True)
+connA = nbd.NBD ()
+connA.set_handle_name ("A")
+connA.connect_unix (sock)
+print ("Check the size.", flush=True)
+assert connA.get_size () == 1024
+
+print ("Truncate %s to 512 bytes." % data, flush=True)
+os.truncate (data, 512)
+
+print ("Connection B.", flush=True)
+connB = nbd.NBD ()
+connB.set_handle_name ("B")
+connB.connect_unix (sock)
+print ("Check the size.", flush=True)
+assert connB.get_size () == 1024 # because of the round-up parameter
+print ("Read data from connection B.", flush=True)
+buf = connB.pread (1024, 0)
+assert buf == b"TEST"*128 + b"\0"*512
+
+print ("Restore the file size and original data.", flush=True)
+restore_file ()
+
+print ("Read data from connection A.", flush=True)
+buf = connA.pread (1024, 0)
+assert 1024 == len (buf)
+assert buf == b"TEST"*256
+'
-- 
2.25.0

On 5/20/20 7:50 AM, Richard W.M. Jones wrote:
> The old test relied on issuing an asynchronous command to a qemu-io
> subprocess, requiring a sleep to ensure that the command had been
> carried out.  ‘sleep 1’ was not long enough on s390x as you can see
> from this partial trace:
Nice analysis.
> Here are still connecting to nbdkit from "connection A", long
after
> the sleep has finished and the truncate has been done.
> 
> Instead of increasing the sleep and hoping for the best, I chose to
> rewrite the test to use nbdsh.  This allows us to control both
> connections from a single process, making the test predicatable and
> easier to understand.
Indeed.  nbdsh is making a lot of tests easier to write, and it is now 
just a question of revisiting the older tests that predated it.
> ---
>   tests/test-truncate4.sh | 90 ++++++++++++++++++++++++++---------------
>   1 file changed, 58 insertions(+), 32 deletions(-)
> 
> diff --git a/tests/test-truncate4.sh b/tests/test-truncate4.sh
> index e4be626b..c5dc608c 100755
> --- a/tests/test-truncate4.sh
> +++ b/tests/test-truncate4.sh
> -requires qemu-io --version
> +requires nbdsh --version
> +requires truncate --help
We probably don't need truncate...
> +
> +if ! dd --help | grep fullblock; then
> +    echo "$0: requires GNU dd with the iflag=fullblock option"
> +    exit 77
> +fi
>   
>   sock=`mktemp -u`
> -files="truncate4.out truncate4.pid $sock truncate4.data"
> +data=truncate4.data
> +files="truncate4.pid $sock $data"
>   rm -f $files
>   cleanup_fn rm -f $files
>   
> -# Initial file contents: 1k of pattern 1
> -truncate -s 1024 truncate4.data
> -qemu-io -c 'w -P 1 0 1024' -f raw truncate4.data
> +truncate -s 0 $data
...true, the old code used it (without requiring it!), but in the new 
code, you could get away with ': > $data' instead of having to invoke
truncate.  Then where the old code resized the file with truncate 
between connections A and B, the new code is using python.

The conversion itself looks correct to me.  ACK.

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