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

[Hanna Reitz <hreitz@redhat.com>]

On 20.01.22 17:00, Markus Armbruster wrote:
> 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.

Well.  I have absolutely nothing against systemd.  Still, I will not use 
it in an iotest, that’s for sure.

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

Well, my rationale for adding the feature doesn’t really extend beyond 
“I want it, I find it useful, and so I assume others will, too”.

I don’t really like putting “qemu-nbd has it” there, because... it was 
again me who implemented it for qemu-nbd.  Because I found it useful.  
But I can of course do that, if it counts as a reason.

I can certainly (and understand the need to, and will) elaborate on the 
“This will require forking the process before we do any complex 
initialization steps” part.

Hanna