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

[Daniel P . Berrangé]

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>

> 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);
> +    close(pidfd);
> +    return false;
> +}

Thinking about this again, I think it is not robust enough.

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.

    while (1) {
        struct stat a, b;
        if ((fd = open(path, O_WRONLY|O_CREAT, 0644)) < 0) {
            return -1;
        }

        if (fstat(fd, &b) < 0) {
            close(fd);
            return -1;
        }

        if (lockf(fd, F_TLOCK, 0) < 0) {
            close(fd);
            return -1;
        }

        /* Now make sure the pidfile we locked is the same
         * one that now exists on the filesystem
         */
        if (stat(path, &a) < 0) {
            close(fd);
            /* Someone else must be racing with us, so try again */
            continue;
        }
        
        if (a.st_ino == b.st_ino)
            break;

        close(fd);
        /* Someone else must be racing with us, so try again */
    }


    if (ftruncate(fd, 0)) {
        close(fd);
        return -1;
    }

    snprintf(pidstr, sizeof(pidstr), "%d\n", getpid());
    if (write(fd, pidstr, strlen(pidstr)) != strlen(pidstr)) {
        close(fd);
        return -1;
    }

    return fd;


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