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

[Max Reitz]

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 <address@hidden>
>> Signed-off-by: Max Reitz <address@hidden>
>> ---
>>  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

signature.asc
Description: OpenPGP digital signature