Re: [Qemu-devel] [PATCH 1/3] util: add qemu_write_pidfile()

["Daniel P. Berrangé" <berrange@redhat.com>]

On Fri, Aug 31, 2018 at 04:53:12PM +0200, Marc-André Lureau wrote:
> There are variants of qemu_create_pidfile() in qemu-pr-helper and
> qemu-ga. Let's have a common implementation in libqemuutil.
> 
> The code is initially based from pr-helper write_pidfile(), with
> various improvements and suggestions from Daniel Berrangé:
> 
>   QEMU will leave the pidfile existing on disk when it exits which
>   initially made me think it avoids the deletion race. The app
>   managing QEMU, however, may well delete the pidfile after it has
>   seen QEMU exit, and even if the app locks the pidfile before
>   deleting it, there is still a race.
> 
>   eg consider the following sequence
> 
>         QEMU 1        libvirtd        QEMU 2
> 
>   1.    lock(pidfile)
> 
>   2.    exit()
> 
>   3.                 open(pidfile)
> 
>   4.                 lock(pidfile)
> 
>   5.                                  open(pidfile)
> 
>   6.                 unlink(pidfile)
> 
>   7.                 close(pidfile)
> 
>   8.                                  lock(pidfile)
> 
>   IOW, at step 8 the new QEMU has successfully acquired the lock, but
>   the pidfile no longer exists on disk because it was deleted after
>   the original QEMU exited.
> 
>   While we could just say no external app should ever delete the
>   pidfile, I don't think that is satisfactory as people don't read
>   docs, and admins don't like stale pidfiles being left around on
>   disk.
> 
>   To make this robust, I think we might want to copy libvirt's
>   approach to pidfile acquisition which runs in a loop and checks that
>   the file on disk /after/ acquiring the lock matches the file that
>   was locked. Then we could in fact safely let QEMU delete its own
>   pidfiles on clean exit..
> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  include/qemu/osdep.h  |  3 +-
>  os-posix.c            | 24 ---------------
>  os-win32.c            | 25 ----------------
>  qga/main.c            | 54 +++++++---------------------------
>  scsi/qemu-pr-helper.c | 40 ++++---------------------
>  util/oslib-posix.c    | 68 +++++++++++++++++++++++++++++++++++++++++++
>  util/oslib-win32.c    | 27 +++++++++++++++++
>  vl.c                  |  4 +--
>  8 files changed, 114 insertions(+), 131 deletions(-)

Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>


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 :|