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

[Kevin Wolf]

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

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

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

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.

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
> 
> > > Instead of adding custom new code to do so, just reuse process_options()
> > > and give it a @pre_init_pass argument to distinguish the two passes.  I
> > > believe there are some other switches but --daemonize that deserve
> > > parsing in the first pass:
> > > 
> > > - --help and --version are supposed to only print some text and then
> > >    immediately exit (so any initialization we do would be for naught).
> > >    This changes behavior, because now "--blockdev inv-drv --help" will
> > >    print a help text instead of complaining about the --blockdev
> > >    argument.
> > >    Note that this is similar in behavior to other tools, though: "--help"
> > >    is generally immediately acted upon when finding it in the argument
> > >    list, potentially before other arguments (even ones before it) are
> > >    acted on.  For example, "ls /does-not-exist --help" prints a help text
> > >    and does not complain about ENOENT.
> > > 
> > > - --pidfile does not need initialization, and is already exempted from
> > >    the sequential order that process_options() claims to strictly follow
> > >    (the PID file is only created after all arguments are processed, not
> > >    at the time the --pidfile argument appears), so it makes sense to
> > >    include it in the same category as --daemonize.
> > > 
> > > - Invalid arguments should always be reported as soon as possible.  (The
> > >    same caveat with --help applies: That means that "--blockdev inv-drv
> > >    --inv-arg" will now complain about --inv-arg, not inv-drv.)
> > > 
> > > Note that we could decide to check only for --daemonize in the first
> > > pass, and defer --help, --version, and checking for invalid arguments to
> > > the second one, thus largely keeping our current behavior.  However,
> > > this would break "--help --daemonize": The child would print the help
> > > text to stdout, which is redirected to /dev/null, and so the text would
> > > disappear.  We would need to have the text be printed to stderr instead,
> > > and this would then make the parent process exit with EXIT_FAILURE,
> > > which is probably not what we want for --help.
> > > 
> > > This patch does make some references to --daemonize without having
> > > implemented it yet, but that will happen in the next patch.
> > > 
> > > Signed-off-by: Hanna Reitz <hreitz@redhat.com>
>