Re: [Qemu-devel] [PATCH] rework daemonizing logic in qemu-nbd

[Paolo Bonzini <pbonzini@redhat.com>]

On 01/15/2012 01:50 PM, Michael Tokarev wrote:
> On 15.01.2012 14:42, Paolo Bonzini wrote:
>> On 01/14/2012 01:39 PM, Michael Tokarev wrote:
>>>            if (pid == 0) {
>>> -            close(stderr_fd[0]);
>>> -            ret = qemu_daemon(0, 0);
>>> -
>>> -            /* Temporarily redirect stderr to the parent's pipe...  */
>>> -            dup2(stderr_fd[1], STDERR_FILENO);
>>> -            if (ret == -1) {
>>> +            int nullfd = open("/dev/null", O_RDWR);
>>> +            if (nullfd<   0 || setsid()<   0) {
>>>                    err(EXIT_FAILURE, "Failed to daemonize");
>>>                }
>>
>> This is forking only once.
>
> Is it good or bad?  There's no need to fork twice.  Second
> fork (to the one which is already done in daemon(3)) has
> been done to work around lack of proper communication between
> parent and child in case of using plain daemon(3).  I.e., due
> to daemon(3) interface being unflexible/unsuitable for the
> current use case.

daemon(3) forks twice (so qemu-nbd is effectively forking three times, 
one of which is unnecessary).

See 
http://stackoverflow.com/questions/881388/what-is-the-reason-for-performing-a-double-fork-when-creating-a-daemon 
for why there is a fork before setsid and one after.

>>> -
>>> -            /* ... close the descriptor we inherited and go on.  */
>>> -            close(stderr_fd[1]);
>>> -        } else {
>>> -            bool errors = false;
>>> -            char *buf;
>>> -
>>> -            /* In the parent.  Print error messages from the child until
>>> -             * it closes the pipe.
>>> +            /* redirect stdin from /dev/null,
>>> +             * stdout (temporarily) to the pipe to parent,
>>
>> This is a bit of a hack.
>
> There's another way -- to keep the writing pipe end in some
> local variable and use that one instead of STDOUT_FILENO.
> I can do it that way for sure, just thought it's already
> using too much local variables.

Yes, that would be better.

>>> +    /* now complete the daemonizing procedure.
>>> +     */
>>> +    if (device&&  !verbose) {
>>> +        if (chdir("/")<  0) {
>>> +            err(EXIT_FAILURE, "unable to chdir to /");
>>> +        }
>>> +        /* this redirects stderr to /dev/null */
>>> +        dup2(STDIN_FILENO, STDERR_FILENO);
>>> +        /* this redirects stdout to /dev/null too, and closes parent pipe */
>>> +        dup2(STDIN_FILENO, STDOUT_FILENO);
>>> +    }
>>> +
>>
>> Half of this is already done in client_thread, and that would be
>> theplace where you should add dup2(0, 1).
>
> I partly disagree.
>
> I wanted to de-couple -c (device) case with daemonizing.
> client_thread only works in -c case, but daemonizing in
> that case is wrong as I already pointed out in another
> email - we should either stop daemonizing here at all
> or have a separate option for it.

We can only clean up standard file descriptors after all initialization 
tasks have been done.  nbd_client_thread could still write error 
messages.  Your patch introduces a race.

>>   Also, the chdir can be moved earlier, after bdrv_open.
>
> There's no need to, afiacs.  We complete init process and
> enter main loop.  Chdir should be done befor entering main
> loop, the rest makes no difference (as long as the files
> we open will be accessible from cwd).

Yes, but I prefer to have the chdir done unconditionally as soon as 
possible.

Paolo