qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH v4 20/29] util: add qemu_write_pidfile()


From: Marc-André Lureau
Subject: Re: [Qemu-devel] [PATCH v4 20/29] util: add qemu_write_pidfile()
Date: Tue, 28 Aug 2018 18:04:04 +0200

Hi
On Tue, Aug 28, 2018 at 5:53 PM Daniel P. Berrangé <address@hidden> wrote:
>
> On Fri, Jul 13, 2018 at 03:09:07PM +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 based from pr-helper write_pidfile(), but allows the
> > caller to deal with error reporting and behaviour.
> >
> > Signed-off-by: Marc-André Lureau <address@hidden>
> > ---
> >  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    | 33 ++++++++++++++++++++++++++
> >  util/oslib-win32.c    | 27 ++++++++++++++++++++++
> >  vl.c                  |  4 ++--
> >  8 files changed, 79 insertions(+), 131 deletions(-)
>
>
>
> > diff --git a/util/oslib-posix.c b/util/oslib-posix.c
> > index 13b6f8d776..da1d4a3201 100644
> > --- a/util/oslib-posix.c
> > +++ b/util/oslib-posix.c
> > @@ -88,6 +88,39 @@ int qemu_daemon(int nochdir, int noclose)
> >      return daemon(nochdir, noclose);
> >  }
> >
> > +bool qemu_write_pidfile(const char *pidfile, Error **errp)
> > +{
> > +    int pidfd;
> > +    char pidstr[32];
> > +
> > +    pidfd = qemu_open(pidfile, O_CREAT | O_WRONLY, S_IRUSR | S_IWUSR);
> > +    if (pidfd == -1) {
> > +        error_setg_errno(errp, errno, "Cannot open pid file");
> > +        return false;
> > +    }
> > +
> > +    if (lockf(pidfd, F_TLOCK, 0)) {
> > +        error_setg_errno(errp, errno, "Cannot lock pid file");
> > +        goto fail;
> > +    }
> > +    if (ftruncate(pidfd, 0)) {
> > +        error_setg_errno(errp, errno, "Failed to truncate pid file");
> > +        goto fail;
> > +    }
> > +
> > +    snprintf(pidstr, sizeof(pidstr), "%d\n", getpid());
> > +    if (write(pidfd, pidstr, strlen(pidstr)) != strlen(pidstr)) {
> > +        error_setg(errp, "Failed to write pid file");
> > +        goto fail;
> > +    }
> > +    return true;
> > +
> > +fail:
> > +    unlink(pidfile);
>
> Danger,  Will Robinson !
>
> We can get to this fail: label if we were unable to lockf() the
> pidfile. ie someone else owns the pidfile, and we've now unlinked
> the pidfile they own.

The code was based on qemu-pr-helper, so the problem exists there.

So we better follow ga_open_pidfile() version here, and close the fd,
return an error. ok

>
>
> > +    close(pidfd);
> > +    return false;
> > +}
> > +
> >  void *qemu_oom_check(void *ptr)
> >  {
> >      if (ptr == NULL) {
>
>
> 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 :|
>


-- 
Marc-André Lureau



reply via email to

[Prev in Thread] Current Thread [Next in Thread]