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

["Richard W.M. Jones" <rjones@redhat.com>]

On Tue, Mar 02, 2021 at 09:29:26AM +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>
> ---
> v2:
>  * Add documentation about startup order [Rich, Daniel]
> 
>  docs/tools/qemu-storage-daemon.rst   | 14 ++++++++++++
>  storage-daemon/qemu-storage-daemon.c | 34 ++++++++++++++++++++++++++++
>  2 files changed, 48 insertions(+)
> 
> diff --git a/docs/tools/qemu-storage-daemon.rst b/docs/tools/qemu-storage-daemon.rst
> index f63627eaf6..3d9704d835 100644
> --- a/docs/tools/qemu-storage-daemon.rst
> +++ b/docs/tools/qemu-storage-daemon.rst
> @@ -118,6 +118,20 @@ 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.
> +
> +  The pid file is written after chardevs, exports, and NBD servers have been
> +  created but before accepting connections. The daemon has started successfully
> +  when the pid file is written and clients may begin connecting.
> +
>  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..86cf6a1f08 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,13 @@ int main(int argc, char *argv[])
>      qemu_init_main_loop(&error_fatal);
>      process_options(argc, argv);
>  
> +    /*
> +     * Write the pid file after creating chardevs, exports, and NBD servers but
> +     * before accepting connections. This ordering is documented. Do not change
> +     * it.
> +     */
> +    pid_file_init();
> +
>      while (!exit_requested) {
>          main_loop_wait(false);
>      }
> -- 

Looks good:

Reviewed-by: Richard W.M. Jones <rjones@redhat.com>

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-df lists disk usage of guests without needing to install any
software inside the virtual machine.  Supports Linux and Windows.
http://people.redhat.com/~rjones/virt-df/