[Qemu-devel] [PATCH v3 0/5] iotests: Let 233 run concurrently

[Qemu-devel] [PATCH v3 0/5] iotests: Let 233 run concurrently

[Max Reitz]

Currently, 233 cannot reliably run concurrently to other NBD TCP tests.
When it starts, it looks for a free port and then attempts to use that
for the whole duration of the test run.  This is a TOCTTOU race
condition: It does not reserve that port, so another NBD TCP test that
runs in parallel can grab it.

To fix this, we must not use the same port all the time, but always
choose a new one when qemu-nbd is started.  We cannot check whether it
is free, but must let qemu-nbd do so and take it atomically.  We can
achieve this by using the existing --fork option.

There are two problems with --fork, however.  First, it does not give us
a chance to reliably get the server’s PID, which we need.  We can change
that by letting qemu-nbd (optionally) write a PID file, though.  (Which
makes sense if we have a daemon mode.)

Second, it currently discards all output after the server has been
started.  That looks like an accident to me, because we clearly try to
restore the old stderr channel after having redirected all startup
messages to the parent process.  If it is a bug, we can fix it.


v3:
- Patch 1: Dropped “pid_file” variable, so it actually compiles...


git backport-diff of v3 against v2:

Key:
[----] : patches are identical
[####] : number of functional differences between upstream/downstream patch
[down] : patch is downstream-only
The flags [FC] indicate (F)unctional and (C)ontextual differences, respectively

001/5:[0001] [FC] 'qemu-nbd: Add --pid-file option'
002/5:[----] [--] 'iotests.py: Add qemu_nbd_early_pipe()'
003/5:[----] [--] 'qemu-nbd: Do not close stderr'
004/5:[----] [--] 'iotests: Use qemu-nbd's --pid-file'
005/5:[----] [--] 'iotests: Let 233 run concurrently'


v2:
- Patch 1:
  - Use qemu_write_pidfile() [Dan]
  - %s/pid_path/pid_filename/ [Eric]
- Patch 4: Drop the now superfluous subshell [Eric]
  (Didn’t touch _qemu_img_wrapper, because, well, it doesn’t belong in
  this series?)
- Patch 5:
  - s/racey/racy/ [Eric]
  - Unite the “rm -f”s [Eric]
  (Did not address the “FIFO filling up” problem, because 64 kB of FIFO
  space ought to be enough.  Also, cat-ing around that felt weird.)


git backport-diff of v2 against v1:

Key:
[----] : patches are identical
[####] : number of functional differences between upstream/downstream patch
[down] : patch is downstream-only
The flags [FC] indicate (F)unctional and (C)ontextual differences, respectively

001/5:[0025] [FC] 'qemu-nbd: Add --pid-file option'
002/5:[----] [--] 'iotests.py: Add qemu_nbd_early_pipe()'
003/5:[----] [--] 'qemu-nbd: Do not close stderr'
004/5:[0006] [FC] 'iotests: Use qemu-nbd's --pid-file'
005/5:[0003] [FC] 'iotests: Let 233 run concurrently'


Max Reitz (5):
  qemu-nbd: Add --pid-file option
  iotests.py: Add qemu_nbd_early_pipe()
  qemu-nbd: Do not close stderr
  iotests: Use qemu-nbd's --pid-file
  iotests: Let 233 run concurrently

 qemu-nbd.c                    | 14 +++++-
 qemu-nbd.texi                 |  2 +
 tests/qemu-iotests/147        |  4 +-
 tests/qemu-iotests/233        |  1 -
 tests/qemu-iotests/common.nbd | 94 ++++++++++++++++-------------------
 tests/qemu-iotests/common.rc  |  6 +--
 tests/qemu-iotests/iotests.py |  9 ++--
 7 files changed, 67 insertions(+), 63 deletions(-)

-- 
2.21.0

[Qemu-devel] [PATCH v3 1/5] qemu-nbd: Add --pid-file option

[Max Reitz]

--fork is a bit boring if there is no way to get the child's PID.  This
option helps.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 qemu-nbd.c    | 11 +++++++++++
 qemu-nbd.texi |  2 ++
 2 files changed, 13 insertions(+)

diff --git a/qemu-nbd.c b/qemu-nbd.c
index dca9e72cee..edb5195208 100644
--- a/qemu-nbd.c
+++ b/qemu-nbd.c
@@ -59,6 +59,7 @@
 #define QEMU_NBD_OPT_IMAGE_OPTS    262
 #define QEMU_NBD_OPT_FORK          263
 #define QEMU_NBD_OPT_TLSAUTHZ      264
+#define QEMU_NBD_OPT_PID_FILE      265
 
 #define MBR_SIZE 512
 
@@ -111,6 +112,7 @@ static void usage(const char *name)
 "                            specify tracing options\n"
 "  --fork                    fork off the server process and exit the parent\n"
 "                            once the server is running\n"
+"  --pid-file=PATH           store the server's process ID in the given file\n"
 #if HAVE_NBD_DEVICE
 "\n"
 "Kernel NBD client support:\n"
@@ -651,6 +653,7 @@ int main(int argc, char **argv)
         { "image-opts", no_argument, NULL, QEMU_NBD_OPT_IMAGE_OPTS },
         { "trace", required_argument, NULL, 'T' },
         { "fork", no_argument, NULL, QEMU_NBD_OPT_FORK },
+        { "pid-file", required_argument, NULL, QEMU_NBD_OPT_PID_FILE },
         { NULL, 0, NULL, 0 }
     };
     int ch;
@@ -677,6 +680,7 @@ int main(int argc, char **argv)
     bool list = false;
     int old_stderr = -1;
     unsigned socket_activation;
+    const char *pid_file_name = NULL;
 
     /* The client thread uses SIGTERM to interrupt the server.  A signal
      * handler ensures that "qemu-nbd -v -c" exits with a nice status code.
@@ -876,6 +880,9 @@ int main(int argc, char **argv)
         case 'L':
             list = true;
             break;
+        case QEMU_NBD_OPT_PID_FILE:
+            pid_file_name = optarg;
+            break;
         }
     }
 
@@ -1196,6 +1203,10 @@ int main(int argc, char **argv)
 
     nbd_update_server_watch();
 
+    if (pid_file_name) {
+        qemu_write_pidfile(pid_file_name, &error_fatal);
+    }
+
     /* now when the initialization is (almost) complete, chdir("/")
      * to free any busy filesystems */
     if (chdir("/") < 0) {
diff --git a/qemu-nbd.texi b/qemu-nbd.texi
index de342c76b8..7f55657722 100644
--- a/qemu-nbd.texi
+++ b/qemu-nbd.texi
@@ -117,6 +117,8 @@ option; or provide the credentials needed for connecting as a client
 in list mode.
 @item --fork
 Fork off the server process and exit the parent once the server is running.
+@item --pid-file=PATH
+Store the server's process ID in the given file.
 @item --tls-authz=ID
 Specify the ID of a qauthz object previously created with the
 --object option. This will be used to authorize connecting users
-- 
2.21.0

Re: [Qemu-devel] [PATCH v3 1/5] qemu-nbd: Add --pid-file option

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 2806 bytes --]

On 5/8/19 4:18 PM, Max Reitz wrote:
> --fork is a bit boring if there is no way to get the child's PID.  This
> option helps.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  qemu-nbd.c    | 11 +++++++++++
>  qemu-nbd.texi |  2 ++
>  2 files changed, 13 insertions(+)
> 
> diff --git a/qemu-nbd.c b/qemu-nbd.c
> index dca9e72cee..edb5195208 100644
> --- a/qemu-nbd.c
> +++ b/qemu-nbd.c
> @@ -59,6 +59,7 @@
>  #define QEMU_NBD_OPT_IMAGE_OPTS    262
>  #define QEMU_NBD_OPT_FORK          263
>  #define QEMU_NBD_OPT_TLSAUTHZ      264
> +#define QEMU_NBD_OPT_PID_FILE      265
>  
>  #define MBR_SIZE 512
>  
> @@ -111,6 +112,7 @@ static void usage(const char *name)
>  "                            specify tracing options\n"
>  "  --fork                    fork off the server process and exit the parent\n"
>  "                            once the server is running\n"
> +"  --pid-file=PATH           store the server's process ID in the given file\n"

Are we guaranteed that the pid file does not appear until after the
socket has been created?

/me rereads full file...

> @@ -876,6 +880,9 @@ int main(int argc, char **argv)
>          case 'L':
>              list = true;
>              break;
> +        case QEMU_NBD_OPT_PID_FILE:
> +            pid_file_name = optarg;
> +            break;
>          }
>      }
>  
...
    socket_activation = check_socket_activation();
    if (socket_activation == 0) {
        setup_address_and_port(&bindto, &port);
    } else {
        /* Using socket activation - check user didn't use -p etc. */
...

> @@ -1196,6 +1203,10 @@ int main(int argc, char **argv)
>  
>      nbd_update_server_watch();
>  
> +    if (pid_file_name) {
> +        qemu_write_pidfile(pid_file_name, &error_fatal);
> +    }

Yes, we are.  So,

> +
>      /* now when the initialization is (almost) complete, chdir("/")
>       * to free any busy filesystems */
>      if (chdir("/") < 0) {
> diff --git a/qemu-nbd.texi b/qemu-nbd.texi
> index de342c76b8..7f55657722 100644
> --- a/qemu-nbd.texi
> +++ b/qemu-nbd.texi
> @@ -117,6 +117,8 @@ option; or provide the credentials needed for connecting as a client
>  in list mode.
>  @item --fork
>  Fork off the server process and exit the parent once the server is running.
> +@item --pid-file=PATH
> +Store the server's process ID in the given file.

since that is a useful trick to know (the pid file intentionally does
NOT appear until after the socket is ready to go), we should mention it
in the documentation.

I can make that tweak while queueing, if you'd like.

Reviewed-by: Eric Blake <eblake@redhat.com>

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [Qemu-devel] [PATCH v3 1/5] qemu-nbd: Add --pid-file option

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 2892 bytes --]

On 24.05.19 17:07, Eric Blake wrote:
> On 5/8/19 4:18 PM, Max Reitz wrote:
>> --fork is a bit boring if there is no way to get the child's PID.  This
>> option helps.
>>
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>> ---
>>  qemu-nbd.c    | 11 +++++++++++
>>  qemu-nbd.texi |  2 ++
>>  2 files changed, 13 insertions(+)
>>
>> diff --git a/qemu-nbd.c b/qemu-nbd.c
>> index dca9e72cee..edb5195208 100644
>> --- a/qemu-nbd.c
>> +++ b/qemu-nbd.c
>> @@ -59,6 +59,7 @@
>>  #define QEMU_NBD_OPT_IMAGE_OPTS    262
>>  #define QEMU_NBD_OPT_FORK          263
>>  #define QEMU_NBD_OPT_TLSAUTHZ      264
>> +#define QEMU_NBD_OPT_PID_FILE      265
>>  
>>  #define MBR_SIZE 512
>>  
>> @@ -111,6 +112,7 @@ static void usage(const char *name)
>>  "                            specify tracing options\n"
>>  "  --fork                    fork off the server process and exit the parent\n"
>>  "                            once the server is running\n"
>> +"  --pid-file=PATH           store the server's process ID in the given file\n"
> 
> Are we guaranteed that the pid file does not appear until after the
> socket has been created?
> 
> /me rereads full file...
> 
>> @@ -876,6 +880,9 @@ int main(int argc, char **argv)
>>          case 'L':
>>              list = true;
>>              break;
>> +        case QEMU_NBD_OPT_PID_FILE:
>> +            pid_file_name = optarg;
>> +            break;
>>          }
>>      }
>>  
> ...
>     socket_activation = check_socket_activation();
>     if (socket_activation == 0) {
>         setup_address_and_port(&bindto, &port);
>     } else {
>         /* Using socket activation - check user didn't use -p etc. */
> ...
> 
>> @@ -1196,6 +1203,10 @@ int main(int argc, char **argv)
>>  
>>      nbd_update_server_watch();
>>  
>> +    if (pid_file_name) {
>> +        qemu_write_pidfile(pid_file_name, &error_fatal);
>> +    }
> 
> Yes, we are.  So,
> 
>> +
>>      /* now when the initialization is (almost) complete, chdir("/")
>>       * to free any busy filesystems */
>>      if (chdir("/") < 0) {
>> diff --git a/qemu-nbd.texi b/qemu-nbd.texi
>> index de342c76b8..7f55657722 100644
>> --- a/qemu-nbd.texi
>> +++ b/qemu-nbd.texi
>> @@ -117,6 +117,8 @@ option; or provide the credentials needed for connecting as a client
>>  in list mode.
>>  @item --fork
>>  Fork off the server process and exit the parent once the server is running.
>> +@item --pid-file=PATH
>> +Store the server's process ID in the given file.
> 
> since that is a useful trick to know (the pid file intentionally does
> NOT appear until after the socket is ready to go), we should mention it
> in the documentation.
> 
> I can make that tweak while queueing, if you'd like.

I don’t mind, but I personally find it more useful to just use --fork
and wait for the main process to exit.

Max


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[Qemu-devel] [PATCH v3 2/5] iotests.py: Add qemu_nbd_early_pipe()

[Max Reitz]

qemu_nbd_pipe() currently unconditionally reads qemu-nbd's output.  That
is not ideal because qemu-nbd may keep stderr open after the parent
process has exited.

Currently, the only user of qemu_nbd_pipe() is 147, which discards the
whole output if the parent process returned success and only evaluates
it on error.  Therefore, we can replace qemu_nbd_pipe() by
qemu_nbd_early_pipe() that does the same: Discard the output on success,
and return it on error.

Signed-off-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 tests/qemu-iotests/147        | 4 ++--
 tests/qemu-iotests/iotests.py | 9 ++++++---
 2 files changed, 8 insertions(+), 5 deletions(-)

diff --git a/tests/qemu-iotests/147 b/tests/qemu-iotests/147
index 82513279b0..2d84fddb01 100755
--- a/tests/qemu-iotests/147
+++ b/tests/qemu-iotests/147
@@ -24,7 +24,7 @@ import socket
 import stat
 import time
 import iotests
-from iotests import cachemode, imgfmt, qemu_img, qemu_nbd, qemu_nbd_pipe
+from iotests import cachemode, imgfmt, qemu_img, qemu_nbd, qemu_nbd_early_pipe
 
 NBD_PORT_START      = 32768
 NBD_PORT_END        = NBD_PORT_START + 1024
@@ -93,7 +93,7 @@ class QemuNBD(NBDBlockdevAddBase):
             pass
 
     def _try_server_up(self, *args):
-        status, msg = qemu_nbd_pipe('-f', imgfmt, test_img, *args)
+        status, msg = qemu_nbd_early_pipe('-f', imgfmt, test_img, *args)
         if status == 0:
             return True
         if 'Address already in use' in msg:
diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index f811f69135..ce21d83182 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -204,9 +204,9 @@ def qemu_nbd(*args):
     '''Run qemu-nbd in daemon mode and return the parent's exit code'''
     return subprocess.call(qemu_nbd_args + ['--fork'] + list(args))
 
-def qemu_nbd_pipe(*args):
+def qemu_nbd_early_pipe(*args):
     '''Run qemu-nbd in daemon mode and return both the parent's exit code
-       and its output'''
+       and its output in case of an error'''
     subp = subprocess.Popen(qemu_nbd_args + ['--fork'] + list(args),
                             stdout=subprocess.PIPE,
                             stderr=subprocess.STDOUT,
@@ -216,7 +216,10 @@ def qemu_nbd_pipe(*args):
         sys.stderr.write('qemu-nbd received signal %i: %s\n' %
                          (-exitcode,
                           ' '.join(qemu_nbd_args + ['--fork'] + list(args))))
-    return exitcode, subp.communicate()[0]
+    if exitcode == 0:
+        return exitcode, ''
+    else:
+        return exitcode, subp.communicate()[0]
 
 def compare_images(img1, img2, fmt1=imgfmt, fmt2=imgfmt):
     '''Return True if two image files are identical'''
-- 
2.21.0

[Qemu-devel] [PATCH v3 3/5] qemu-nbd: Do not close stderr

[Max Reitz]

We kept old_stderr specifically so we could keep emitting error message
on stderr.  However, qemu_daemon() closes stderr.  Therefore, we need to
dup() stderr to old_stderr before invoking qemu_daemon().

Signed-off-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 qemu-nbd.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/qemu-nbd.c b/qemu-nbd.c
index edb5195208..e4446d479c 100644
--- a/qemu-nbd.c
+++ b/qemu-nbd.c
@@ -1014,10 +1014,11 @@ int main(int argc, char **argv)
             exit(EXIT_FAILURE);
         } else if (pid == 0) {
             close(stderr_fd[0]);
+
+            old_stderr = dup(STDERR_FILENO);
             ret = qemu_daemon(1, 0);
 
             /* Temporarily redirect stderr to the parent's pipe...  */
-            old_stderr = dup(STDERR_FILENO);
             dup2(stderr_fd[1], STDERR_FILENO);
             if (ret < 0) {
                 error_report("Failed to daemonize: %s", strerror(errno));
-- 
2.21.0

[Qemu-devel] [PATCH v3 4/5] iotests: Use qemu-nbd's --pid-file

[Max Reitz]

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 tests/qemu-iotests/common.rc | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/tests/qemu-iotests/common.rc b/tests/qemu-iotests/common.rc
index 93f87389b6..5502c3da2f 100644
--- a/tests/qemu-iotests/common.rc
+++ b/tests/qemu-iotests/common.rc
@@ -105,10 +105,8 @@ _qemu_io_wrapper()
 
 _qemu_nbd_wrapper()
 {
-    (
-        echo $BASHPID > "${QEMU_TEST_DIR}/qemu-nbd.pid"
-        exec "$QEMU_NBD_PROG" $QEMU_NBD_OPTIONS "$@"
-    )
+    "$QEMU_NBD_PROG" --pid-file="${QEMU_TEST_DIR}/qemu-nbd.pid" \
+                     $QEMU_NBD_OPTIONS "$@"
 }
 
 _qemu_vxhs_wrapper()
-- 
2.21.0

Re: [Qemu-devel] [PATCH v3 4/5] iotests: Use qemu-nbd's --pid-file

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 937 bytes --]

On 5/8/19 4:18 PM, Max Reitz wrote:
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  tests/qemu-iotests/common.rc | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)

Reviewed-by: Eric Blake <eblake@redhat.com>

> 
> diff --git a/tests/qemu-iotests/common.rc b/tests/qemu-iotests/common.rc
> index 93f87389b6..5502c3da2f 100644
> --- a/tests/qemu-iotests/common.rc
> +++ b/tests/qemu-iotests/common.rc
> @@ -105,10 +105,8 @@ _qemu_io_wrapper()
>  
>  _qemu_nbd_wrapper()
>  {
> -    (
> -        echo $BASHPID > "${QEMU_TEST_DIR}/qemu-nbd.pid"
> -        exec "$QEMU_NBD_PROG" $QEMU_NBD_OPTIONS "$@"
> -    )
> +    "$QEMU_NBD_PROG" --pid-file="${QEMU_TEST_DIR}/qemu-nbd.pid" \
> +                     $QEMU_NBD_OPTIONS "$@"
>  }
>  
>  _qemu_vxhs_wrapper()
> 

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[Qemu-devel] [PATCH v3 5/5] iotests: Let 233 run concurrently

[Max Reitz]

common.nbd's nbd_server_set_tcp_port() tries to find a free port, and
then uses it for the whole test run.  However, this is racy because even
if the port was free at the beginning, there is no guarantee it will
continue to be available.  Therefore, 233 currently cannot reliably be
run concurrently with other NBD TCP tests.

This patch addresses the problem by dropping nbd_server_set_tcp_port(),
and instead finding a new port every time nbd_server_start_tcp_socket()
is invoked.  For this, we run qemu-nbd with --fork and on error evaluate
the output to see whether it contains "Address already in use".  If so,
we try the next port.

On success, we still want to continually redirect the output from
qemu-nbd to stderr.  To achieve both, we redirect qemu-nbd's stderr to a
FIFO that we then open in bash.  If the parent process exits with status
0 (which means that the server has started successfully), we launch a
background cat process that copies the FIFO to stderr.  On failure, we
read the whole content into a variable and then evaluate it.

While at it, use --fork in nbd_server_start_unix_socket(), too.  Doing
so allows us to drop nbd_server_wait_for_*_socket().

Note that the reason common.nbd did not use --fork before is that
qemu-nbd did not have --pid-file.

Signed-off-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 tests/qemu-iotests/233        |  1 -
 tests/qemu-iotests/common.nbd | 94 ++++++++++++++++-------------------
 2 files changed, 42 insertions(+), 53 deletions(-)

diff --git a/tests/qemu-iotests/233 b/tests/qemu-iotests/233
index b8b6c8cc4c..8682ea277c 100755
--- a/tests/qemu-iotests/233
+++ b/tests/qemu-iotests/233
@@ -50,7 +50,6 @@ _supported_proto file
 _supported_os Linux
 _require_command QEMU_NBD
 
-nbd_server_set_tcp_port
 tls_x509_init
 
 echo
diff --git a/tests/qemu-iotests/common.nbd b/tests/qemu-iotests/common.nbd
index 25fc9ffaa4..24b01b60aa 100644
--- a/tests/qemu-iotests/common.nbd
+++ b/tests/qemu-iotests/common.nbd
@@ -22,6 +22,11 @@
 nbd_unix_socket="${TEST_DIR}/qemu-nbd.sock"
 nbd_tcp_addr="127.0.0.1"
 nbd_pid_file="${TEST_DIR}/qemu-nbd.pid"
+nbd_stderr_fifo="${TEST_DIR}/qemu-nbd.fifo"
+
+# If bash version is >= 4.1, this will be overwritten by a dynamically
+# assigned file descriptor value.
+nbd_fifo_fd=10
 
 nbd_server_stop()
 {
@@ -33,77 +38,62 @@ nbd_server_stop()
             kill "$NBD_PID"
         fi
     fi
-    rm -f "$nbd_unix_socket"
-}
-
-nbd_server_wait_for_unix_socket()
-{
-    pid=$1
-
-    for ((i = 0; i < 300; i++))
-    do
-        if [ -r "$nbd_unix_socket" ]; then
-            return
-        fi
-        kill -s 0 $pid 2>/dev/null
-        if test $? != 0
-        then
-            echo "qemu-nbd unexpectedly quit"
-            exit 1
-        fi
-        sleep 0.1
-    done
-    echo "Failed in check of unix socket created by qemu-nbd"
-    exit 1
+    rm -f "$nbd_unix_socket" "$nbd_stderr_fifo"
 }
 
 nbd_server_start_unix_socket()
 {
     nbd_server_stop
-    $QEMU_NBD -v -t -k "$nbd_unix_socket" "$@" &
-    nbd_server_wait_for_unix_socket $!
+    $QEMU_NBD -v -t -k "$nbd_unix_socket" --fork "$@"
 }
 
-nbd_server_set_tcp_port()
+nbd_server_start_tcp_socket()
 {
-    (ss --help) >/dev/null 2>&1 || _notrun "ss utility not found, skipping test"
+    nbd_server_stop
 
+    mkfifo "$nbd_stderr_fifo"
     for ((port = 10809; port <= 10909; port++))
     do
-        if ! ss -tln | grep -sqE ":$port\b"; then
+        # Redirect stderr to FIFO, so we can later decide whether we
+        # want to read it or to redirect it to our stderr, depending
+        # on whether the command fails or not
+        $QEMU_NBD -v -t -b $nbd_tcp_addr -p $port --fork "$@" \
+            2> "$nbd_stderr_fifo" &
+
+        # Taken from common.qemu
+        if [[ "${BASH_VERSINFO[0]}" -ge "5" ||
+            ("${BASH_VERSINFO[0]}" -ge "4" && "${BASH_VERSINFO[1]}" -ge "1") ]]
+        then
+            exec {nbd_fifo_fd}<"$nbd_stderr_fifo"
+        else
+            let _nbd_fifo_fd++
+            eval "exec ${_nbd_fifo_fd}<'$nbd_stderr_fifo'"
+        fi
+        wait $!
+
+        if test $? == 0
+        then
+            # Success, redirect qemu-nbd's stderr to our stderr
             nbd_tcp_port=$port
+            (cat <&$nbd_fifo_fd >&2) &
+            eval "exec $nbd_fifo_fd>&-"
             return
         fi
-    done
 
-    echo "Cannot find free TCP port for nbd in range 10809-10909"
-    exit 1
-}
-
-nbd_server_wait_for_tcp_socket()
-{
-    pid=$1
+        # Failure, read the output
+        output=$(cat <&$nbd_fifo_fd)
+        eval "exec $nbd_fifo_fd>&-"
 
-    for ((i = 0; i < 300; i++))
-    do
-        if ss -tln | grep -sqE ":$nbd_tcp_port\b"; then
-            return
-        fi
-        kill -s 0 $pid 2>/dev/null
-        if test $? != 0
+        if ! echo "$output" | grep -q "Address already in use"
         then
-            echo "qemu-nbd unexpectedly quit"
+            # Unknown error, print it
+            echo "$output" >&2
+            rm -f "$nbd_stderr_fifo"
             exit 1
         fi
-        sleep 0.1
     done
-    echo "Failed in check of TCP socket created by qemu-nbd"
-    exit 1
-}
 
-nbd_server_start_tcp_socket()
-{
-    nbd_server_stop
-    $QEMU_NBD -v -t -b $nbd_tcp_addr -p $nbd_tcp_port "$@" &
-    nbd_server_wait_for_tcp_socket $!
+    echo "Cannot find free TCP port for nbd in range 10809-10909"
+    rm -f "$nbd_stderr_fifo"
+    exit 1
 }
-- 
2.21.0

Re: [Qemu-devel] [PATCH v3 0/5] iotests: Let 233 run concurrently

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1479 bytes --]

On 5/8/19 4:18 PM, Max Reitz wrote:
> Currently, 233 cannot reliably run concurrently to other NBD TCP tests.
> When it starts, it looks for a free port and then attempts to use that
> for the whole duration of the test run.  This is a TOCTTOU race
> condition: It does not reserve that port, so another NBD TCP test that
> runs in parallel can grab it.
> 
> To fix this, we must not use the same port all the time, but always
> choose a new one when qemu-nbd is started.  We cannot check whether it
> is free, but must let qemu-nbd do so and take it atomically.  We can
> achieve this by using the existing --fork option.
> 
> There are two problems with --fork, however.  First, it does not give us
> a chance to reliably get the server’s PID, which we need.  We can change
> that by letting qemu-nbd (optionally) write a PID file, though.  (Which
> makes sense if we have a daemon mode.)
> 
> Second, it currently discards all output after the server has been
> started.  That looks like an accident to me, because we clearly try to
> restore the old stderr channel after having redirected all startup
> messages to the parent process.  If it is a bug, we can fix it.
> 
> 
> v3:
> - Patch 1: Dropped “pid_file” variable, so it actually compiles...
> 

Thanks; will apply to my NBD tree, and send a PR Monday.

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]