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

[Michael Tokarev <mjt@tls.msk.ru>]

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.

>> -
>> -            /* ... 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.

>> +    /* 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 the place 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.

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

Thanks,

/mjt