qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Max Reitz
Subject: Re: [Qemu-devel] [PATCH v3 1/3] qemu-nbd: Add --fork option
Date: Fri, 23 Sep 2016 17:16:49 +0200
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.3.0

On 29.08.2016 18:59, Sascha Silbe wrote:
> Dear Max,
> 
> 
> thanks for taking the time to fix the race condition!
> 
> 
> Max Reitz <address@hidden> 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: signature.asc
Description: OpenPGP digital signature


reply via email to

[Prev in Thread] Current Thread [Next in Thread]