On 19.01.22 13:58, Markus Armbruster wrote:
Hanna Reitz <hre...@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 <hre...@redhat.com>