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

[Hanna Reitz]

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

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

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>