All of lore.kernel.org
 help / color / mirror / Atom feed
* [Qemu-devel] [PATCH 0/3] util: add qemu_write_pidfile()
@ 2018-08-31 14:53 Marc-André Lureau
  2018-08-31 14:53 ` [Qemu-devel] [PATCH 1/3] " Marc-André Lureau
                   ` (3 more replies)
  0 siblings, 4 replies; 8+ messages in thread
From: Marc-André Lureau @ 2018-08-31 14:53 UTC (permalink / raw)
  To: qemu-devel
  Cc: Paolo Bonzini, berrange, qemu-block, Stefan Weil, Fam Zheng,
	Michael Roth, 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

Marc-André Lureau (3):
  util: add qemu_write_pidfile()
  util: use fcntl() for qemu_write_pidfile() locking
  RFC: 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                  | 15 +++++++--
 8 files changed, 129 insertions(+), 132 deletions(-)

-- 
2.19.0.rc1

^ permalink raw reply	[flat|nested] 8+ messages in thread

* [Qemu-devel] [PATCH 1/3] util: add qemu_write_pidfile()
  2018-08-31 14:53 [Qemu-devel] [PATCH 0/3] util: add qemu_write_pidfile() Marc-André Lureau
@ 2018-08-31 14:53 ` Marc-André Lureau
  2018-09-03  9:55   ` Daniel P. Berrangé
  2018-08-31 14:53 ` [Qemu-devel] [PATCH 2/3] util: use fcntl() for qemu_write_pidfile() locking Marc-André Lureau
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 8+ messages in thread
From: Marc-André Lureau @ 2018-08-31 14:53 UTC (permalink / raw)
  To: qemu-devel
  Cc: Paolo Bonzini, berrange, qemu-block, Stefan Weil, Fam Zheng,
	Michael Roth, 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>
---
 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

^ permalink raw reply related	[flat|nested] 8+ messages in thread

* [Qemu-devel] [PATCH 2/3] util: use fcntl() for qemu_write_pidfile() locking
  2018-08-31 14:53 [Qemu-devel] [PATCH 0/3] util: add qemu_write_pidfile() Marc-André Lureau
  2018-08-31 14:53 ` [Qemu-devel] [PATCH 1/3] " Marc-André Lureau
@ 2018-08-31 14:53 ` Marc-André Lureau
  2018-08-31 14:53 ` [Qemu-devel] [PATCH 3/3] RFC: delete PID file on exit Marc-André Lureau
  2018-09-11 13:14 ` [Qemu-devel] [PATCH 0/3] util: add qemu_write_pidfile() Paolo Bonzini
  3 siblings, 0 replies; 8+ messages in thread
From: Marc-André Lureau @ 2018-08-31 14:53 UTC (permalink / raw)
  To: qemu-devel
  Cc: Paolo Bonzini, berrange, qemu-block, Stefan Weil, Fam Zheng,
	Michael Roth, 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

^ permalink raw reply related	[flat|nested] 8+ messages in thread

* [Qemu-devel] [PATCH 3/3] RFC: delete PID file on exit
  2018-08-31 14:53 [Qemu-devel] [PATCH 0/3] util: add qemu_write_pidfile() Marc-André Lureau
  2018-08-31 14:53 ` [Qemu-devel] [PATCH 1/3] " Marc-André Lureau
  2018-08-31 14:53 ` [Qemu-devel] [PATCH 2/3] util: use fcntl() for qemu_write_pidfile() locking Marc-André Lureau
@ 2018-08-31 14:53 ` Marc-André Lureau
  2018-08-31 16:29   ` Stefan Weil
  2018-09-11 13:14 ` [Qemu-devel] [PATCH 0/3] util: add qemu_write_pidfile() Paolo Bonzini
  3 siblings, 1 reply; 8+ messages in thread
From: Marc-André Lureau @ 2018-08-31 14:53 UTC (permalink / raw)
  To: qemu-devel
  Cc: Paolo Bonzini, berrange, qemu-block, Stefan Weil, Fam Zheng,
	Michael Roth, Marc-André Lureau

Register an exit handler 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 | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/vl.c b/vl.c
index 0eaf948d32..743679ddb7 100644
--- a/vl.c
+++ b/vl.c
@@ -2603,6 +2603,15 @@ static void qemu_run_exit_notifiers(void)
     notifier_list_notify(&exit_notifiers, NULL);
 }
 
+static const char *pid_file;
+
+static void qemu_unlink_pidfile(void)
+{
+    if (pid_file) {
+        unlink(pid_file);
+    }
+}
+
 bool machine_init_done;
 
 void qemu_add_machine_init_done_notifier(Notifier *notify)
@@ -2927,7 +2936,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;
@@ -4000,6 +4008,7 @@ int main(int argc, char **argv, char **envp)
         error_reportf_err(err, "cannot create PID file: ");
         exit(1);
     }
