[v3,1/3] qemu-nbd: Add --fork option
diff mbox

Message ID 1472142645-7370-2-git-send-email-mreitz@redhat.com
State New
Headers show

Commit Message

Max Reitz Aug. 25, 2016, 4:30 p.m. UTC
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(-)

Comments

Sascha Silbe Aug. 29, 2016, 4:59 p.m. UTC | #1
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
Max Reitz Sept. 23, 2016, 3:16 p.m. UTC | #2
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
Kevin Wolf Sept. 27, 2016, 9:04 a.m. UTC | #3
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
Max Reitz Sept. 27, 2016, 7:41 p.m. UTC | #4
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

Patch
diff mbox

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);