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

[Stefan Hajnoczi]

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

signature.asc
Description: PGP signature