[Qemu-devel] [PATCH v2 0/3] util: add qemu_write_pidfile()

[Qemu-devel] [PATCH v2 0/3] util: add qemu_write_pidfile()

[Marc-André Lureau]

Hi,

Here are a few PID file related patches extracted from "[PATCH v4
00/29] vhost-user for input & GPU" series, with suggestions from
Daniel Berrangé.

thanks

v2:
- use an exit notifier (instead of atexit handler)
- add an r-b tag

Marc-André Lureau (3):
  util: add qemu_write_pidfile()
  util: use fcntl() for qemu_write_pidfile() locking
  Delete PID file on exit

 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    | 73 +++++++++++++++++++++++++++++++++++++++++++
 util/oslib-win32.c    | 27 ++++++++++++++++
 vl.c                  | 18 +++++++++--
 8 files changed, 132 insertions(+), 132 deletions(-)

-- 
2.19.0.rc1

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

[Marc-André Lureau]

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>
Reviewed-by: Daniel P. Berrangé <berrange@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(-)

diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h
index a91068df0e..47fa570bd4 100644
--- a/include/qemu/osdep.h
+++ b/include/qemu/osdep.h
@@ -448,7 +448,8 @@ bool qemu_has_ofd_lock(void);
 #define FMT_pid "%d"
 #endif
 
-int qemu_create_pidfile(const char *filename);
+bool qemu_write_pidfile(const char *pidfile, Error **errp);
+
 int qemu_get_thread_id(void);
 
 #ifndef CONFIG_IOVEC
diff --git a/os-posix.c b/os-posix.c
index 9ce6f74513..0e9403b4ff 100644
--- a/os-posix.c
+++ b/os-posix.c
@@ -352,30 +352,6 @@ void os_set_line_buffering(void)
     setvbuf(stdout, NULL, _IOLBF, 0);
 }
 
-int qemu_create_pidfile(const char *filename)
-{
-    char buffer[128];
-    int len;
-    int fd;
-
-    fd = qemu_open(filename, O_RDWR | O_CREAT, 0600);
-    if (fd == -1) {
-        return -1;
-    }
-    if (lockf(fd, F_TLOCK, 0) == -1) {
-        close(fd);
-        return -1;
-    }
-    len = snprintf(buffer, sizeof(buffer), FMT_pid "\n", getpid());
-    if (write(fd, buffer, len) != len) {
-        close(fd);
-        return -1;
-    }
-
-    /* keep pidfile open & locked forever */
-    return 0;
-}
-
 bool is_daemonized(void)
 {
     return daemonize;
diff --git a/os-win32.c b/os-win32.c
index 0674f94b57..0e0d7f50f3 100644
--- a/os-win32.c
+++ b/os-win32.c
@@ -97,28 +97,3 @@ int os_parse_cmd_args(int index, const char *optarg)
 {
     return -1;
 }
-
-int qemu_create_pidfile(const char *filename)
-{
-    char buffer[128];
-    int len;
-    HANDLE file;
-    OVERLAPPED overlap;
-    BOOL ret;
-    memset(&overlap, 0, sizeof(overlap));
-
-    file = CreateFile(filename, GENERIC_WRITE, FILE_SHARE_READ, NULL,
-                      OPEN_ALWAYS, FILE_ATTRIBUTE_NORMAL, NULL);
-
-    if (file == INVALID_HANDLE_VALUE) {
-        return -1;
-    }
-    len = snprintf(buffer, sizeof(buffer), "%d\n", getpid());
-    ret = WriteFile(file, (LPCVOID)buffer, (DWORD)len,
-                    NULL, &overlap);
-    CloseHandle(file);
-    if (ret == 0) {
-        return -1;
-    }
-    return 0;
-}
diff --git a/qga/main.c b/qga/main.c
index 6d70242d05..c399320d3c 100644
--- a/qga/main.c
+++ b/qga/main.c
@@ -340,46 +340,6 @@ static FILE *ga_open_logfile(const char *logfile)
     return f;
 }
 
