Re: [PATCH 1/3] qsd: Add pre-init argument parsing pass

[Markus Armbruster]

Kevin Wolf <kwolf@redhat.com> writes:

> Am 19.01.2022 um 14:44 hat Hanna Reitz geschrieben:
>> On 19.01.22 13:58, Markus Armbruster wrote:
>> > Hanna Reitz <hreitz@redhat.com> writes:
>> > 
>> > > We want to add a --daemonize argument to QSD's command line.
>> > 
>> > Why?
>> 
>> OK, s/we/I/.  I find it useful, because without such an option, I need to
>> have whoever invokes QSD loop until the PID file exists, before I can be
>> sure that all exports are set up.  I make use of it in the test cases added
>> in patch 3.
>> 
>> I suppose this could be worked around with a special character device, like
>> so:
>> 
>> ```
>> ncat --listen -U /tmp/qsd-done.sock </dev/null &
>> ncat_pid=$!
>> 
>> qemu-storage-daemon \
>>     ... \
>>     --chardev socket,id=signal_done,path=/tmp/qsd-done.sock \
>>     --monitor signal_done \
>>     --pidfile /tmp/qsd.pid &
>> 
>> wait $ncat_pid
>> ```
>> 
>> But having to use an extra tool for this is unergonomic.  I mean, if there’s
>> no other way...

I know duplicating this into every program that could server as a daemon
is the Unix tradition.  Doesn't make it good.  Systemd[*] has tried to
make it superfluous.

> The other point is that the system emulator has it, qemu-nbd has it,
> so certainly qsd should have it as well. Not the least because it should
> be able to replace qemu-nbd (at least for the purpose of exporting NBD.
> not necessarily for attaching it to the host).

Point taken, but I think it's a somewhat weak one.  qsd could certainly
replace qemu-nbd even without --daemonize; we could use other means to
run it in the background.

>> > >                                                                This will
>> > > require forking the process before we do any complex initialization
>> > > steps, like setting up the block layer or QMP.  Therefore, we must scan
>> > > the command line for it long before our current process_options() call.
>> > 
>> > Can you explain in a bit more detail why early forking is required?
>> > 
>> > I have a strong dislike for parsing more than once...
>> 
>> Because I don’t want to set up QMP and block devices, and then fork the
>> process into two.  That sounds like there’d be a lot of stuff to think
>> about, which just isn’t necessary, because we don’t need to set up any
>> of this in the parent.

We must fork() before we create threads.  Other resources are easy
enough to hand over to the child.  Still, having to think about less is
good, I readily grant you that.

The trouble is that forking early creates a new problem: any
configuration errors detected in the child must be propagated to the
parent somehow (output and exit status).  I peeked at your PATCH 2, and
I'm not convinced, but that's detail here.

> Here we can compare again: Both the system emulator and qemu-nbd behave
> the same, they fork before they do anything interesting.
>
> The difference is that they still parse the command line only once
> because they don't immediately create things, but just store the options
> and later process them in their own magic order. I'd much rather parse
> the command line twice than copy that behaviour.

The part I hate is "own magic order".  Without that, multiple passes are
just fine with me.

Parsing twice is a bit like having a two pass compiler run the first
pass left to right, and then both passes intertwined left to right.  The
pedestrian way to do it is running the first pass left to right, then
the second pass left to right.

We're clearly talking taste here.

>
> Kevin
>
>> For example, if I set up a monitor on a Unix socket (server=true),
>> processing is delayed until the client connects.  Say I put --daemonize
>> afterwards.  I connect to the waiting server socket, the child is forked
>> off, and then... I’m not sure what happens, actually.  Do I have a
>> connection with both the parent and the child listening?  I know that in
>> practice, what happens is that once the parent exits, the connection is
>> closed, and I get a “qemu: qemu_thread_join: Invalid argument” warning/error
>> on the QSD side.
>> 
>> There’s a lot of stuff to think about if you allow forking after other
>> options, so it should be done first.  We could just require the user to put
>> --daemonize before all other options, and so have a single pass; but still,
>> before options are even parsed, we have already for example called
>> bdrv_init(), init_qmp_commands(), qemu_init_main_loop().  These are all
>> things that the parent of a daemonizing process doesn’t need to do, and
>> where I’d simply rather not think about what impact it has if we fork
>> afterwards.
>> 
>> Hanna

Care to put a brief version of the rationale for --daemonize and for
forking early in the commit message?

[...]


[*] Not everything systemd does is bad.  It's a big, mixed bag of ideas.