+    atexit(qemu_unlink_pidfile);
 
     if (qemu_init_main_loop(&main_loop_err)) {
         error_report_err(main_loop_err);
-- 
2.19.0.rc1

^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [Qemu-devel] [PATCH 3/3] RFC: delete PID file on exit
  2018-08-31 14:53 ` [Qemu-devel] [PATCH 3/3] RFC: delete PID file on exit Marc-André Lureau
@ 2018-08-31 16:29   ` Stefan Weil
  2018-08-31 16:35     ` Marc-André Lureau
  0 siblings, 1 reply; 8+ messages in thread
From: Stefan Weil @ 2018-08-31 16:29 UTC (permalink / raw)
  To: Marc-André Lureau, qemu-devel
  Cc: Paolo Bonzini, berrange, qemu-block, Fam Zheng, Michael Roth

Am 31.08.2018 um 16:53 schrieb Marc-André Lureau:
[...]
> +static const char *pid_file;
> +
> +static void qemu_unlink_pidfile(void)
> +{
> +    if (pid_file) {
> +        unlink(pid_file);
> +    }
> +}
> +
>  bool machine_init_done;
>  
>  void qemu_add_machine_init_done_notifier(Notifier *notify)
> @@ -2927,7 +2936,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;
> @@ -4000,6 +4008,7 @@ int main(int argc, char **argv, char **envp)
>          error_reportf_err(err, "cannot create PID file: ");
>          exit(1);
>      }
> +    atexit(qemu_unlink_pidfile);


Could qemu_unlink_pidfile be combined with qemu_run_exit_notifiers to
avoid an additional call of atexit from the same function (main)?

Even if there is support for 32 or more atexit calls, it might be good
to be a little bit more restrictive.

Regards
Stefan

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [Qemu-devel] [PATCH 3/3] RFC: delete PID file on exit
  2018-08-31 16:29   ` Stefan Weil
@ 2018-08-31 16:35     ` Marc-André Lureau
  0 siblings, 0 replies; 8+ messages in thread
From: Marc-André Lureau @ 2018-08-31 16:35 UTC (permalink / raw)
  To: Stefan Weil
  Cc: qemu-devel, Paolo Bonzini, P. Berrange, Daniel, qemu-block,
	Fam Zheng, Michael Roth

Hi

On Fri, Aug 31, 2018 at 6:29 PM, Stefan Weil <sw@weilnetz.de> wrote:
> Am 31.08.2018 um 16:53 schrieb Marc-André Lureau:
> [...]
>> +static const char *pid_file;
>> +
>> +static void qemu_unlink_pidfile(void)
>> +{
>> +    if (pid_file) {
>> +        unlink(pid_file);
>> +    }
>> +}
>> +
>>  bool machine_init_done;
>>
>>  void qemu_add_machine_init_done_notifier(Notifier *notify)
>> @@ -2927,7 +2936,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;
>> @@ -4000,6 +4008,7 @@ int main(int argc, char **argv, char **envp)
>>          error_reportf_err(err, "cannot create PID file: ");
>>          exit(1);
>>      }
>> +    atexit(qemu_unlink_pidfile);
>
>
> Could qemu_unlink_pidfile be combined with qemu_run_exit_notifiers to
> avoid an additional call of atexit from the same function (main)?
>
> Even if there is support for 32 or more atexit calls, it might be good
> to be a little bit more restrictive.

Sure, that would require an additional Notifier, but that's not much anyway.
I'll update the patch.

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [Qemu-devel] [PATCH 1/3] util: add qemu_write_pidfile()
  2018-08-31 14:53 ` [Qemu-devel] [PATCH 1/3] " Marc-André Lureau
@ 2018-09-03  9:55   ` Daniel P. Berrangé
  0 siblings, 0 replies; 8+ messages in thread
From: Daniel P. Berrangé @ 2018-09-03  9:55 UTC (permalink / raw)
  To: Marc-André Lureau
  Cc: qemu-devel, Paolo Bonzini, qemu-block, Stefan Weil, Fam Zheng,
	Michael Roth

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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [Qemu-devel] [PATCH 0/3] util: add qemu_write_pidfile()
  2018-08-31 14:53 [Qemu-devel] [PATCH 0/3] util: add qemu_write_pidfile() Marc-André Lureau
                   ` (2 preceding siblings ...)
  2018-08-31 14:53 ` [Qemu-devel] [PATCH 3/3] RFC: delete PID file on exit Marc-André Lureau
@ 2018-09-11 13:14 ` Paolo Bonzini
  3 siblings, 0 replies; 8+ messages in thread
From: Paolo Bonzini @ 2018-09-11 13:14 UTC (permalink / raw)
  To: Marc-André Lureau, qemu-devel
  Cc: berrange, qemu-block, Stefan Weil, Fam Zheng, Michael Roth

On 31/08/2018 16:53, Marc-André Lureau wrote:
> 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
> 
> Marc-André Lureau (3):
>   util: add qemu_write_pidfile()
>   util: use fcntl() for qemu_write_pidfile() locking
>   RFC: 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                  | 15 +++++++--
>  8 files changed, 129 insertions(+), 132 deletions(-)
> 

Queued 1-2, thanks.

Paolo

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2018-09-11 13:14 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-08-31 14:53 [Qemu-devel] [PATCH 0/3] util: add qemu_write_pidfile() Marc-André Lureau
2018-08-31 14:53 ` [Qemu-devel] [PATCH 1/3] " Marc-André Lureau
2018-09-03  9:55   ` Daniel P. Berrangé
2018-08-31 14:53 ` [Qemu-devel] [PATCH 2/3] util: use fcntl() for qemu_write_pidfile() locking Marc-André Lureau
2018-08-31 14:53 ` [Qemu-devel] [PATCH 3/3] RFC: delete PID file on exit Marc-André Lureau
2018-08-31 16:29   ` Stefan Weil
2018-08-31 16:35     ` Marc-André Lureau
2018-09-11 13:14 ` [Qemu-devel] [PATCH 0/3] util: add qemu_write_pidfile() Paolo Bonzini

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.