[Qemu-devel] [PATCH v3 0/3] iotests: Fix test 162

[Qemu-devel] [PATCH v3 0/3] iotests: Fix test 162

[Max Reitz]

162 is potentially racy and makes some invalid assumptions about what
should happen when connecting to a non-existing domain name. This series
fixes both issues.


v3: [Sascha]
- Added patches 1 and 2
- Dropped the raciness from patch 3


Max Reitz (3):
  qemu-nbd: Add --fork option
  iotests: Remove raciness from 162
  iotests: Do not rely on unavailable domains in 162

 qemu-nbd.c                 | 15 ++++++++++++++-
 tests/qemu-iotests/162     | 22 ++++++++++++++++------
 tests/qemu-iotests/162.out |  2 +-
 3 files changed, 31 insertions(+), 8 deletions(-)

-- 
2.5.5

[Qemu-devel] [PATCH v3 1/3] qemu-nbd: Add --fork option

[Max Reitz]

Using the --fork option, one can make qemu-nbd fork the worker process.
The original process will exit on error of the worker or once the worker
enters the main loop.

Suggested-by: Sascha Silbe <silbe@linux.vnet.ibm.com>
Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 qemu-nbd.c | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

diff --git a/qemu-nbd.c b/qemu-nbd.c
index e3571c2..8c2d582 100644
--- a/qemu-nbd.c
+++ b/qemu-nbd.c
@@ -48,6 +48,7 @@
 #define QEMU_NBD_OPT_OBJECT        260
 #define QEMU_NBD_OPT_TLSCREDS      261
 #define QEMU_NBD_OPT_IMAGE_OPTS    262
+#define QEMU_NBD_OPT_FORK          263
 
 #define MBR_SIZE 512
 
@@ -503,6 +504,7 @@ int main(int argc, char **argv)
         { "tls-creds", required_argument, NULL, QEMU_NBD_OPT_TLSCREDS },
         { "image-opts", no_argument, NULL, QEMU_NBD_OPT_IMAGE_OPTS },
         { "trace", required_argument, NULL, 'T' },
+        { "fork", no_argument, NULL, QEMU_NBD_OPT_FORK },
         { NULL, 0, NULL, 0 }
     };
     int ch;
@@ -524,6 +526,8 @@ int main(int argc, char **argv)
     bool imageOpts = false;
     bool writethrough = true;
     char *trace_file = NULL;
