Re: [PATCH] qemu-storage-daemon: add --pidfile option

[Richard W.M. Jones]

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