[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH] qemu-storage-daemon: add --pidfile option
From: |
Richard W.M. Jones |
Subject: |
Re: [PATCH] qemu-storage-daemon: add --pidfile option |
Date: |
Mon, 1 Mar 2021 16:24:09 +0000 |
User-agent: |
Mutt/1.5.21 (2010-09-15) |
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 :|
--
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
Fedora Windows cross-compiler. Compile Windows programs, test, and
build Windows installers. Over 100 libraries supported.
http://fedoraproject.org/wiki/MinGW