+    bool fork_process = false;
+    int old_stderr = -1;
 
     /* The client thread uses SIGTERM to interrupt the server.  A signal
      * handler ensures that "qemu-nbd -v -c" exits with a nice status code.
@@ -714,6 +718,9 @@ int main(int argc, char **argv)
             g_free(trace_file);
             trace_file = trace_opt_parse(optarg);
             break;
+        case QEMU_NBD_OPT_FORK:
+            fork_process = true;
+            break;
         }
     }
 
@@ -773,7 +780,7 @@ int main(int argc, char **argv)
         return 0;
     }
 
-    if (device && !verbose) {
+    if ((device && !verbose) || fork_process) {
         int stderr_fd[2];
         pid_t pid;
         int ret;
@@ -796,6 +803,7 @@ int main(int argc, char **argv)
             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));
@@ -951,6 +959,11 @@ int main(int argc, char **argv)
         exit(EXIT_FAILURE);
     }
 
+    if (fork_process) {
+        dup2(old_stderr, STDERR_FILENO);
+        close(old_stderr);
+    }
+
     state = RUNNING;
     do {
         main_loop_wait(false);
-- 
2.5.5

Re: [Qemu-devel] [PATCH v3 1/3] qemu-nbd: Add --fork option

[Sascha Silbe]

Dear Max,


thanks for taking the time to fix the race condition!


Max Reitz <mreitz@redhat.com> writes:

> Using the --fork option, one can make qemu-nbd fork the worker process.
> The original process will exit on error of the worker or once the worker
> enters the main loop.

> @@ -773,7 +780,7 @@ int main(int argc, char **argv)
>          return 0;
>      }
>
> -    if (device && !verbose) {
> +    if ((device && !verbose) || fork_process) {
>          int stderr_fd[2];
>          pid_t pid;
>          int ret;

Looking at the surrounding (unchanged) code I see that qemu-nbd already
implemented a daemon mode. It's just that it's completely undocumented
and hinges on both the --device and the --verbose option. Yuck.

It seems there are two things --verbose does (from a user point of
view):

1. Print "NBD device %s is now connected to %s" and keep stderr open.

   Debug messages are always printed to stderr, but in non-verbose
   daemon mode they end up at /dev/null.

   This is more or less what one usually expects from an option named
   --verbose. Except that it only affects daemon mode and messages are
   always printed (but end up at /dev/null).

2. Disable daemon mode.

   I might expect this for an option named --debug, but certainly not
   for --verbose...


A clean way forward would be something like this:

1. Introduce --foreground / --daemon, --quiet

   Default to daemon mode with silent output if --connect is given,
   foreground mode with visible output otherwise. Set non-daemon mode
   with visible output if --verbose is given. Let --foreground /
   --daemon / --quiet any default or implicit value. Document that
   --verbose implicitly enables daemon mode for compatibility with
   previous versions and that future versions may stop doing so
   (i.e. users should use either --verbose --foreground or --verbose
   --daemon).

3. At some point in the future (qemu 3.0?) we can stop having --verbose
   imply --foreground.


I can give it a try if it's out of scope for your current task.


Sascha
-- 
Softwareentwicklung Sascha Silbe, Niederhofenstraße 5/1, 71229 Leonberg
https://se-silbe.de/
USt-IdNr. DE281696641

Re: [Qemu-devel] [PATCH v3 1/3] qemu-nbd: Add --fork option

[Max Reitz]

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

On 29.08.2016 18:59, Sascha Silbe wrote:
> Dear Max,
> 
> 
> thanks for taking the time to fix the race condition!
> 
> 
> Max Reitz <mreitz@redhat.com> writes:
> 
>> Using the --fork option, one can make qemu-nbd fork the worker process.
>> The original process will exit on error of the worker or once the worker
>> enters the main loop.
> 
>> @@ -773,7 +780,7 @@ int main(int argc, char **argv)
>>          return 0;
>>      }
>>
>> -    if (device && !verbose) {
>> +    if ((device && !verbose) || fork_process) {
>>          int stderr_fd[2];
>>          pid_t pid;
>>          int ret;
> 
> Looking at the surrounding (unchanged) code I see that qemu-nbd already
> implemented a daemon mode. It's just that it's completely undocumented
> and hinges on both the --device and the --verbose option. Yuck.
> 
> It seems there are two things --verbose does (from a user point of
> view):
> 
> 1. Print "NBD device %s is now connected to %s" and keep stderr open.
> 
>    Debug messages are always printed to stderr, but in non-verbose
>    daemon mode they end up at /dev/null.
> 
>    This is more or less what one usually expects from an option named
>    --verbose. Except that it only affects daemon mode and messages are
>    always printed (but end up at /dev/null).
> 
> 2. Disable daemon mode.
> 
>    I might expect this for an option named --debug, but certainly not
>    for --verbose...

Does it? There is explicit daemon mode until this patch. While it is
actually implemented, it is only used implicitly when you want to
connect to the kernel's NBD interface (which is what the --connect
option is for (whose argument is kept in the @device variable)).

This patch introduces a way to generally make use of the "daemon" mode;
and when you use the respective option (--fork), then it will work
regardless of whether you have specified --verbose or not.

The only thing --verbose does is disable the implicit daemon mode when
using --connect, and that seems reasonable to me.

> A clean way forward would be something like this:
> 
> 1. Introduce --foreground / --daemon, --quiet
> 
>    Default to daemon mode with silent output if --connect is given,
>    foreground mode with visible output otherwise. Set non-daemon mode
>    with visible output if --verbose is given. Let --foreground /
>    --daemon / --quiet any default or implicit value. Document that
>    --verbose implicitly enables daemon mode for compatibility with
>    previous versions and that future versions may stop doing so
>    (i.e. users should use either --verbose --foreground or --verbose
>    --daemon).

Note that we haven't even documented that --connect implicitly puts
qemu-nbd in a daemon mode.

> 3. At some point in the future (qemu 3.0?) we can stop having --verbose
>    imply --foreground.

While in my opinion --verbose actually is a debugging option, I don't
think the behavior of --connect when used together with --verbose really
affects this patch series.

> I can give it a try if it's out of scope for your current task.

I certainly won't stop you. ;-)

Max


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

Re: [Qemu-devel] [PATCH v3 1/3] qemu-nbd: Add --fork option

[Kevin Wolf]

Am 25.08.2016 um 18:30 hat Max Reitz geschrieben:
> Using the --fork option, one can make qemu-nbd fork the worker process.
> The original process will exit on error of the worker or once the worker
> enters the main loop.
> 
> Suggested-by: Sascha Silbe <silbe@linux.vnet.ibm.com>
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  qemu-nbd.c | 15 ++++++++++++++-
>  1 file changed, 14 insertions(+), 1 deletion(-)
> 
> diff --git a/qemu-nbd.c b/qemu-nbd.c
> index e3571c2..8c2d582 100644
> --- a/qemu-nbd.c
> +++ b/qemu-nbd.c
> @@ -48,6 +48,7 @@
>  #define QEMU_NBD_OPT_OBJECT        260
>  #define QEMU_NBD_OPT_TLSCREDS      261
>  #define QEMU_NBD_OPT_IMAGE_OPTS    262
> +#define QEMU_NBD_OPT_FORK          263
>  
>  #define MBR_SIZE 512
>  
> @@ -503,6 +504,7 @@ int main(int argc, char **argv)
>          { "tls-creds", required_argument, NULL, QEMU_NBD_OPT_TLSCREDS },
>          { "image-opts", no_argument, NULL, QEMU_NBD_OPT_IMAGE_OPTS },
>          { "trace", required_argument, NULL, 'T' },
> +        { "fork", no_argument, NULL, QEMU_NBD_OPT_FORK },
>          { NULL, 0, NULL, 0 }
>      };
>      int ch;
> @@ -524,6 +526,8 @@ int main(int argc, char **argv)
>      bool imageOpts = false;
>      bool writethrough = true;
>      char *trace_file = NULL;
> +    bool fork_process = false;
> +    int old_stderr = -1;
>  
>      /* The client thread uses SIGTERM to interrupt the server.  A signal
>       * handler ensures that "qemu-nbd -v -c" exits with a nice status code.
> @@ -714,6 +718,9 @@ int main(int argc, char **argv)
>              g_free(trace_file);
>              trace_file = trace_opt_parse(optarg);
>              break;
> +        case QEMU_NBD_OPT_FORK:
> +            fork_process = true;
> +            break;
>          }
>      }
>  
> @@ -773,7 +780,7 @@ int main(int argc, char **argv)
>          return 0;
>      }
>  
> -    if (device && !verbose) {
> +    if ((device && !verbose) || fork_process) {
>          int stderr_fd[2];
>          pid_t pid;
>          int ret;
> @@ -796,6 +803,7 @@ int main(int argc, char **argv)
>              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));

I don't have a kernel with NBD support, so I can't test this easily, but
doesn't this break the daemon mode for --connect? I mean, it will still
fork, but won't the parent be alive until the child exits?

> @@ -951,6 +959,11 @@ int main(int argc, char **argv)
>          exit(EXIT_FAILURE);
>      }
>  
> +    if (fork_process) {
> +        dup2(old_stderr, STDERR_FILENO);
> +        close(old_stderr);
> +    }

Because this code doesn't run for --connect (unless --fork is given,
too).

>      state = RUNNING;
>      do {
>          main_loop_wait(false);

The documentation needs an update, too.

Kevin

Re: [Qemu-devel] [PATCH v3 1/3] qemu-nbd: Add --fork option

[Max Reitz]

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

On 27.09.2016 11:04, Kevin Wolf wrote:
> Am 25.08.2016 um 18:30 hat Max Reitz geschrieben:
>> Using the --fork option, one can make qemu-nbd fork the worker process.
>> The original process will exit on error of the worker or once the worker
>> enters the main loop.
>>
>> Suggested-by: Sascha Silbe <silbe@linux.vnet.ibm.com>
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>> ---
>>  qemu-nbd.c | 15 ++++++++++++++-
>>  1 file changed, 14 insertions(+), 1 deletion(-)
>>
>> diff --git a/qemu-nbd.c b/qemu-nbd.c
>> index e3571c2..8c2d582 100644
>> --- a/qemu-nbd.c
>> +++ b/qemu-nbd.c
>> @@ -48,6 +48,7 @@
>>  #define QEMU_NBD_OPT_OBJECT        260
>>  #define QEMU_NBD_OPT_TLSCREDS      261
>>  #define QEMU_NBD_OPT_IMAGE_OPTS    262
>> +#define QEMU_NBD_OPT_FORK          263
>>  
>>  #define MBR_SIZE 512
>>  
>> @@ -503,6 +504,7 @@ int main(int argc, char **argv)
>>          { "tls-creds", required_argument, NULL, QEMU_NBD_OPT_TLSCREDS },
>>          { "image-opts", no_argument, NULL, QEMU_NBD_OPT_IMAGE_OPTS },
>>          { "trace", required_argument, NULL, 'T' },
>> +        { "fork", no_argument, NULL, QEMU_NBD_OPT_FORK },
>>          { NULL, 0, NULL, 0 }
>>      };
>>      int ch;
>> @@ -524,6 +526,8 @@ int main(int argc, char **argv)
>>      bool imageOpts = false;
>>      bool writethrough = true;
>>      char *trace_file = NULL;
>> +    bool fork_process = false;
>> +    int old_stderr = -1;
>>  
>>      /* The client thread uses SIGTERM to interrupt the server.  A signal
>>       * handler ensures that "qemu-nbd -v -c" exits with a nice status code.
>> @@ -714,6 +718,9 @@ int main(int argc, char **argv)
>>              g_free(trace_file);
>>              trace_file = trace_opt_parse(optarg);
>>              break;
>> +        case QEMU_NBD_OPT_FORK:
>> +            fork_process = true;
>> +            break;
>>          }
>>      }
>>  
>> @@ -773,7 +780,7 @@ int main(int argc, char **argv)
>>          return 0;
>>      }
>>  
>> -    if (device && !verbose) {
>> +    if ((device && !verbose) || fork_process) {
>>          int stderr_fd[2];
>>          pid_t pid;
>>          int ret;
>> @@ -796,6 +803,7 @@ int main(int argc, char **argv)
>>              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));
> 
> I don't have a kernel with NBD support, so I can't test this easily, but
> doesn't this break the daemon mode for --connect? I mean, it will still
> fork, but won't the parent be alive until the child exits?

Well, for me the parent closes as it should.

> 
>> @@ -951,6 +959,11 @@ int main(int argc, char **argv)
>>          exit(EXIT_FAILURE);
>>      }
>>  
>> +    if (fork_process) {
>> +        dup2(old_stderr, STDERR_FILENO);
>> +        close(old_stderr);
>> +    }
> 
> Because this code doesn't run for --connect (unless --fork is given,
> too).

Hm, so? It never ran before either, because I'm only just now
introducing it. And the fact that I'm keeping the original stderr FD
open has nothing to do with when the parent process will quit because
the parent process is not connected to that *original* stderr.

Also, when using --connect, the FD is closed in nbd_client_thread().

> 
>>      state = RUNNING;
>>      do {
>>          main_loop_wait(false);
> 
> The documentation needs an update, too.

Right. I wonder why I forgot this. I guess the answer is "Because I
wrote this in some spare time at KVM Forum to see if it would work at
all"...

Max


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

[Qemu-devel] [PATCH v3 2/3] iotests: Remove raciness from 162

[Max Reitz]

With qemu-nbd's new --fork option, we no longer need to launch it the
hacky way.

Suggested-by: Sascha Silbe <silbe@linux.vnet.ibm.com>
Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 tests/qemu-iotests/162 | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/tests/qemu-iotests/162 b/tests/qemu-iotests/162
index 0b43ea3..c7e6593 100755
--- a/tests/qemu-iotests/162
+++ b/tests/qemu-iotests/162
@@ -51,8 +51,7 @@ $QEMU_IMG info 'json:{"driver": "nbd", "host": "does.not.exist.example.com", "po
 # This is a test for NBD's bdrv_refresh_filename() implementation: It expects
 # either host or path to be set, but it must not assume that they are set to
 # strings in the options QDict
-$QEMU_NBD -k "$PWD/42" -f raw null-co:// &
-sleep 0.5
+$QEMU_NBD -k "$PWD/42" -f raw --fork null-co://
 $QEMU_IMG info 'json:{"driver": "nbd", "path": 42}' | grep '^image'
 rm -f 42
 
-- 
2.5.5

[Qemu-devel] [PATCH v3 3/3] iotests: Do not rely on unavailable domains in 162

[Max Reitz]

There are some (mostly ISP-specific) name servers who will redirect
non-existing domains to special hosts. In this case, we will get a
different error message when trying to connect to such a host, which
breaks test 162.

162 needed this specific error message so it can confirm that qemu was
indeed trying to connect to the user-specified port. However, we can
also confirm this by setting up a local NBD server on exactly that port;
so we can fix the issue by doing just that.

Reported-by: Stefan Hajnoczi <stefanha@redhat.com>
Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 tests/qemu-iotests/162     | 19 +++++++++++++++----
 tests/qemu-iotests/162.out |  2 +-
 2 files changed, 16 insertions(+), 5 deletions(-)

diff --git a/tests/qemu-iotests/162 b/tests/qemu-iotests/162
index c7e6593..f8eecb3 100755
--- a/tests/qemu-iotests/162
+++ b/tests/qemu-iotests/162
@@ -43,10 +43,21 @@ echo '=== NBD ==='
 $QEMU_IMG info 'json:{"driver": "nbd", "host": 42}'
 
 # And this should not treat @port as if it had not been specified
-# (We cannot use localhost with an invalid port here, but we need to use a
-#  non-existing domain, because otherwise the error message will not contain
-#  the port)
-$QEMU_IMG info 'json:{"driver": "nbd", "host": "does.not.exist.example.com", "port": 42}'
+# (We need to set up a server here, because the error message for "Connection
+#  refused" does not contain the destination port)
+
+# Launching qemu-nbd is done in a loop: We try to set up an NBD server on some
+# random port and continue until success, i.e. until we have found a port that
+# is not in use yet.
+while true; do
+    port=$((RANDOM + 32768))
+    if $QEMU_NBD -p $port -f raw --fork null-co:// 2> /dev/null; then
+        break
+    fi
+done
+
+$QEMU_IMG info "json:{'driver': 'nbd', 'host': 'localhost', 'port': $port}" \
+    | grep '^image' | sed -e "s/$port/PORT/"
 
 # This is a test for NBD's bdrv_refresh_filename() implementation: It expects
 # either host or path to be set, but it must not assume that they are set to
diff --git a/tests/qemu-iotests/162.out b/tests/qemu-iotests/162.out
index 9bba723..3c5be2c 100644
--- a/tests/qemu-iotests/162.out
+++ b/tests/qemu-iotests/162.out
@@ -2,7 +2,7 @@ QA output created by 162
 
 === NBD ===
 qemu-img: Could not open 'json:{"driver": "nbd", "host": 42}': Failed to connect socket: Invalid argument
-qemu-img: Could not open 'json:{"driver": "nbd", "host": "does.not.exist.example.com", "port": 42}': address resolution failed for does.not.exist.example.com:42: Name or service not known
+image: nbd://localhost:PORT
 image: nbd+unix://?socket=42
 
 === SSH ===
-- 
2.5.5