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

[Paolo Bonzini]

On 01/16/2012 08:22 AM, Michael Tokarev wrote:
> 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?

Ah, sorry, I don't connect to the VPN usually during the weekend so I 
thought it was sent privately.

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

It's a chicken-and-egg problem.  To connect a socket to an NBD device, 
you need to have negotiated with the server already, so the socket must 
be connected already.  Since the other side of the socket is also 
handled by qemu-nbd, you need threads.

It's true that in principle you could use the main loop and do the 
NBD_DO_IT in a new thread.  However, qemu-sockets.c does not support 
asynchronous connect(2).  You could run that code already inside the 
main loop, for example in a coroutine, but then patches pile up quickly.

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

I'm not saying it's particularly pretty, but it's also not as easy to 
improve as you'd think.  There are a lot of constraints (fork/daemonize 
before initializing the block layer, report errors properly, avoid 
races, ...).

Paolo