| Message ID | 20220303164814.284974-3-hreitz@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | qsd: Add --daemonize; and add job quit tests | expand |
On Thu, Mar 03, 2022 at 05:48:12PM +0100, Hanna Reitz wrote: > In contrast to qemu-nbd (where it is called --fork) and the system > emulator, QSD does not have a --daemonize switch yet. Just like them, > QSD allows setting up block devices and exports on the command line. > When doing so, it is often necessary for whoever invoked the QSD to wait > until these exports are fully set up. A --daemonize switch allows > precisely this, by virtue of the parent process exiting once everything > is set up. > > Note that there are alternative ways of waiting for all exports to be > set up, for example: > - Passing the --pidfile option and waiting until the respective file > exists (but I do not know if there is a way of implementing this > without a busy wait loop) Non-portably, you could use inotify or similar, to get a true event-driven wakeup when the file is created. And here's the python glue that libnbd uses, instead of --pidfile: https://gitlab.com/nbdkit/libnbd/-/blob/master/interop/interop-qemu-storage-daemon.sh#L58 > - Set up some network server (e.g. on a Unix socket) and have the QSD > connect to it after all arguments have been processed by appending > corresponding --chardev and --monitor options to the command line, > and then wait until the QSD connects > > Having a --daemonize option would make this simpler, though, without > having to rely on additional tools (to set up a network server) or busy > waiting. > > Implementing a --daemonize switch means having to fork the QSD process. > Ideally, we should do this as early as possible: All the parent process > has to do is to wait for the child process to signal completion of its > set-up phase, and therefore there is basically no initialization that > needs to be done before the fork. On the other hand, forking after > initialization steps means having to consider how those steps (like > setting up the block layer or QMP) interact with a later fork, which is > often not trivial. > > In order to fork this early, we must scan the command line for > --daemonize long before our current process_options() call. 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 s/but/beyond/ > 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. Well, GNU ls does that, but only if POSIXLY_CORRECT is not set (a strict POSIX ls must give you two 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.) > > 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> > Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> > --- > storage-daemon/qemu-storage-daemon.c | 43 ++++++++++++++++++++++++---- > 1 file changed, 38 insertions(+), 5 deletions(-) > Reviewed-by: Eric Blake <eblake@redhat.com>
diff --git a/storage-daemon/qemu-storage-daemon.c b/storage-daemon/qemu-storage-daemon.c index 504d33aa91..b798954edb 100644 --- a/storage-daemon/qemu-storage-daemon.c +++ b/storage-daemon/qemu-storage-daemon.c @@ -177,7 +177,23 @@ static int getopt_set_loc(int argc, char **argv, const char *optstring, return c; } -static void process_options(int argc, char *argv[]) +/** + * Process QSD command-line arguments. + * + * This is done in two passes: + * + * First (@pre_init_pass is true), we do a pass where all global + * arguments pertaining to the QSD process (like --help or --daemonize) + * are processed. This pass is done before most of the QEMU-specific + * initialization steps (e.g. initializing the block layer or QMP), and + * so must only process arguments that are not really QEMU-specific. + * + * Second (@pre_init_pass is false), we (sequentially) process all + * QEMU/QSD-specific arguments. Many of these arguments are effectively + * translated to QMP commands (like --blockdev for blockdev-add, or + * --export for block-export-add). + */ +static void process_options(int argc, char *argv[], bool pre_init_pass) { int c; @@ -196,11 +212,26 @@ static void process_options(int argc, char *argv[]) }; /* - * In contrast to the system emulator, options are processed in the order - * they are given on the command lines. This means that things must be - * defined first before they can be referenced in another option. + * In contrast to the system emulator, QEMU-specific options are processed + * in the order they are given on the command lines. This means that things + * must be defined first before they can be referenced in another option. */ + optind = 1; while ((c = getopt_set_loc(argc, argv, "-hT:V", long_options)) != -1) { + bool handle_option_pre_init; + + /* Should this argument be processed in the pre-init pass? */ + handle_option_pre_init = + c == '?' || + c == 'h' || + c == 'V' || + c == OPTION_PIDFILE; + + /* Process every option only in its respective pass */ + if (pre_init_pass != handle_option_pre_init) { + continue; + } + switch (c) { case '?': exit(EXIT_FAILURE); @@ -334,6 +365,8 @@ int main(int argc, char *argv[]) qemu_init_exec_dir(argv[0]); os_setup_signal_handling(); + process_options(argc, argv, true); + module_call_init(MODULE_INIT_QOM); module_call_init(MODULE_INIT_TRACE); qemu_add_opts(&qemu_trace_opts); @@ -348,7 +381,7 @@ int main(int argc, char *argv[]) qemu_set_log(LOG_TRACE); qemu_init_main_loop(&error_fatal); - process_options(argc, argv); + process_options(argc, argv, false); /* * Write the pid file after creating chardevs, exports, and NBD servers but