| Message ID | 20210301160857.130478-1-stefanha@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | qemu-storage-daemon: add --pidfile option | expand |
On Mon, Mar 01, 2021 at 04:08:57PM +0000, Stefan Hajnoczi wrote: > Daemons often have a --pidfile option where the pid is written to a file > so that scripts can stop the daemon by sending a signal. > > The pid file also acts as a lock to prevent multiple instances of the > daemon from launching for a given pid file. > > QEMU, qemu-nbd, qemu-ga, virtiofsd, and qemu-pr-helper all support the > --pidfile option. Add it to qemu-storage-daemon too. > > Reported-by: Richard W.M. Jones <rjones@redhat.com> > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> > --- > docs/tools/qemu-storage-daemon.rst | 10 ++++++++++ > storage-daemon/qemu-storage-daemon.c | 29 ++++++++++++++++++++++++++++ > 2 files changed, 39 insertions(+) > > diff --git a/docs/tools/qemu-storage-daemon.rst b/docs/tools/qemu-storage-daemon.rst > index f63627eaf6..8f4ab16ffc 100644 > --- a/docs/tools/qemu-storage-daemon.rst > +++ b/docs/tools/qemu-storage-daemon.rst > @@ -118,6 +118,16 @@ Standard options: > List object properties with ``<type>,help``. See the :manpage:`qemu(1)` > manual page for a description of the object properties. > > +.. option:: --pidfile PATH > + > + is the path to a file where the daemon writes its pid. This allows scripts to > + stop the daemon by sending a signal:: > + > + $ kill -SIGTERM $(<path/to/qsd.pid) > + > + A file lock is applied to the file so only one instance of the daemon can run > + with a given pid file path. The daemon unlinks its pid file when terminating. Usually a pidfile wants to have some explicit synchronization rules defined. AFAICS, qsd doesn't have a --daemonize option to sync against. If we're using the FD passing trick for the monitor, however, we want a guarantee that the pidfile is written before the monitor accepts the first client. IOW, the parent process needs to know that once it has done the QMP handshake, there is guaranteed tobe a pidfile present, or if the QMP handshake fails, then the app is guaranteed to have quit. IIUC, this impl should be ok in this respect, because we won't process the QMP handdshake until the main loop runs, at which point the pidfile is present. So we just need to document that the pidfile is guaranteed to be written by the time QMP is active. > + > Examples > -------- > Launch the daemon with QMP monitor socket ``qmp.sock`` so clients can execute > diff --git a/storage-daemon/qemu-storage-daemon.c b/storage-daemon/qemu-storage-daemon.c > index 9021a46b3a..011ae49ac3 100644 > --- a/storage-daemon/qemu-storage-daemon.c > +++ b/storage-daemon/qemu-storage-daemon.c > @@ -59,6 +59,7 @@ > #include "sysemu/runstate.h" > #include "trace/control.h" > > +static const char *pid_file; > static volatile bool exit_requested = false; > > void qemu_system_killed(int signal, pid_t pid) > @@ -126,6 +127,7 @@ enum { > OPTION_MONITOR, > OPTION_NBD_SERVER, > OPTION_OBJECT, > + OPTION_PIDFILE, > }; > > extern QemuOptsList qemu_chardev_opts; > @@ -164,6 +166,7 @@ static void process_options(int argc, char *argv[]) > {"monitor", required_argument, NULL, OPTION_MONITOR}, > {"nbd-server", required_argument, NULL, OPTION_NBD_SERVER}, > {"object", required_argument, NULL, OPTION_OBJECT}, > + {"pidfile", required_argument, NULL, OPTION_PIDFILE}, > {"trace", required_argument, NULL, 'T'}, > {"version", no_argument, NULL, 'V'}, > {0, 0, 0, 0} > @@ -275,6 +278,9 @@ static void process_options(int argc, char *argv[]) > qobject_unref(args); > break; > } > + case OPTION_PIDFILE: > + pid_file = optarg; > + break; > default: > g_assert_not_reached(); > } > @@ -285,6 +291,27 @@ static void process_options(int argc, char *argv[]) > } > } > > +static void pid_file_cleanup(void) > +{ > + unlink(pid_file); > +} > + > +static void pid_file_init(void) > +{ > + Error *err = NULL; > + > + if (!pid_file) { > + return; > + } > + > + if (!qemu_write_pidfile(pid_file, &err)) { > + error_reportf_err(err, "cannot create PID file: "); > + exit(EXIT_FAILURE); > + } > + > + atexit(pid_file_cleanup); > +} > + > int main(int argc, char *argv[]) > { > #ifdef CONFIG_POSIX > @@ -312,6 +339,8 @@ int main(int argc, char *argv[]) > qemu_init_main_loop(&error_fatal); > process_options(argc, argv); > > + pid_file_init(); > + > while (!exit_requested) { > main_loop_wait(false); > } > -- > 2.29.2 > Regards, Daniel
On Mon, Mar 01, 2021 at 04:15:47PM +0000, Daniel P. Berrangé wrote: > On Mon, Mar 01, 2021 at 04:08:57PM +0000, Stefan Hajnoczi wrote: > > Daemons often have a --pidfile option where the pid is written to a file > > so that scripts can stop the daemon by sending a signal. > > > > The pid file also acts as a lock to prevent multiple instances of the > > daemon from launching for a given pid file. > > > > QEMU, qemu-nbd, qemu-ga, virtiofsd, and qemu-pr-helper all support the > > --pidfile option. Add it to qemu-storage-daemon too. > > > > Reported-by: Richard W.M. Jones <rjones@redhat.com> > > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> > > --- > > docs/tools/qemu-storage-daemon.rst | 10 ++++++++++ > > storage-daemon/qemu-storage-daemon.c | 29 ++++++++++++++++++++++++++++ > > 2 files changed, 39 insertions(+) > > > > diff --git a/docs/tools/qemu-storage-daemon.rst b/docs/tools/qemu-storage-daemon.rst > > index f63627eaf6..8f4ab16ffc 100644 > > --- a/docs/tools/qemu-storage-daemon.rst > > +++ b/docs/tools/qemu-storage-daemon.rst > > @@ -118,6 +118,16 @@ Standard options: > > List object properties with ``<type>,help``. See the :manpage:`qemu(1)` > > manual page for a description of the object properties. > > > > +.. option:: --pidfile PATH > > + > > + is the path to a file where the daemon writes its pid. This allows scripts to > > + stop the daemon by sending a signal:: > > + > > + $ kill -SIGTERM $(<path/to/qsd.pid) > > + > > + A file lock is applied to the file so only one instance of the daemon can run > > + with a given pid file path. The daemon unlinks its pid file when terminating. > > Usually a pidfile wants to have some explicit synchronization rules > defined. AFAICS, qsd doesn't have a --daemonize option to sync against. > If we're using the FD passing trick for the monitor, however, we want > a guarantee that the pidfile is written before the monitor accepts the > first client. > > IOW, the parent process needs to know that once it has done the QMP > handshake, there is guaranteed tobe a pidfile present, or if the > QMP handshake fails, then the app is guaranteed to have quit. > > IIUC, this impl should be ok in this respect, because we won't process > the QMP handdshake until the main loop runs, at which point the pidfile > is present. So we just need to document that the pidfile is guaranteed > to be written by the time QMP is active. I'm not sure if I follow this exactly, but from my point of view I'd like to know that: (1) If we're using --nbd-server addr.type=inet|unix then the PID file must not be created until the socket has been created and is listening. Here I mean the NBD socket, but the same would apply to the QMP socket or any other listening socket. This allows you to do scripting sanely (qemu-storage-daemon ... &) without arbitrary sleeps. (2) If we're using the FD passing trick instead, we don't care and would probably not use the --pidfile option anyway (since we have the PID from calling fork). Rich. > > > + > > Examples > > -------- > > Launch the daemon with QMP monitor socket ``qmp.sock`` so clients can execute > > diff --git a/storage-daemon/qemu-storage-daemon.c b/storage-daemon/qemu-storage-daemon.c > > index 9021a46b3a..011ae49ac3 100644 > > --- a/storage-daemon/qemu-storage-daemon.c > > +++ b/storage-daemon/qemu-storage-daemon.c > > @@ -59,6 +59,7 @@ > > #include "sysemu/runstate.h" > > #include "trace/control.h" > > > > +static const char *pid_file; > > static volatile bool exit_requested = false; > > > > void qemu_system_killed(int signal, pid_t pid) > > @@ -126,6 +127,7 @@ enum { > > OPTION_MONITOR, > > OPTION_NBD_SERVER, > > OPTION_OBJECT, > > + OPTION_PIDFILE, > > }; > > > > extern QemuOptsList qemu_chardev_opts; > > @@ -164,6 +166,7 @@ static void process_options(int argc, char *argv[]) > > {"monitor", required_argument, NULL, OPTION_MONITOR}, > > {"nbd-server", required_argument, NULL, OPTION_NBD_SERVER}, > > {"object", required_argument, NULL, OPTION_OBJECT}, > > + {"pidfile", required_argument, NULL, OPTION_PIDFILE}, > > {"trace", required_argument, NULL, 'T'}, > > {"version", no_argument, NULL, 'V'}, > > {0, 0, 0, 0} > > @@ -275,6 +278,9 @@ static void process_options(int argc, char *argv[]) > > qobject_unref(args); > > break; > > } > > + case OPTION_PIDFILE: > > + pid_file = optarg; > > + break; > > default: > > g_assert_not_reached(); > > } > > @@ -285,6 +291,27 @@ static void process_options(int argc, char *argv[]) > > } > > } > > > > +static void pid_file_cleanup(void) > > +{ > > + unlink(pid_file); > > +} > > + > > +static void pid_file_init(void) > > +{ > > + Error *err = NULL; > > + > > + if (!pid_file) { > > + return; > > + } > > + > > + if (!qemu_write_pidfile(pid_file, &err)) { > > + error_reportf_err(err, "cannot create PID file: "); > > + exit(EXIT_FAILURE); > > + } > > + > > + atexit(pid_file_cleanup); > > +} > > + > > int main(int argc, char *argv[]) > > { > > #ifdef CONFIG_POSIX > > @@ -312,6 +339,8 @@ int main(int argc, char *argv[]) > > qemu_init_main_loop(&error_fatal); > > process_options(argc, argv); > > > > + pid_file_init(); > > + > > while (!exit_requested) { > > main_loop_wait(false); > > } > > -- > > 2.29.2 > > > > Regards, > Daniel > -- > |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| > |: https://libvirt.org -o- https://fstop138.berrange.com :| > |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
On Mon, Mar 01, 2021 at 04:24:09PM +0000, Richard W.M. Jones wrote: > On Mon, Mar 01, 2021 at 04:15:47PM +0000, Daniel P. Berrangé wrote: > > On Mon, Mar 01, 2021 at 04:08:57PM +0000, Stefan Hajnoczi wrote: > > > Daemons often have a --pidfile option where the pid is written to a file > > > so that scripts can stop the daemon by sending a signal. > > > > > > The pid file also acts as a lock to prevent multiple instances of the > > > daemon from launching for a given pid file. > > > > > > QEMU, qemu-nbd, qemu-ga, virtiofsd, and qemu-pr-helper all support the > > > --pidfile option. Add it to qemu-storage-daemon too. > > > > > > Reported-by: Richard W.M. Jones <rjones@redhat.com> > > > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> > > > --- > > > docs/tools/qemu-storage-daemon.rst | 10 ++++++++++ > > > storage-daemon/qemu-storage-daemon.c | 29 ++++++++++++++++++++++++++++ > > > 2 files changed, 39 insertions(+) > > > > > > diff --git a/docs/tools/qemu-storage-daemon.rst b/docs/tools/qemu-storage-daemon.rst > > > index f63627eaf6..8f4ab16ffc 100644 > > > --- a/docs/tools/qemu-storage-daemon.rst > > > +++ b/docs/tools/qemu-storage-daemon.rst > > > @@ -118,6 +118,16 @@ Standard options: > > > List object properties with ``<type>,help``. See the :manpage:`qemu(1)` > > > manual page for a description of the object properties. > > > > > > +.. option:: --pidfile PATH > > > + > > > + is the path to a file where the daemon writes its pid. This allows scripts to > > > + stop the daemon by sending a signal:: > > > + > > > + $ kill -SIGTERM $(<path/to/qsd.pid) > > > + > > > + A file lock is applied to the file so only one instance of the daemon can run > > > + with a given pid file path. The daemon unlinks its pid file when terminating. > > > > Usually a pidfile wants to have some explicit synchronization rules > > defined. AFAICS, qsd doesn't have a --daemonize option to sync against. > > If we're using the FD passing trick for the monitor, however, we want > > a guarantee that the pidfile is written before the monitor accepts the > > first client. > > > > IOW, the parent process needs to know that once it has done the QMP > > handshake, there is guaranteed tobe a pidfile present, or if the > > QMP handshake fails, then the app is guaranteed to have quit. > > > > IIUC, this impl should be ok in this respect, because we won't process > > the QMP handdshake until the main loop runs, at which point the pidfile > > is present. So we just need to document that the pidfile is guaranteed > > to be written by the time QMP is active. > > I'm not sure if I follow this exactly, but from my point of view I'd > like to know that: > > (1) If we're using --nbd-server addr.type=inet|unix then the PID file > must not be created until the socket has been created and is > listening. Here I mean the NBD socket, but the same would apply to > the QMP socket or any other listening socket. This allows you to do > scripting sanely (qemu-storage-daemon ... &) without arbitrary sleeps. Okay. This is guaranteed by the code (--chardev creates the character device and listens before the pid file is written). > (2) If we're using the FD passing trick instead, we don't care and > would probably not use the --pidfile option anyway (since we have the > PID from calling fork). Yep. I will document the things that you and Dan mentioned. Stefan
diff --git a/docs/tools/qemu-storage-daemon.rst b/docs/tools/qemu-storage-daemon.rst index f63627eaf6..8f4ab16ffc 100644 --- a/docs/tools/qemu-storage-daemon.rst +++ b/docs/tools/qemu-storage-daemon.rst @@ -118,6 +118,16 @@ Standard options: List object properties with ``<type>,help``. See the :manpage:`qemu(1)` manual page for a description of the object properties. +.. option:: --pidfile PATH + + is the path to a file where the daemon writes its pid. This allows scripts to + stop the daemon by sending a signal:: + + $ kill -SIGTERM $(<path/to/qsd.pid) + + A file lock is applied to the file so only one instance of the daemon can run + with a given pid file path. The daemon unlinks its pid file when terminating. + Examples -------- Launch the daemon with QMP monitor socket ``qmp.sock`` so clients can execute diff --git a/storage-daemon/qemu-storage-daemon.c b/storage-daemon/qemu-storage-daemon.c index 9021a46b3a..011ae49ac3 100644 --- a/storage-daemon/qemu-storage-daemon.c +++ b/storage-daemon/qemu-storage-daemon.c @@ -59,6 +59,7 @@ #include "sysemu/runstate.h" #include "trace/control.h" +static const char *pid_file; static volatile bool exit_requested = false; void qemu_system_killed(int signal, pid_t pid) @@ -126,6 +127,7 @@ enum { OPTION_MONITOR, OPTION_NBD_SERVER, OPTION_OBJECT, + OPTION_PIDFILE, }; extern QemuOptsList qemu_chardev_opts; @@ -164,6 +166,7 @@ static void process_options(int argc, char *argv[]) {"monitor", required_argument, NULL, OPTION_MONITOR}, {"nbd-server", required_argument, NULL, OPTION_NBD_SERVER}, {"object", required_argument, NULL, OPTION_OBJECT}, + {"pidfile", required_argument, NULL, OPTION_PIDFILE}, {"trace", required_argument, NULL, 'T'}, {"version", no_argument, NULL, 'V'}, {0, 0, 0, 0} @@ -275,6 +278,9 @@ static void process_options(int argc, char *argv[]) qobject_unref(args); break; } + case OPTION_PIDFILE: + pid_file = optarg; + break; default: g_assert_not_reached(); } @@ -285,6 +291,27 @@ static void process_options(int argc, char *argv[]) } } +static void pid_file_cleanup(void) +{ + unlink(pid_file); +} + +static void pid_file_init(void) +{ + Error *err = NULL; + + if (!pid_file) { + return; + } + + if (!qemu_write_pidfile(pid_file, &err)) { + error_reportf_err(err, "cannot create PID file: "); + exit(EXIT_FAILURE); + } + + atexit(pid_file_cleanup); +} + int main(int argc, char *argv[]) { #ifdef CONFIG_POSIX @@ -312,6 +339,8 @@ int main(int argc, char *argv[]) qemu_init_main_loop(&error_fatal); process_options(argc, argv); + pid_file_init(); + while (!exit_requested) { main_loop_wait(false); }
Daemons often have a --pidfile option where the pid is written to a file so that scripts can stop the daemon by sending a signal. The pid file also acts as a lock to prevent multiple instances of the daemon from launching for a given pid file. QEMU, qemu-nbd, qemu-ga, virtiofsd, and qemu-pr-helper all support the --pidfile option. Add it to qemu-storage-daemon too. Reported-by: Richard W.M. Jones <rjones@redhat.com> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> --- docs/tools/qemu-storage-daemon.rst | 10 ++++++++++ storage-daemon/qemu-storage-daemon.c | 29 ++++++++++++++++++++++++++++ 2 files changed, 39 insertions(+)