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

[Michael Tokarev]

On 15.01.2012 21:31, Paolo Bonzini wrote:
> On 01/15/2012 05:44 PM, Michael Tokarev wrote:
>>>>>> +             * 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.
>>
>> Done in a v2 version I sent you.
> 
> Please stay on the list.

Sorry?  I sent it to you and to the list, here's the command
line from my .bash_history:

 git format-patch --subject-prefix="PATCH v2" --stdout --to 'address@hidden' 
--cc "Paolo Bonzini <address@hidden>" --cc address@hidden HEAD^ | 
/usr/sbin/sendmail -t -i

On which list I shoult stay?

[]
>> Um, I missed that "half of this" part.  Indeed, nbd_client_thread()
>> does dup2(STDOUT_FILENO, STDERR_FILENO) which should go away, but
>> it is harmless for now, and can be addressed in a separate patch.
> 
> Again, _the client thread_ is the right place to do this!  See below.

[]
>> We're doomed anyway, and it is even good
>> we've a small remote chance for our error message to
>> be seen.  Currently it just goes to /dev/null.
> 
> No, currently it is sent from the daemon to the parent through the pipe, the 
> parent prints it and exits with status code 1.  With your patch, if the dup2 
> wins the race you exit with status code 0; if the client thread wins the race 
> it is the same as master.

Aha. I finally see what you mean.

I still disagree, -- all the operations done in the client
thread can be done before forking a new thread, syncronously,
and _that_ will be the easiest and cleanest solution here.

>> That's not a bad intention.  I'm fixing existing logic without
>> introducing new logical changes.  If you want to fix other
>> stuff, it is better be done in a separate commit/change.
> 
> AFAIK the only known bug (besides the devfd/sockfd mixup) is the missing 
> chdir, and that should be fixed first.

It all looked so ugly to me that I didn't even want to think
about just adding a chdir() instead of getting rid of daemon().
But ok, I can go that ugly route too.

Thanks,

/mjt