-#ifndef _WIN32
-static bool ga_open_pidfile(const char *pidfile)
-{
-    int pidfd;
-    char pidstr[32];
-
-    pidfd = qemu_open(pidfile, O_CREAT|O_WRONLY, S_IRUSR|S_IWUSR);
-    if (pidfd == -1 || lockf(pidfd, F_TLOCK, 0)) {
-        g_critical("Cannot lock pid file, %s", strerror(errno));
-        if (pidfd != -1) {
-            close(pidfd);
-        }
-        return false;
-    }
-
-    if (ftruncate(pidfd, 0)) {
-        g_critical("Failed to truncate pid file");
-        goto fail;
-    }
-    snprintf(pidstr, sizeof(pidstr), "%d\n", getpid());
-    if (write(pidfd, pidstr, strlen(pidstr)) != strlen(pidstr)) {
-        g_critical("Failed to write pid file");
-        goto fail;
-    }
-
-    /* keep pidfile open & locked forever */
-    return true;
-
-fail:
-    unlink(pidfile);
-    close(pidfd);
-    return false;
-}
-#else /* _WIN32 */
-static bool ga_open_pidfile(const char *pidfile)
-{
-    return true;
-}
-#endif
-
 static gint ga_strcmp(gconstpointer str1, gconstpointer str2)
 {
     return strcmp(str1, str2);
@@ -479,8 +439,11 @@ void ga_unset_frozen(GAState *s)
     ga_enable_logging(s);
     g_warning("logging re-enabled due to filesystem unfreeze");
     if (s->deferred_options.pid_filepath) {
-        if (!ga_open_pidfile(s->deferred_options.pid_filepath)) {
-            g_warning("failed to create/open pid file");
+        Error *err = NULL;
+
+        if (!qemu_write_pidfile(s->deferred_options.pid_filepath, &err)) {
+            g_warning("%s", error_get_pretty(err));
+            error_free(err);
         }
         s->deferred_options.pid_filepath = NULL;
     }
@@ -515,8 +478,11 @@ static void become_daemon(const char *pidfile)
     }
 
     if (pidfile) {
-        if (!ga_open_pidfile(pidfile)) {
-            g_critical("failed to create pidfile");
+        Error *err = NULL;
+
+        if (!qemu_write_pidfile(pidfile, &err)) {
+            g_critical("%s", error_get_pretty(err));
+            error_free(err);
             exit(EXIT_FAILURE);
         }
     }
diff --git a/scsi/qemu-pr-helper.c b/scsi/qemu-pr-helper.c
index ed037aabee..ce40008bfc 100644
--- a/scsi/qemu-pr-helper.c
+++ b/scsi/qemu-pr-helper.c
@@ -117,39 +117,6 @@ QEMU_COPYRIGHT "\n"
     , name);
 }
 
-static void write_pidfile(void)
-{
-    int pidfd;
-    char pidstr[32];
-
-    pidfd = qemu_open(pidfile, O_CREAT|O_WRONLY, S_IRUSR|S_IWUSR);
-    if (pidfd == -1) {
-        error_report("Cannot open pid file, %s", strerror(errno));
-        exit(EXIT_FAILURE);
-    }
-
-    if (lockf(pidfd, F_TLOCK, 0)) {
-        error_report("Cannot lock pid file, %s", strerror(errno));
-        goto fail;
-    }
-    if (ftruncate(pidfd, 0)) {
-        error_report("Failed to truncate pid file");
-        goto fail;
-    }
-
-    snprintf(pidstr, sizeof(pidstr), "%d\n", getpid());
-    if (write(pidfd, pidstr, strlen(pidstr)) != strlen(pidstr)) {
-        error_report("Failed to write pid file");
-        goto fail;
-    }
-    return;
-
-fail:
-    unlink(pidfile);
-    close(pidfd);
-    exit(EXIT_FAILURE);
-}
-
 /* SG_IO support */
 
 typedef struct PRHelperSGIOData {
@@ -1080,8 +1047,11 @@ int main(int argc, char **argv)
         }
     }
 
-    if (daemonize || pidfile_specified)
-        write_pidfile();
+    if ((daemonize || pidfile_specified) &&
+        !qemu_write_pidfile(pidfile, &local_err)) {
+        error_report_err(local_err);
+        exit(EXIT_FAILURE);
+    }
 
 #ifdef CONFIG_LIBCAP
     if (drop_privileges() < 0) {
diff --git a/util/oslib-posix.c b/util/oslib-posix.c
index 13b6f8d776..0e3ab9d959 100644
--- a/util/oslib-posix.c
+++ b/util/oslib-posix.c
@@ -88,6 +88,74 @@ int qemu_daemon(int nochdir, int noclose)
     return daemon(nochdir, noclose);
 }
 
