| Message ID | 20210827164954.13951-1-raphael.norwitz@nutanix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/2] storage-daemon: add opt to print when initialized | expand |
On Fri, Aug 27, 2021 at 04:50:35PM +0000, Raphael Norwitz wrote: > This change adds a command line option to print a line to standard out > when the storage daemon has completed initialization and is ready to > serve client connections. > > This option will be used to resolve a hang in the vhost-user-blk-test. Doesn't the existing --pidfile already serve the same job? That is, why not fix vhost-user-blk-test to take advantage of the pid-file creation rather than output to stdout as evidence of when the storage daemon is up and running? Therefore, I don't think we need this patch. > > Signed-off-by: Raphael Norwitz <raphael.norwitz@nutanix.com> > --- > storage-daemon/qemu-storage-daemon.c | 21 +++++++++++++++++++++ > 1 file changed, 21 insertions(+) Missing a corresponding change to the man page (docs/tools/qemu-storage-daemon.rst), if we decide to go with this after all.
On Fri, Aug 27, 2021 at 01:51:48PM -0500, eblake@redhat.com wrote: > On Fri, Aug 27, 2021 at 04:50:35PM +0000, Raphael Norwitz wrote: > > This change adds a command line option to print a line to standard out > > when the storage daemon has completed initialization and is ready to > > serve client connections. > > > > This option will be used to resolve a hang in the vhost-user-blk-test. > > Doesn't the existing --pidfile already serve the same job? That is, > why not fix vhost-user-blk-test to take advantage of the pid-file > creation rather than output to stdout as evidence of when the storage > daemon is up and running? > > Therefore, I don't think we need this patch. > Sure - that make sense. I didn't use the pid-file because I didn't want to risk leaving junk on the filesystem if the storage-daemon crashed. I'll send a V2 using pid-file without this change. > > > > Signed-off-by: Raphael Norwitz <raphael.norwitz@nutanix.com> > > --- > > storage-daemon/qemu-storage-daemon.c | 21 +++++++++++++++++++++ > > 1 file changed, 21 insertions(+) > > Missing a corresponding change to the man page > (docs/tools/qemu-storage-daemon.rst), if we decide to go with this > after all. > Ack > -- > Eric Blake, Principal Software Engineer > Red Hat, Inc. +1-919-301-3266 > Virtualization: qemu.org | libvirt.org >
On Mon, Aug 30, 2021 at 03:56:16PM +0000, Raphael Norwitz wrote: > On Fri, Aug 27, 2021 at 01:51:48PM -0500, eblake@redhat.com wrote: > > On Fri, Aug 27, 2021 at 04:50:35PM +0000, Raphael Norwitz wrote: > > > This change adds a command line option to print a line to standard out > > > when the storage daemon has completed initialization and is ready to > > > serve client connections. > > > > > > This option will be used to resolve a hang in the vhost-user-blk-test. > > > > Doesn't the existing --pidfile already serve the same job? That is, > > why not fix vhost-user-blk-test to take advantage of the pid-file > > creation rather than output to stdout as evidence of when the storage > > daemon is up and running? > > > > Therefore, I don't think we need this patch. > > > > Sure - that make sense. I didn't use the pid-file because I didn't want to > risk leaving junk on the filesystem if the storage-daemon crashed. Ideally, storage-daemon doesn't crash during the test. But even if it does, we should still be able to register which files will be cleaned up while exiting the test (if they exist), regardless of whether the test succeeded or failed, because we have control over the pidfile name before starting storage-daemon. Put another way, the task of cleaning up a pidfile during a test should not be a show-stopper. [Side note: A long time ago, there were patches submitted to make the iotests ./check engine run EVERY test in its own subdirectory, so that cleaning up all files created by the test was trivial: nuke the directory. It also has the benefit that for debugging a failing test, you merely pass an option to ./check that says to not nuke the directory. But it did not get applied at the time, and we have had enough changes in the meantime that reinstating such a useful patch would basically be work from scratch at this point] > > I'll send a V2 using pid-file without this change. Thanks, looking forward to it.
On Mon, Aug 30, 2021 at 11:05:35AM -0500, eblake@redhat.com wrote: > On Mon, Aug 30, 2021 at 03:56:16PM +0000, Raphael Norwitz wrote: > > On Fri, Aug 27, 2021 at 01:51:48PM -0500, eblake@redhat.com wrote: > > > On Fri, Aug 27, 2021 at 04:50:35PM +0000, Raphael Norwitz wrote: > > > > This change adds a command line option to print a line to standard out > > > > when the storage daemon has completed initialization and is ready to > > > > serve client connections. > > > > > > > > This option will be used to resolve a hang in the vhost-user-blk-test. > > > > > > Doesn't the existing --pidfile already serve the same job? That is, > > > why not fix vhost-user-blk-test to take advantage of the pid-file > > > creation rather than output to stdout as evidence of when the storage > > > daemon is up and running? > > > > > > Therefore, I don't think we need this patch. > > > > > > > Sure - that make sense. I didn't use the pid-file because I didn't want to > > risk leaving junk on the filesystem if the storage-daemon crashed. > > Ideally, storage-daemon doesn't crash during the test. But even if it > does, we should still be able to register which files will be cleaned > up while exiting the test (if they exist), regardless of whether the > test succeeded or failed, because we have control over the pidfile > name before starting storage-daemon. Put another way, the task of > cleaning up a pidfile during a test should not be a show-stopper. > > [Side note: A long time ago, there were patches submitted to make the > iotests ./check engine run EVERY test in its own subdirectory, so that > cleaning up all files created by the test was trivial: nuke the > directory. It also has the benefit that for debugging a failing test, > you merely pass an option to ./check that says to not nuke the > directory. But it did not get applied at the time, and we have had > enough changes in the meantime that reinstating such a useful patch > would basically be work from scratch at this point] Yea I had that thought too. Pity it got lost. > > > > I'll send a V2 using pid-file without this change. > > Thanks, looking forward to it. > > -- > Eric Blake, Principal Software Engineer > Red Hat, Inc. +1-919-301-3266 > Virtualization: qemu.org | libvirt.org
diff --git a/storage-daemon/qemu-storage-daemon.c b/storage-daemon/qemu-storage-daemon.c index fc8b150629..c4f76d1564 100644 --- a/storage-daemon/qemu-storage-daemon.c +++ b/storage-daemon/qemu-storage-daemon.c @@ -61,6 +61,9 @@ static const char *pid_file; static volatile bool exit_requested = false; +static bool print_setup; + +const char *init_msg = "Block exports setup\n"; void qemu_system_killed(int signal, pid_t pid) { @@ -82,6 +85,7 @@ static void help(void) " -T, --trace [[enable=]<pattern>][,events=<file>][,file=<file>]\n" " specify tracing options\n" " -V, --version output version information and exit\n" +" -P, --printset print to stdout once server is fully initialized\n" "\n" " --blockdev [driver=]<driver>[,node-name=<N>][,discard=ignore|unmap]\n" " [,cache.direct=on|off][,cache.no-flush=on|off]\n" @@ -174,6 +178,7 @@ static void process_options(int argc, char *argv[]) {"nbd-server", required_argument, NULL, OPTION_NBD_SERVER}, {"object", required_argument, NULL, OPTION_OBJECT}, {"pidfile", required_argument, NULL, OPTION_PIDFILE}, + {"printset", no_argument, NULL, 'P'}, {"trace", required_argument, NULL, 'T'}, {"version", no_argument, NULL, 'V'}, {0, 0, 0, 0} @@ -195,6 +200,9 @@ static void process_options(int argc, char *argv[]) trace_opt_parse(optarg); trace_init_file(); break; + case 'P': + print_setup = true; + break; case 'V': printf("qemu-storage-daemon version " QEMU_FULL_VERSION "\n" QEMU_COPYRIGHT "\n"); @@ -310,6 +318,7 @@ static void pid_file_init(void) int main(int argc, char *argv[]) { + int err; #ifdef CONFIG_POSIX signal(SIGPIPE, SIG_IGN); #endif @@ -341,6 +350,18 @@ int main(int argc, char *argv[]) */ pid_file_init(); + /* + * If requested to pipe output once exports are initialized, print to + * stdout. + */ + if (print_setup) { + err = write(1, init_msg, strlen(init_msg)); + if (err == -1) { + fprintf(stderr, "Write to pipe failed: %m\n"); + return -1; + } + } + while (!exit_requested) { main_loop_wait(false); }
This change adds a command line option to print a line to standard out when the storage daemon has completed initialization and is ready to serve client connections. This option will be used to resolve a hang in the vhost-user-blk-test. Signed-off-by: Raphael Norwitz <raphael.norwitz@nutanix.com> --- storage-daemon/qemu-storage-daemon.c | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+)