| Message ID | 20180713130916.4153-21-marcandre.lureau@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | vhost-user for input & GPU | expand |
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 <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 | 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. > + close(pidfd); > + return false; > +} > + > void *qemu_oom_check(void *ptr) > { > if (ptr == NULL) { Regards, Daniel
Hi On Tue, Aug 28, 2018 at 5:53 PM Daniel P. Berrangé <berrange@redhat.com> 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 <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 | 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 :| >
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 <marcandre.lureau@redhat.com> > 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
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 537cc0e162..cc50692098 100644 --- a/qga/main.c +++ b/qga/main.c @@ -341,46 +341,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); @@ -480,8 +440,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; } @@ -516,8 +479,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 1528a712a0..cf6d360652 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 { @@ -1076,8 +1043,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..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; +} + void *qemu_oom_check(void *ptr) { if (ptr == NULL) { diff --git a/util/oslib-win32.c b/util/oslib-win32.c index bb5ad28bd3..66d05c88e6 100644 --- a/util/oslib-win32.c +++ b/util/oslib-win32.c @@ -767,3 +767,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), "%d\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 5272b4939f..8a9d3457ec 100644 --- a/vl.c +++ b/vl.c @@ -3993,8 +3993,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); }
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 <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 | 33 ++++++++++++++++++++++++++ util/oslib-win32.c | 27 ++++++++++++++++++++++ vl.c | 4 ++-- 8 files changed, 79 insertions(+), 131 deletions(-)