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

[Qemu-devel] [PATCH 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.


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                    | 32 +++++++++++-
 qemu-nbd.texi                 |  2 +
 tests/qemu-iotests/147        |  4 +-
 tests/qemu-iotests/233        |  1 -
 tests/qemu-iotests/common.nbd | 93 ++++++++++++++++-------------------
 tests/qemu-iotests/common.rc  |  4 +-
 tests/qemu-iotests/iotests.py |  9 ++--
 7 files changed, 85 insertions(+), 60 deletions(-)

-- 
2.20.1

[Qemu-devel] [PATCH 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    | 29 +++++++++++++++++++++++++++++
 qemu-nbd.texi |  2 ++
 2 files changed, 31 insertions(+)

diff --git a/qemu-nbd.c b/qemu-nbd.c
index dca9e72cee..7e48154f44 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,8 @@ int main(int argc, char **argv)
     bool list = false;
     int old_stderr = -1;
     unsigned socket_activation;
+    const char *pid_path = NULL;
+    FILE *pid_file;
 
     /* 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 +881,9 @@ int main(int argc, char **argv)
         case 'L':
             list = true;
             break;
+        case QEMU_NBD_OPT_PID_FILE:
+            pid_path = optarg;
+            break;
         }
     }
 
@@ -1196,6 +1204,27 @@ int main(int argc, char **argv)
 
     nbd_update_server_watch();
 
+    if (pid_path) {
+        pid_file = fopen(pid_path, "w");
+        if (!pid_file) {
+            error_report("Failed to store PID in %s: %s",
+                         pid_path, strerror(errno));
+            exit(EXIT_FAILURE);
+        }
+
+        ret = fprintf(pid_file, "%ld", (long)getpid());
+        if (ret < 0) {
+            ret = -errno;
+        }
+        fclose(pid_file);
+
+        if (ret < 0) {
+            error_report("Failed to store PID in %s: %s",
+                         pid_path, strerror(-ret));
+            exit(EXIT_FAILURE);
+        }
+    }
+
     /* 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.20.1

[Qemu-devel] [PATCH 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>
---
 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.20.1

[Qemu-devel] [PATCH 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>
---
As I hinted at in the cover letter, I am not sure whether this is truly
a bug or whether the current behavior is intentional.  So if you
disagree with me on this patch, you are welcome to suggest an
alternative.

I personally can see two:
(1) Add a --log option for a file to store the server's messages in.
    Seems a bit cumbersome to me.

(2) Add a --keep-stderr option, which specifically enables this behavior
    here.  Without this option we keep the old behavior.
---
 qemu-nbd.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/qemu-nbd.c b/qemu-nbd.c
index 7e48154f44..3805324153 100644
--- a/qemu-nbd.c
+++ b/qemu-nbd.c
@@ -1015,10 +1015,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.20.1

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

[Max Reitz]

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

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

[Qemu-devel] [PATCH 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 racey 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>
---
 tests/qemu-iotests/233        |  1 -
 tests/qemu-iotests/common.nbd | 93 ++++++++++++++++-------------------
 2 files changed, 42 insertions(+), 52 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..e003478a57 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()
 {
@@ -34,76 +39,62 @@ nbd_server_stop()
         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_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.20.1

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

[Eric Blake]

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

On 5/7/19 1:36 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    | 29 +++++++++++++++++++++++++++++
>  qemu-nbd.texi |  2 ++
>  2 files changed, 31 insertions(+)
> 

> @@ -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"

Should --pid-file imply --fork, or be an error if --fork was not
supplied? As coded, it writes a pid file regardless of --fork, even
though it is less obvious that it is useful in that case. I don't have a
strong preference (there doesn't seem to be a useful consensus on what
forking daemons should do), but it would at least be worth documenting
the intended action (even if that implies a tweak to the patch to match
the intent).

>  #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,8 @@ int main(int argc, char **argv)
>      bool list = false;
>      int old_stderr = -1;
>      unsigned socket_activation;
> +    const char *pid_path = NULL;

Bikeshedding: pid_name is nicer (path makes me think of $PATH and other
colon-separated lists, which this is not).

Otherwise, I agree that this is long overdue. Thanks! If you can justify
the behavior without --fork,
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 2/5] iotests.py: Add qemu_nbd_early_pipe()

[Eric Blake]

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

On 5/7/19 1:36 PM, Max Reitz wrote:
> 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>
> ---
>  tests/qemu-iotests/147        | 4 ++--
>  tests/qemu-iotests/iotests.py | 9 ++++++---
>  2 files changed, 8 insertions(+), 5 deletions(-)
> 

> @@ -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]
>  

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 1/5] qemu-nbd: Add --pid-file option

[Max Reitz]

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

On 07.05.19 21:30, Eric Blake wrote:
> On 5/7/19 1:36 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    | 29 +++++++++++++++++++++++++++++
>>  qemu-nbd.texi |  2 ++
>>  2 files changed, 31 insertions(+)
>>
> 
>> @@ -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"
> 
> Should --pid-file imply --fork, or be an error if --fork was not
> supplied? As coded, it writes a pid file regardless of --fork, even
> though it is less obvious that it is useful in that case. I don't have a
> strong preference (there doesn't seem to be a useful consensus on what
> forking daemons should do), but it would at least be worth documenting
> the intended action (even if that implies a tweak to the patch to match
> the intent).

I think the documentation is pretty clear.  It stores the server's PID,
whether it has been forked or not.

I don't think we would gain anything from forbidding --pid-file without
--fork, would we?

>>  #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,8 @@ int main(int argc, char **argv)
>>      bool list = false;
>>      int old_stderr = -1;
>>      unsigned socket_activation;
>> +    const char *pid_path = NULL;
> 
> Bikeshedding: pid_name is nicer (path makes me think of $PATH and other
> colon-separated lists, which this is not).

I'd prefer pid_filename myself, then, because pid_name sounds like a
weird way to say "process name". O:-)

> Otherwise, I agree that this is long overdue. Thanks! If you can justify
> the behavior without --fork,

I just can’t think of a reason not to allow it without --fork.  Maybe a
user doesn’t need --fork because they just start the server in the
background and that’s good enough, but they still want a PID file.  So
basically like common.rc’s _qemu_nbd_wrapper() before this series.

Max


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

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

[Eric Blake]

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

On 5/7/19 1:36 PM, Max Reitz wrote:
> 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>
> ---
> As I hinted at in the cover letter, I am not sure whether this is truly
> a bug or whether the current behavior is intentional.  So if you
> disagree with me on this patch, you are welcome to suggest an
> alternative.

Looks to me like a bug fix, for a problem present since commit c1f8fdc3
added old_stderr in 2011.

> 
> I personally can see two:
> (1) Add a --log option for a file to store the server's messages in.
>     Seems a bit cumbersome to me.
> 
> (2) Add a --keep-stderr option, which specifically enables this behavior
>     here.  Without this option we keep the old behavior.

The approach done here is simplest, I wouldn't worry about your two
alternatives.

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 1/5] qemu-nbd: Add --pid-file option

[Eric Blake]

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

On 5/7/19 2:39 PM, Max Reitz wrote:
> On 07.05.19 21:30, Eric Blake wrote:
>> On 5/7/19 1:36 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    | 29 +++++++++++++++++++++++++++++
>>>  qemu-nbd.texi |  2 ++
>>>  2 files changed, 31 insertions(+)
>>>
>>
>>> @@ -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"
>>
>> Should --pid-file imply --fork, or be an error if --fork was not
>> supplied? As coded, it writes a pid file regardless of --fork, even
>> though it is less obvious that it is useful in that case. I don't have a
>> strong preference (there doesn't seem to be a useful consensus on what
>> forking daemons should do), but it would at least be worth documenting
>> the intended action (even if that implies a tweak to the patch to match
>> the intent).
> 
> I think the documentation is pretty clear.  It stores the server's PID,
> whether it has been forked or not.
> 
> I don't think we would gain anything from forbidding --pid-file without
> --fork, would we?

I can't think of any reason to forbid it. So it sounds like we are
intentional, this writes the pid into --pid-file regardless of whether
that pid can be learned by other means as well.


>>> +    const char *pid_path = NULL;
>>
>> Bikeshedding: pid_name is nicer (path makes me think of $PATH and other
>> colon-separated lists, which this is not).
> 
> I'd prefer pid_filename myself, then, because pid_name sounds like a
> weird way to say "process name". O:-)

Works for me, even if it is longer. Do you want to respin, or just have
me touch it up when folding it into my NBD tree?

-- 
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 4/5] iotests: Use qemu-nbd's --pid-file

[Eric Blake]

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

On 5/7/19 1:36 PM, Max Reitz wrote:
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  tests/qemu-iotests/common.rc | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/tests/qemu-iotests/common.rc b/tests/qemu-iotests/common.rc
> index 93f87389b6..217cf3874d 100644
> --- a/tests/qemu-iotests/common.rc
> +++ b/tests/qemu-iotests/common.rc
> @@ -106,8 +106,8 @@ _qemu_io_wrapper()
>  _qemu_nbd_wrapper()
>  {
>      (
> -        echo $BASHPID > "${QEMU_TEST_DIR}/qemu-nbd.pid"
> -        exec "$QEMU_NBD_PROG" $QEMU_NBD_OPTIONS "$@"
> +        exec "$QEMU_NBD_PROG" --pid-file="${QEMU_TEST_DIR}/qemu-nbd.pid" \
> +                              $QEMU_NBD_OPTIONS "$@"
>      )

Beforehand, we needed the subshell + exec to guarantee that the pid we
were writing was that of the subshell. Now, we don't need either; this
could be simplified to:

_qemu_nbd_wrapper()
{
    "$QEMU_NBD_PROG" --pid-file... "$@"
}

-- 
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 4/5] iotests: Use qemu-nbd's --pid-file

[Max Reitz]

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

On 07.05.19 21:53, Eric Blake wrote:
> On 5/7/19 1:36 PM, Max Reitz wrote:
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>> ---
>>  tests/qemu-iotests/common.rc | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/tests/qemu-iotests/common.rc b/tests/qemu-iotests/common.rc
>> index 93f87389b6..217cf3874d 100644
>> --- a/tests/qemu-iotests/common.rc
>> +++ b/tests/qemu-iotests/common.rc
>> @@ -106,8 +106,8 @@ _qemu_io_wrapper()
>>  _qemu_nbd_wrapper()
>>  {
>>      (
>> -        echo $BASHPID > "${QEMU_TEST_DIR}/qemu-nbd.pid"
>> -        exec "$QEMU_NBD_PROG" $QEMU_NBD_OPTIONS "$@"
>> +        exec "$QEMU_NBD_PROG" --pid-file="${QEMU_TEST_DIR}/qemu-nbd.pid" \
>> +                              $QEMU_NBD_OPTIONS "$@"
>>      )
> 
> Beforehand, we needed the subshell + exec to guarantee that the pid we
> were writing was that of the subshell. Now, we don't need either; this
> could be simplified to:
> 
> _qemu_nbd_wrapper()
> {
>     "$QEMU_NBD_PROG" --pid-file... "$@"
> }

True, but I just followed _qemu_img_wrapper()’s example.  I could change
both, of course...

Max


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

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

[Max Reitz]

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

On 07.05.19 21:51, Eric Blake wrote:
> On 5/7/19 2:39 PM, Max Reitz wrote:
>> On 07.05.19 21:30, Eric Blake wrote:
>>> On 5/7/19 1:36 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    | 29 +++++++++++++++++++++++++++++
>>>>  qemu-nbd.texi |  2 ++
>>>>  2 files changed, 31 insertions(+)
>>>>
>>>
>>>> @@ -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"
>>>
>>> Should --pid-file imply --fork, or be an error if --fork was not
>>> supplied? As coded, it writes a pid file regardless of --fork, even
>>> though it is less obvious that it is useful in that case. I don't have a
>>> strong preference (there doesn't seem to be a useful consensus on what
>>> forking daemons should do), but it would at least be worth documenting
>>> the intended action (even if that implies a tweak to the patch to match
>>> the intent).
>>
>> I think the documentation is pretty clear.  It stores the server's PID,
>> whether it has been forked or not.
>>
>> I don't think we would gain anything from forbidding --pid-file without
>> --fork, would we?
> 
> I can't think of any reason to forbid it. So it sounds like we are
> intentional, this writes the pid into --pid-file regardless of whether
> that pid can be learned by other means as well.
> 
> 
>>>> +    const char *pid_path = NULL;
>>>
>>> Bikeshedding: pid_name is nicer (path makes me think of $PATH and other
>>> colon-separated lists, which this is not).
>>
>> I'd prefer pid_filename myself, then, because pid_name sounds like a
>> weird way to say "process name". O:-)
> 
> Works for me, even if it is longer. Do you want to respin, or just have
> me touch it up when folding it into my NBD tree?

I suppose I’d prefer a respin, independently of what you make of patches
4 and 5.

Max


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

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

[Eric Blake]

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

On 5/7/19 1:36 PM, Max Reitz wrote:
> 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 racey because

racy

> 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>
> ---
>  tests/qemu-iotests/233        |  1 -
>  tests/qemu-iotests/common.nbd | 93 ++++++++++++++++-------------------
>  2 files changed, 42 insertions(+), 52 deletions(-)
> 

> @@ -34,76 +39,62 @@ nbd_server_stop()
>          fi
>      fi
>      rm -f "$nbd_unix_socket"
> -}
> -
> -nbd_server_wait_for_unix_socket()
> -{
...
> -    echo "Failed in check of unix socket created by qemu-nbd"
> -    exit 1
> +    rm -f "$nbd_stderr_fifo"

You could use a single 'rm -f "$nbd_unix_socket" "$nbd_stderr_fifo"'.
That's cosmetic, though.

Are we sure that even on failure, our fifo will not fill up and cause
deadlock? If the failing qemu-nbd has so much output as to be non-atomic
so that it blocks waiting for a reader, but we don't read anything until
after qemu-nbd exits after forking the daemon, then we have deadlock.
But in the common case, I don't think qemu-nbd ever spits out that much
in errors, even when it fails to start whether due to a socket in use or
for other reasons.  And even if it does hang, it is our testsuite (and
our CI tools will probably notice it), rather than our main code.

Otherwise, it's a lot of shell code with quite a few bash-isms, but we
already require bash, and I didn't spot anything blatantly wrong.

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 5/5] iotests: Let 233 run concurrently

[Max Reitz]

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

On 07.05.19 22:38, Eric Blake wrote:
> On 5/7/19 1:36 PM, Max Reitz wrote:
>> 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 racey because
> 
> racy
> 
>> 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>
>> ---
>>  tests/qemu-iotests/233        |  1 -
>>  tests/qemu-iotests/common.nbd | 93 ++++++++++++++++-------------------
>>  2 files changed, 42 insertions(+), 52 deletions(-)
>>
> 
>> @@ -34,76 +39,62 @@ nbd_server_stop()
>>          fi
>>      fi
>>      rm -f "$nbd_unix_socket"
>> -}
>> -
>> -nbd_server_wait_for_unix_socket()
>> -{
> ...
>> -    echo "Failed in check of unix socket created by qemu-nbd"
>> -    exit 1
>> +    rm -f "$nbd_stderr_fifo"
> 
> You could use a single 'rm -f "$nbd_unix_socket" "$nbd_stderr_fifo"'.
> That's cosmetic, though.
> 
> Are we sure that even on failure, our fifo will not fill up and cause
> deadlock? If the failing qemu-nbd has so much output as to be non-atomic
> so that it blocks waiting for a reader, but we don't read anything until
> after qemu-nbd exits after forking the daemon, then we have deadlock.

Hm, right.  I don’t think it will happen, but if it does, it won’t be
because of an “Address already in use”.  So if it did happen, the test
should fail anyway.

Of course, a hang is not the nicest way to fail a test, but I think as
long as we don’t think it will be a problem, it should be fine.

(The alternative I can think of would be to start a background cat that
copies data over to a log file, and then kill it after the qemu-nbd
parent process has exited.  On error, we read the log; on success, we
print it to stderr and then start the cat from nbd_stderr_fifo to stderr.)

> But in the common case, I don't think qemu-nbd ever spits out that much
> in errors, even when it fails to start whether due to a socket in use or
> for other reasons.  And even if it does hang, it is our testsuite (and
> our CI tools will probably notice it), rather than our main code.
> 
> Otherwise, it's a lot of shell code with quite a few bash-isms, but we
> already require bash, and I didn't spot anything blatantly wrong.
> 
> Reviewed-by: Eric Blake <eblake@redhat.com>

Thanks again!

I’ll prepare the v2.

Max


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

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

[Daniel P. Berrangé]

On Tue, May 07, 2019 at 08:36:06PM +0200, 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    | 29 +++++++++++++++++++++++++++++
>  qemu-nbd.texi |  2 ++
>  2 files changed, 31 insertions(+)
> 
> diff --git a/qemu-nbd.c b/qemu-nbd.c
> index dca9e72cee..7e48154f44 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,8 @@ int main(int argc, char **argv)
>      bool list = false;
>      int old_stderr = -1;
>      unsigned socket_activation;
> +    const char *pid_path = NULL;
> +    FILE *pid_file;
>  
>      /* 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 +881,9 @@ int main(int argc, char **argv)
>          case 'L':
>              list = true;
>              break;
> +        case QEMU_NBD_OPT_PID_FILE:
> +            pid_path = optarg;
> +            break;
>          }
>      }
>  
> @@ -1196,6 +1204,27 @@ int main(int argc, char **argv)
>  
>      nbd_update_server_watch();
>  
> +    if (pid_path) {
> +        pid_file = fopen(pid_path, "w");
> +        if (!pid_file) {
> +            error_report("Failed to store PID in %s: %s",
> +                         pid_path, strerror(errno));
> +            exit(EXIT_FAILURE);
> +        }
> +
> +        ret = fprintf(pid_file, "%ld", (long)getpid());
> +        if (ret < 0) {
> +            ret = -errno;
> +        }
> +        fclose(pid_file);
> +
> +        if (ret < 0) {
> +            error_report("Failed to store PID in %s: %s",
> +                         pid_path, strerror(-ret));
> +            exit(EXIT_FAILURE);
> +        }
> +    }

This is racy because multiple qemu-nbd's can be started pointing to
the same pidfile and one will blindly overwrite the other.

Please use  qemu_write_pidfile instead which acquires locks on the
pidfile in a race free manner.


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

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

[Daniel P. Berrangé]

On Tue, May 07, 2019 at 09:39:01PM +0200, Max Reitz wrote:
> On 07.05.19 21:30, Eric Blake wrote:
> > On 5/7/19 1:36 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    | 29 +++++++++++++++++++++++++++++
> >>  qemu-nbd.texi |  2 ++
> >>  2 files changed, 31 insertions(+)
> >>
> > 
> >> @@ -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"
> > 
> > Should --pid-file imply --fork, or be an error if --fork was not
> > supplied? As coded, it writes a pid file regardless of --fork, even
> > though it is less obvious that it is useful in that case. I don't have a
> > strong preference (there doesn't seem to be a useful consensus on what
> > forking daemons should do), but it would at least be worth documenting
> > the intended action (even if that implies a tweak to the patch to match
> > the intent).
> 
> I think the documentation is pretty clear.  It stores the server's PID,
> whether it has been forked or not.
> 
> I don't think we would gain anything from forbidding --pid-file without
> --fork, would we?

Indeed, use of --pid-file should be independant of --fork, as a mgmt app
may have already forked it into the background, and merely want to get
the pidfile

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

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

[Max Reitz]

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

On 08.05.19 11:01, Daniel P. Berrangé wrote:
> On Tue, May 07, 2019 at 08:36:06PM +0200, 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    | 29 +++++++++++++++++++++++++++++
>>  qemu-nbd.texi |  2 ++
>>  2 files changed, 31 insertions(+)
>>
>> diff --git a/qemu-nbd.c b/qemu-nbd.c
>> index dca9e72cee..7e48154f44 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,8 @@ int main(int argc, char **argv)
>>      bool list = false;
>>      int old_stderr = -1;
>>      unsigned socket_activation;
>> +    const char *pid_path = NULL;
>> +    FILE *pid_file;
>>  
>>      /* 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 +881,9 @@ int main(int argc, char **argv)
>>          case 'L':
>>              list = true;
>>              break;
>> +        case QEMU_NBD_OPT_PID_FILE:
>> +            pid_path = optarg;
>> +            break;
>>          }
>>      }
>>  
>> @@ -1196,6 +1204,27 @@ int main(int argc, char **argv)
>>  
>>      nbd_update_server_watch();
>>  
>> +    if (pid_path) {
>> +        pid_file = fopen(pid_path, "w");
>> +        if (!pid_file) {
>> +            error_report("Failed to store PID in %s: %s",
>> +                         pid_path, strerror(errno));
>> +            exit(EXIT_FAILURE);
>> +        }
>> +
>> +        ret = fprintf(pid_file, "%ld", (long)getpid());
>> +        if (ret < 0) {
>> +            ret = -errno;
>> +        }
>> +        fclose(pid_file);
>> +
>> +        if (ret < 0) {
>> +            error_report("Failed to store PID in %s: %s",
>> +                         pid_path, strerror(-ret));
>> +            exit(EXIT_FAILURE);
>> +        }
>> +    }
> 
> This is racy because multiple qemu-nbd's can be started pointing to
> the same pidfile and one will blindly overwrite the other.
> 
> Please use  qemu_write_pidfile instead which acquires locks on the
> pidfile in a race free manner.

Ah, nice, that makes things better and easier. :-)

Max


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