+bool qemu_write_pidfile(const char *path, Error **errp)
+{
+    int fd;
+    char pidstr[32];
+
+    while (1) {
+        struct stat a, b;
+
+        fd = qemu_open(path, O_CREAT | O_WRONLY, S_IRUSR | S_IWUSR);
+        if (fd == -1) {
+            error_setg_errno(errp, errno, "Cannot open pid file");
+            return false;
+        }
+
+        if (fstat(fd, &b) < 0) {
+            error_setg_errno(errp, errno, "Cannot stat file");
+            goto fail_close;
+        }
+
+        if (lockf(fd, F_TLOCK, 0) < 0) {
+            error_setg_errno(errp, errno, "Cannot lock pid file");
+            goto fail_close;
+        }
+
+        /*
+         * Now make sure the path we locked is the same one that now
+         * exists on the filesystem.
+         */
+        if (stat(path, &a) < 0) {
+            /*
+             * PID file disappeared, someone else must be racing with
+             * us, so try again.
+             */
+            close(fd);
+            continue;
+        }
+
+        if (a.st_ino == b.st_ino) {
+            break;
+        }
+
+        /*
+         * PID file was recreated, someone else must be racing with
+         * us, so try again.
+         */
+        close(fd);
+    }
+
+    if (ftruncate(fd, 0) < 0) {
+        error_setg_errno(errp, errno, "Failed to truncate pid file");
+        goto fail_unlink;
+    }
+
+    snprintf(pidstr, sizeof(pidstr), FMT_pid "\n", getpid());
+    if (write(fd, pidstr, strlen(pidstr)) != strlen(pidstr)) {
+        error_setg(errp, "Failed to write pid file");
+        goto fail_unlink;
+    }
+
+    return true;
+
+fail_unlink:
+    unlink(path);
+fail_close:
+    close(fd);
+    return false;
+}
+
 void *qemu_oom_check(void *ptr)
 {
     if (ptr == NULL) {
diff --git a/util/oslib-win32.c b/util/oslib-win32.c
index 25dd1595ad..a83fc24a33 100644
--- a/util/oslib-win32.c
+++ b/util/oslib-win32.c
@@ -776,3 +776,30 @@ ssize_t qemu_recvfrom_wrap(int sockfd, void *buf, size_t len, int flags,
     }
     return ret;
 }
+
+bool qemu_write_pidfile(const char *filename, Error **errp)
+{
+    char buffer[128];
+    int len;
+    HANDLE file;
+    OVERLAPPED overlap;
+    BOOL ret;
+    memset(&overlap, 0, sizeof(overlap));
+
+    file = CreateFile(filename, GENERIC_WRITE, FILE_SHARE_READ, NULL,
+                      OPEN_ALWAYS, FILE_ATTRIBUTE_NORMAL, NULL);
+
+    if (file == INVALID_HANDLE_VALUE) {
+        error_setg(errp, "Failed to create PID file");
+        return false;
+    }
+    len = snprintf(buffer, sizeof(buffer), FMT_pid "\n", getpid());
+    ret = WriteFile(file, (LPCVOID)buffer, (DWORD)len,
+                    NULL, &overlap);
+    CloseHandle(file);
+    if (ret == 0) {
+        error_setg(errp, "Failed to write PID file");
+        return false;
+    }
+    return true;
+}
diff --git a/vl.c b/vl.c
index 5ba06adf78..0eaf948d32 100644
--- a/vl.c
+++ b/vl.c
@@ -3996,8 +3996,8 @@ int main(int argc, char **argv, char **envp)
     os_daemonize();
     rcu_disable_atfork();
 
-    if (pid_file && qemu_create_pidfile(pid_file) != 0) {
-        error_report("could not acquire pid file: %s", strerror(errno));
+    if (pid_file && !qemu_write_pidfile(pid_file, &err)) {
+        error_reportf_err(err, "cannot create PID file: ");
         exit(1);
     }
 
-- 
2.19.0.rc1

[Qemu-devel] [PATCH v2 2/3] util: use fcntl() for qemu_write_pidfile() locking

[Marc-André Lureau]

Daniel Berrangé suggested to use fcntl() locks rather than lockf().

'man lockf':

   On Linux, lockf() is just an interface on top of fcntl(2) locking.
   Many other systems implement lockf() in this way, but note that
   POSIX.1 leaves the relationship between lockf() and fcntl(2) locks
   unspecified.  A portable application should probably avoid mixing
   calls to these interfaces.

IOW, if its just a shim around fcntl() on many systems, it is clearer
if we just use fcntl() directly, as we then know how fcntl() locks will
behave if they're on a network filesystem like NFS.

Suggested-by: Daniel P. Berrangé <berrange@redhat.com>
Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 util/oslib-posix.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/util/oslib-posix.c b/util/oslib-posix.c
index 0e3ab9d959..fbd0dc8c57 100644
--- a/util/oslib-posix.c
+++ b/util/oslib-posix.c
@@ -95,6 +95,11 @@ bool qemu_write_pidfile(const char *path, Error **errp)
 
     while (1) {
         struct stat a, b;
+        struct flock lock = {
+            .l_type = F_WRLCK,
+            .l_whence = SEEK_SET,
+            .l_len = 0,
+        };
 
         fd = qemu_open(path, O_CREAT | O_WRONLY, S_IRUSR | S_IWUSR);
         if (fd == -1) {
@@ -107,7 +112,7 @@ bool qemu_write_pidfile(const char *path, Error **errp)
             goto fail_close;
         }
 
-        if (lockf(fd, F_TLOCK, 0) < 0) {
+        if (fcntl(fd, F_SETLK, &lock)) {
             error_setg_errno(errp, errno, "Cannot lock pid file");
             goto fail_close;
         }
-- 
2.19.0.rc1

[Qemu-devel] [PATCH v2 3/3] Delete PID file on exit

[Marc-André Lureau]

Register an exit notifier to remove the PID file. By the time atexit()
is called, qemu_write_pidfile() guarantees QEMU owns the PID file,
thus we could safely remove it when exiting.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 vl.c | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/vl.c b/vl.c
index 0eaf948d32..51af016602 100644
--- a/vl.c
+++ b/vl.c
@@ -2603,6 +2603,16 @@ static void qemu_run_exit_notifiers(void)
     notifier_list_notify(&exit_notifiers, NULL);
 }
 
+static const char *pid_file;
+static Notifier qemu_unlink_pidfile_notifier;
+
+static void qemu_unlink_pidfile(Notifier *n, void *data)
+{
+    if (pid_file) {
+        unlink(pid_file);
+    }
+}
+
 bool machine_init_done;
 
 void qemu_add_machine_init_done_notifier(Notifier *notify)
@@ -2927,7 +2937,6 @@ int main(int argc, char **argv, char **envp)
     const char *vga_model = NULL;
     const char *qtest_chrdev = NULL;
     const char *qtest_log = NULL;
-    const char *pid_file = NULL;
     const char *incoming = NULL;
     bool userconfig = true;
     bool nographic = false;
@@ -4001,6 +4010,9 @@ int main(int argc, char **argv, char **envp)
         exit(1);
     }
 
+    qemu_unlink_pidfile_notifier.notify = qemu_unlink_pidfile;
+    qemu_add_exit_notifier(&qemu_unlink_pidfile_notifier);
+
     if (qemu_init_main_loop(&main_loop_err)) {
         error_report_err(main_loop_err);
         exit(1);
-- 
2.19.0.rc1

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

[Eric Blake]

On 09/07/2018 07:13 AM, 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.
> 

Unrelated to this patch, but a question that this raises: should 
'qemu-nbd' also have a mode for creating a pid file, since it has --fork 
for daemonizing the server process?

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

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

[Paolo Bonzini]

On 07/09/2018 16:03, Eric Blake wrote:
> On 09/07/2018 07:13 AM, 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.
>>
> 
> Unrelated to this patch, but a question that this raises: should
> 'qemu-nbd' also have a mode for creating a pid file, since it has --fork
> for daemonizing the server process?

Yes, it's probably a good idea.  --fork mode is used mostly with -c/-d,
but why not.

Paolo

Re: [Qemu-devel] [PATCH v2 3/3] Delete PID file on exit

[Paolo Bonzini]

On 07/09/2018 14:13, Marc-André Lureau wrote:
> Register an exit notifier to remove the PID file. By the time atexit()
> is called, qemu_write_pidfile() guarantees QEMU owns the PID file,
> thus we could safely remove it when exiting.
> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>

Queued this one, too.

Paolo

> ---
>  vl.c | 14 +++++++++++++-
>  1 file changed, 13 insertions(+), 1 deletion(-)
> 
> diff --git a/vl.c b/vl.c
> index 0eaf948d32..51af016602 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -2603,6 +2603,16 @@ static void qemu_run_exit_notifiers(void)
>      notifier_list_notify(&exit_notifiers, NULL);
>  }
>  
> +static const char *pid_file;
> +static Notifier qemu_unlink_pidfile_notifier;
> +
> +static void qemu_unlink_pidfile(Notifier *n, void *data)
> +{
> +    if (pid_file) {
> +        unlink(pid_file);
> +    }
> +}
> +
>  bool machine_init_done;
>  
>  void qemu_add_machine_init_done_notifier(Notifier *notify)
> @@ -2927,7 +2937,6 @@ int main(int argc, char **argv, char **envp)
>      const char *vga_model = NULL;
>      const char *qtest_chrdev = NULL;
>      const char *qtest_log = NULL;
> -    const char *pid_file = NULL;
>      const char *incoming = NULL;
>      bool userconfig = true;
>      bool nographic = false;
> @@ -4001,6 +4010,9 @@ int main(int argc, char **argv, char **envp)
>          exit(1);
>      }
>  
> +    qemu_unlink_pidfile_notifier.notify = qemu_unlink_pidfile;
> +    qemu_add_exit_notifier(&qemu_unlink_pidfile_notifier);
> +
>      if (qemu_init_main_loop(&main_loop_err)) {
>          error_report_err(main_loop_err);
>          exit(1);
>