[libvirt] [PATCH 0/5 v2] Some rearrangement & additions to pid file handling/locking
An update to: https://www.redhat.com/archives/libvir-list/2011-August/msg00349.html Changes in v2: - Fix all docs & code bugs from previous review
From: "Daniel P. Berrange" <berrange@redhat.com> Add some simple wrappers around the fcntl() discretionary file locking capability. * src/util/util.c, src/util/util.h, src/libvirt_private.syms: Add virFileLock and virFileUnlock APIs --- src/libvirt_private.syms | 2 + src/util/virfile.c | 83 ++++++++++++++++++++++++++++++++++++++++++++++ src/util/virfile.h | 3 ++ 3 files changed, 88 insertions(+), 0 deletions(-) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 830222b..261f3e0 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1039,6 +1039,7 @@ virFileFindMountPoint; virFileHasSuffix; virFileIsExecutable; virFileLinkPointsTo; +virFileLock; virFileMakePath; virFileMatchesNameSuffix; virFileOpenAs; @@ -1051,6 +1052,7 @@ virFileReadPidPath; virFileResolveLink; virFileSanitizePath; virFileStripSuffix; +virFileUnlock; virFileWaitForDevices; virFileWriteStr; virFindFileInPath; diff --git a/src/util/virfile.c b/src/util/virfile.c index 8b32518..0edf058 100644 --- a/src/util/virfile.c +++ b/src/util/virfile.c @@ -251,3 +251,86 @@ virFileDirectFdFree(virFileDirectFdPtr dfd) virCommandFree(dfd->cmd); VIR_FREE(dfd); } + + +#ifndef WIN32 +/** + * virFileLock: + * @fd: file descriptor to acquire the lock on + * @shared: type of lock to acquire + * @start: byte offset to start lock + * @len: length of lock (0 to acquire entire remaining file from @start) + * + * Attempt to acquire a lock on the file @fd. If @shared + * is true, then a shared lock will be acquired, + * otherwise an exclusive lock will be acquired. If + * the lock cannot be acquired, an error will be + * returned. This will not wait to acquire the lock if + * another process already holds it. + * + * The lock will be released when @fd is closed. The lock + * will also be released if *any* other open file descriptor + * pointing to the same underlying file is closed. As such + * this function should not be relied on in multi-threaded + * apps where other threads can be opening/closing arbitrary + * files. + * + * Returns 0 on success, or -errno otherwise + */ +int virFileLock(int fd, bool shared, off_t start, off_t len) +{ + struct flock fl = { + .l_type = shared ? F_RDLCK : F_WRLCK, + .l_whence = SEEK_SET, + .l_start = start, + .l_len = len, + }; + + if (fcntl(fd, F_SETLK, &fl) < 0) + return -errno; + + return 0; +} + + +/** + * virFileUnlock: + * @fd: file descriptor to release the lock on + * @start: byte offset to start unlock + * @len: length of lock (0 to release entire remaining file from @start) + * + * Release a lock previously acquired with virFileUnlock(). + * NB the lock will also be released if any open file descriptor + * pointing to the same file as @fd is closed + * + * Returns 0 on succcess, or -errno on error + */ +int virFileUnlock(int fd, off_t start, off_t len) +{ + struct flock fl = { + .l_type = F_UNLCK, + .l_whence = SEEK_SET, + .l_start = start, + .l_len = len, + }; + + if (fcntl(fd, F_SETLK, &fl) < 0) + return -errno; + + return 0; +} +#else +int virFileLock(int fd ATTRIBUTE_UNUSED, + bool shared ATTRIBUTE_UNUSED, + off_t start ATTRIBUTE_UNUSED, + off_t len ATTRIBUTE_UNUSED) +{ + return -ENOSYS; +} +int virFileUnlock(int fd ATTRIBUTE_UNUSED, + off_t start ATTRIBUTE_UNUSED, + off_t len ATTRIBUTE_UNUSED) +{ + return -ENOSYS; +} +#endif diff --git a/src/util/virfile.h b/src/util/virfile.h index 0906568..e025614 100644 --- a/src/util/virfile.h +++ b/src/util/virfile.h @@ -65,4 +65,7 @@ int virFileDirectFdClose(virFileDirectFdPtr dfd); void virFileDirectFdFree(virFileDirectFdPtr dfd); +int virFileLock(int fd, bool shared, off_t start, off_t len); +int virFileUnlock(int fd, off_t start, off_t len); + #endif /* __VIR_FILES_H */ -- 1.7.6
From: "Daniel P. Berrange" <berrange@redhat.com> The functions for manipulating pidfiles are in util/util.{c,h}. We will shortly be adding some further pidfile related functions. To avoid further growing util.c, this moves the pidfile related functions into a dedicated virpidfile.{c,h}. The functions are also all renamed to have 'virPidFile' as their name prefix * util/util.h, util/util.c: Remove all pidfile code * util/virpidfile.c, util/virpidfile.h: Add new APIs for pidfile handling. * lxc/lxc_controller.c, lxc/lxc_driver.c, network/bridge_driver.c, qemu/qemu_process.c: Add virpidfile.h include and adapt for API renames --- src/Makefile.am | 1 + src/libvirt_private.syms | 14 ++- src/lxc/lxc_controller.c | 5 +- src/lxc/lxc_driver.c | 5 +- src/network/bridge_driver.c | 19 ++-- src/qemu/qemu_process.c | 5 +- src/util/command.c | 3 +- src/util/util.c | 152 --------------------------------- src/util/util.h | 15 --- src/util/virpidfile.c | 199 +++++++++++++++++++++++++++++++++++++++++++ src/util/virpidfile.h | 50 +++++++++++ 11 files changed, 281 insertions(+), 187 deletions(-) create mode 100644 src/util/virpidfile.c create mode 100644 src/util/virpidfile.h diff --git a/src/Makefile.am b/src/Makefile.am index 009ff25..cf7c003 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -84,6 +84,7 @@ UTIL_SOURCES = \ util/util.c util/util.h \ util/viraudit.c util/viraudit.h \ util/virfile.c util/virfile.h \ + util/virpidfile.c util/virpidfile.h \ util/xml.c util/xml.h \ util/virterror.c util/virterror_internal.h \ util/virkeycode.c util/virkeycode.h \ diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 261f3e0..7a96c1e 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1033,7 +1033,6 @@ virEventAddHandle; virEventRemoveHandle; virFileAbsPath; virFileBuildPath; -virFileDeletePid; virFileExists; virFileFindMountPoint; virFileHasSuffix; @@ -1044,11 +1043,8 @@ virFileMakePath; virFileMatchesNameSuffix; virFileOpenAs; virFileOpenTty; -virFilePid; virFileReadAll; virFileReadLimFD; -virFileReadPid; -virFileReadPidPath; virFileResolveLink; virFileSanitizePath; virFileStripSuffix; @@ -1123,6 +1119,16 @@ virFileFclose; virFileFdopen; +# virpidfile.h +virPidFileBuildPath; +virPidFileRead; +virPidFileReadPath; +virPidFileWrite; +virPidFileWritePath; +virPidFileDelete; +virPidFileDeletePath; + + # virterror_internal.h virDispatchError; virErrorMsg; diff --git a/src/lxc/lxc_controller.c b/src/lxc/lxc_controller.c index 45b4c70..5028251 100644 --- a/src/lxc/lxc_controller.c +++ b/src/lxc/lxc_controller.c @@ -56,6 +56,7 @@ #include "memory.h" #include "util.h" #include "virfile.h" +#include "virpidfile.h" #define VIR_FROM_THIS VIR_FROM_LXC @@ -1136,7 +1137,7 @@ int main(int argc, char *argv[]) goto cleanup; if (pid > 0) { - if ((rc = virFileWritePid(LXC_STATE_DIR, name, pid)) < 0) { + if ((rc = virPidFileWrite(LXC_STATE_DIR, name, pid)) < 0) { virReportSystemError(-rc, _("Unable to write pid file '%s/%s.pid'"), LXC_STATE_DIR, name); @@ -1179,7 +1180,7 @@ int main(int argc, char *argv[]) cleanup: if (def) - virFileDeletePid(LXC_STATE_DIR, def->name); + virPidFileDelete(LXC_STATE_DIR, def->name); lxcControllerCleanupInterfaces(nveths, veths); if (sockpath) unlink(sockpath); diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c index 2d94309..bb560b6 100644 --- a/src/lxc/lxc_driver.c +++ b/src/lxc/lxc_driver.c @@ -50,6 +50,7 @@ #include "stats_linux.h" #include "hooks.h" #include "virfile.h" +#include "virpidfile.h" #include "fdstream.h" #include "domain_audit.h" #include "domain_nwfilter.h" @@ -1030,7 +1031,7 @@ static void lxcVmCleanup(lxc_driver_t *driver, virEventRemoveHandle(priv->monitorWatch); VIR_FORCE_CLOSE(priv->monitor); - virFileDeletePid(driver->stateDir, vm->def->name); + virPidFileDelete(driver->stateDir, vm->def->name); virDomainDeleteConfig(driver->stateDir, NULL, vm); virDomainObjSetState(vm, VIR_DOMAIN_SHUTOFF, reason); @@ -1612,7 +1613,7 @@ static int lxcVmStart(virConnectPtr conn, goto cleanup; /* And get its pid */ - if ((r = virFileReadPid(driver->stateDir, vm->def->name, &vm->pid)) < 0) { + if ((r = virPidFileRead(driver->stateDir, vm->def->name, &vm->pid)) < 0) { virReportSystemError(-r, _("Failed to read pid file %s/%s.pid"), driver->stateDir, vm->def->name); diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index c90db63..8b27320 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -49,6 +49,7 @@ #include "network_conf.h" #include "driver.h" #include "buf.h" +#include "virpidfile.h" #include "util.h" #include "command.h" #include "memory.h" @@ -218,7 +219,7 @@ networkFindActiveConfigs(struct network_driver *driver) { if (obj->def->ips && (obj->def->nips > 0)) { char *pidpath, *radvdpidbase; - if (virFileReadPid(NETWORK_PID_DIR, obj->def->name, + if (virPidFileRead(NETWORK_PID_DIR, obj->def->name, &obj->dnsmasqPid) == 0) { /* Check that it's still alive */ if (kill(obj->dnsmasqPid, 0) != 0) @@ -236,7 +237,7 @@ networkFindActiveConfigs(struct network_driver *driver) { virReportOOMError(); goto cleanup; } - if (virFileReadPid(NETWORK_PID_DIR, radvdpidbase, + if (virPidFileRead(NETWORK_PID_DIR, radvdpidbase, &obj->radvdPid) == 0) { /* Check that it's still alive */ if (kill(obj->radvdPid, 0) != 0) @@ -728,7 +729,7 @@ networkStartDhcpDaemon(virNetworkObjPtr network) goto cleanup; } - if (!(pidfile = virFilePid(NETWORK_PID_DIR, network->def->name))) { + if (!(pidfile = virPidFileBuildPath(NETWORK_PID_DIR, network->def->name))) { virReportOOMError(); goto cleanup; } @@ -765,7 +766,7 @@ networkStartDhcpDaemon(virNetworkObjPtr network) * pid */ - ret = virFileReadPid(NETWORK_PID_DIR, network->def->name, + ret = virPidFileRead(NETWORK_PID_DIR, network->def->name, &network->dnsmasqPid); if (ret < 0) goto cleanup; @@ -818,7 +819,7 @@ networkStartRadvd(virNetworkObjPtr network) virReportOOMError(); goto cleanup; } - if (!(pidfile = virFilePid(NETWORK_PID_DIR, radvdpidbase))) { + if (!(pidfile = virPidFileBuildPath(NETWORK_PID_DIR, radvdpidbase))) { virReportOOMError(); goto cleanup; } @@ -885,7 +886,7 @@ networkStartRadvd(virNetworkObjPtr network) * a dummy pidfile name - virCommand will create the pidfile we * want to use (this is necessary because radvd's internal * daemonization and pidfile creation causes a race, and the - * virFileReadPid() below will fail if we use them). + * virPidFileRead() below will fail if we use them). * Unfortunately, it isn't possible to tell radvd to not create * its own pidfile, so we just let it do so, with a slightly * different name. Unused, but harmless. @@ -901,7 +902,7 @@ networkStartRadvd(virNetworkObjPtr network) if (virCommandRun(cmd, NULL) < 0) goto cleanup; - if (virFileReadPid(NETWORK_PID_DIR, radvdpidbase, + if (virPidFileRead(NETWORK_PID_DIR, radvdpidbase, &network->radvdPid) < 0) goto cleanup; @@ -1919,7 +1920,7 @@ static int networkShutdownNetworkVirtual(struct network_driver *driver, if (!(radvdpidbase = networkRadvdPidfileBasename(network->def->name))) { virReportOOMError(); } else { - virFileDeletePid(NETWORK_PID_DIR, radvdpidbase); + virPidFileDelete(NETWORK_PID_DIR, radvdpidbase); VIR_FREE(radvdpidbase); } } @@ -2486,7 +2487,7 @@ static int networkUndefine(virNetworkPtr net) { virReportOOMError(); goto cleanup; } - virFileDeletePid(NETWORK_PID_DIR, radvdpidbase); + virPidFileDelete(NETWORK_PID_DIR, radvdpidbase); VIR_FREE(radvdpidbase); } diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 30c8b28..6f54b30 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -50,6 +50,7 @@ #include "memory.h" #include "hooks.h" #include "virfile.h" +#include "virpidfile.h" #include "util.h" #include "c-ctype.h" #include "nodeinfo.h" @@ -2771,7 +2772,7 @@ int qemuProcessStart(virConnectPtr conn, priv->gotShutdown = false; VIR_FREE(priv->pidfile); - if (!(priv->pidfile = virFilePid(driver->stateDir, vm->def->name))) { + if (!(priv->pidfile = virPidFileBuildPath(driver->stateDir, vm->def->name))) { virReportSystemError(errno, "%s", _("Failed to build pidfile path.")); goto cleanup; @@ -2880,7 +2881,7 @@ int qemuProcessStart(virConnectPtr conn, /* wait for qemu process to show up */ if (ret == 0) { - if (virFileReadPidPath(priv->pidfile, &vm->pid) < 0) { + if (virPidFileReadPath(priv->pidfile, &vm->pid) < 0) { qemuReportError(VIR_ERR_INTERNAL_ERROR, _("Domain %s didn't show up"), vm->def->name); ret = -1; diff --git a/src/util/command.c b/src/util/command.c index 26fcb28..d390478 100644 --- a/src/util/command.c +++ b/src/util/command.c @@ -39,6 +39,7 @@ #include "util.h" #include "logging.h" #include "virfile.h" +#include "virpidfile.h" #include "buf.h" #include "ignore-value.h" #include "verify.h" @@ -493,7 +494,7 @@ virExecWithHook(const char *const*argv, } if (pid > 0) { - if (pidfile && (virFileWritePidPath(pidfile,pid) < 0)) { + if (pidfile && (virPidFileWritePath(pidfile,pid) < 0)) { kill(pid, SIGTERM); usleep(500*1000); kill(pid, SIGTERM); diff --git a/src/util/util.c b/src/util/util.c index 2e2a6a0..e3b216f 100644 --- a/src/util/util.c +++ b/src/util/util.c @@ -1151,158 +1151,6 @@ int virFileOpenTtyAt(const char *ptmx ATTRIBUTE_UNUSED, } #endif -char* virFilePid(const char *dir, const char* name) -{ - char *pidfile; - if (virAsprintf(&pidfile, "%s/%s.pid", dir, name) < 0) - return NULL; - return pidfile; -} - -int virFileWritePid(const char *dir, - const char *name, - pid_t pid) -{ - int rc; - char *pidfile = NULL; - - if (name == NULL || dir == NULL) { - rc = -EINVAL; - goto cleanup; - } - - if (virFileMakePath(dir) < 0) { - rc = -errno; - goto cleanup; - } - - if (!(pidfile = virFilePid(dir, name))) { - rc = -ENOMEM; - goto cleanup; - } - - rc = virFileWritePidPath(pidfile, pid); - -cleanup: - VIR_FREE(pidfile); - return rc; -} - -int virFileWritePidPath(const char *pidfile, - pid_t pid) -{ - int rc; - int fd; - FILE *file = NULL; - - if ((fd = open(pidfile, - O_WRONLY | O_CREAT | O_TRUNC, - S_IRUSR | S_IWUSR)) < 0) { - rc = -errno; - goto cleanup; - } - - if (!(file = VIR_FDOPEN(fd, "w"))) { - rc = -errno; - VIR_FORCE_CLOSE(fd); - goto cleanup; - } - - if (fprintf(file, "%d", pid) < 0) { - rc = -errno; - goto cleanup; - } - - rc = 0; - -cleanup: - if (VIR_FCLOSE(file) < 0) - rc = -errno; - - return rc; -} - - -int virFileReadPidPath(const char *path, - pid_t *pid) -{ - FILE *file; - int rc; - - *pid = 0; - - if (!(file = fopen(path, "r"))) { - rc = -errno; - goto cleanup; - } - - if (fscanf(file, "%d", pid) != 1) { - rc = -EINVAL; - VIR_FORCE_FCLOSE(file); - goto cleanup; - } - - if (VIR_FCLOSE(file) < 0) { - rc = -errno; - goto cleanup; - } - - rc = 0; - - cleanup: - return rc; -} - - -int virFileReadPid(const char *dir, - const char *name, - pid_t *pid) -{ - int rc; - char *pidfile = NULL; - *pid = 0; - - if (name == NULL || dir == NULL) { - rc = -EINVAL; - goto cleanup; - } - - if (!(pidfile = virFilePid(dir, name))) { - rc = -ENOMEM; - goto cleanup; - } - - rc = virFileReadPidPath(pidfile, pid); - - cleanup: - VIR_FREE(pidfile); - return rc; -} - -int virFileDeletePid(const char *dir, - const char *name) -{ - int rc = 0; - char *pidfile = NULL; - - if (name == NULL || dir == NULL) { - rc = -EINVAL; - goto cleanup; - } - - if (!(pidfile = virFilePid(dir, name))) { - rc = -ENOMEM; - goto cleanup; - } - - if (unlink(pidfile) < 0 && errno != ENOENT) - rc = -errno; - -cleanup: - VIR_FREE(pidfile); - return rc; -} - /* * Creates an absolute path for a potentially relative path. diff --git a/src/util/util.h b/src/util/util.h index af8b15d..e76da9c 100644 --- a/src/util/util.h +++ b/src/util/util.h @@ -120,21 +120,6 @@ int virFileOpenTtyAt(const char *ptmx, char **ttyName, int rawmode); -char* virFilePid(const char *dir, - const char *name); - -int virFileWritePidPath(const char *path, - pid_t pid) ATTRIBUTE_RETURN_CHECK; -int virFileWritePid(const char *dir, - const char *name, - pid_t pid) ATTRIBUTE_RETURN_CHECK; -int virFileReadPidPath(const char *path, - pid_t *pid) ATTRIBUTE_RETURN_CHECK; -int virFileReadPid(const char *dir, - const char *name, - pid_t *pid) ATTRIBUTE_RETURN_CHECK; -int virFileDeletePid(const char *dir, - const char *name); char *virArgvToString(const char *const *argv); diff --git a/src/util/virpidfile.c b/src/util/virpidfile.c new file mode 100644 index 0000000..25c3272 --- /dev/null +++ b/src/util/virpidfile.c @@ -0,0 +1,199 @@ +/* + * virpidfile.c: manipulation of pidfiles + * + * Copyright (C) 2010-2011 Red Hat, Inc. + * Copyright (C) 2006, 2007 Binary Karma + * Copyright (C) 2006 Shuveb Hussain + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library; if not, write to the Free Software + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA + * + */ + +#include <config.h> + +#include <fcntl.h> + +#include "virpidfile.h" +#include "virfile.h" +#include "memory.h" +#include "util.h" + + +char *virPidFileBuildPath(const char *dir, const char* name) +{ + char *pidfile; + + if (virAsprintf(&pidfile, "%s/%s.pid", dir, name) < 0) + return NULL; + + return pidfile; +} + + +int virPidFileWritePath(const char *pidfile, + pid_t pid) +{ + int rc; + int fd; + FILE *file = NULL; + + if ((fd = open(pidfile, + O_WRONLY | O_CREAT | O_TRUNC, + S_IRUSR | S_IWUSR)) < 0) { + rc = -errno; + goto cleanup; + } + + if (!(file = VIR_FDOPEN(fd, "w"))) { + rc = -errno; + VIR_FORCE_CLOSE(fd); + goto cleanup; + } + + if (fprintf(file, "%d", pid) < 0) { + rc = -errno; + goto cleanup; + } + + rc = 0; + +cleanup: + if (VIR_FCLOSE(file) < 0) + rc = -errno; + + return rc; +} + + +int virPidFileWrite(const char *dir, + const char *name, + pid_t pid) +{ + int rc; + char *pidfile = NULL; + + if (name == NULL || dir == NULL) { + rc = -EINVAL; + goto cleanup; + } + + if (virFileMakePath(dir) < 0) { + rc = -errno; + goto cleanup; + } + + if (!(pidfile = virPidFileBuildPath(dir, name))) { + rc = -ENOMEM; + goto cleanup; + } + + rc = virPidFileWritePath(pidfile, pid); + +cleanup: + VIR_FREE(pidfile); + return rc; +} + + +int virPidFileReadPath(const char *path, + pid_t *pid) +{ + FILE *file; + int rc; + + *pid = 0; + + if (!(file = fopen(path, "r"))) { + rc = -errno; + goto cleanup; + } + + if (fscanf(file, "%d", pid) != 1) { + rc = -EINVAL; + VIR_FORCE_FCLOSE(file); + goto cleanup; + } + + if (VIR_FCLOSE(file) < 0) { + rc = -errno; + goto cleanup; + } + + rc = 0; + + cleanup: + return rc; +} + + +int virPidFileRead(const char *dir, + const char *name, + pid_t *pid) +{ + int rc; + char *pidfile = NULL; + *pid = 0; + + if (name == NULL || dir == NULL) { + rc = -EINVAL; + goto cleanup; + } + + if (!(pidfile = virPidFileBuildPath(dir, name))) { + rc = -ENOMEM; + goto cleanup; + } + + rc = virPidFileReadPath(pidfile, pid); + + cleanup: + VIR_FREE(pidfile); + return rc; +} + + +int virPidFileDeletePath(const char *pidfile) +{ + int rc = 0; + + if (unlink(pidfile) < 0 && errno != ENOENT) + rc = -errno; + + return rc; +} + + +int virPidFileDelete(const char *dir, + const char *name) +{ + int rc = 0; + char *pidfile = NULL; + + if (name == NULL || dir == NULL) { + rc = -EINVAL; + goto cleanup; + } + + if (!(pidfile = virPidFileBuildPath(dir, name))) { + rc = -ENOMEM; + goto cleanup; + } + + rc = virPidFileDeletePath(pidfile); + +cleanup: + VIR_FREE(pidfile); + return rc; +} diff --git a/src/util/virpidfile.h b/src/util/virpidfile.h new file mode 100644 index 0000000..e28a3c1 --- /dev/null +++ b/src/util/virpidfile.h @@ -0,0 +1,50 @@ +/* + * virpidfile.h: manipulation of pidfiles + * + * Copyright (C) 2010-2011 Red Hat, Inc. + * Copyright (C) 2006, 2007 Binary Karma + * Copyright (C) 2006 Shuveb Hussain + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library; if not, write to the Free Software + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA + * + */ + +#ifndef __VIR_PIDFILE_H__ +# define __VIR_PIDFILE_H__ + +# include <sys/types.h> +# include "internal.h" + +char *virPidFileBuildPath(const char *dir, + const char *name); + +int virPidFileWritePath(const char *path, + pid_t pid) ATTRIBUTE_RETURN_CHECK; +int virPidFileWrite(const char *dir, + const char *name, + pid_t pid) ATTRIBUTE_RETURN_CHECK; + +int virPidFileReadPath(const char *path, + pid_t *pid) ATTRIBUTE_RETURN_CHECK; +int virPidFileRead(const char *dir, + const char *name, + pid_t *pid) ATTRIBUTE_RETURN_CHECK; + +int virPidFileDeletePath(const char *path); +int virPidFileDelete(const char *dir, + const char *name); + + +#endif /* __VIR_PIDFILE_H__ */ -- 1.7.6
From: "Daniel P. Berrange" <berrange@redhat.com> In some cases the caller of virPidFileRead might like extra checks to determine whether the pid just read is really the one they are expecting. This adds virPidFileReadIfAlive which will check whether the pid is still alive with kill(0, -1), and (on linux only) will look at /proc/$PID/path * libvirt_private.syms, util/virpidfile.c, util/virpidfile.h: Add virPidFileReadIfValid and virPidFileReadPathIfValid * network/bridge_driver.c: Use new APIs to check PID validity --- src/libvirt_private.syms | 2 + src/network/bridge_driver.c | 35 +++------------- src/util/virpidfile.c | 94 +++++++++++++++++++++++++++++++++++++++++++ src/util/virpidfile.h | 8 ++++ 4 files changed, 110 insertions(+), 29 deletions(-) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 7a96c1e..fee3520 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1122,7 +1122,9 @@ virFileFdopen; # virpidfile.h virPidFileBuildPath; virPidFileRead; +virPidFileReadIfAlive; virPidFileReadPath; +virPidFileReadPathIfAlive; virPidFileWrite; virPidFileWritePath; virPidFileDelete; diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index 8b27320..cea8771 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -217,40 +217,17 @@ networkFindActiveConfigs(struct network_driver *driver) { /* Try and read dnsmasq/radvd pids if any */ if (obj->def->ips && (obj->def->nips > 0)) { - char *pidpath, *radvdpidbase; - - if (virPidFileRead(NETWORK_PID_DIR, obj->def->name, - &obj->dnsmasqPid) == 0) { - /* Check that it's still alive */ - if (kill(obj->dnsmasqPid, 0) != 0) - obj->dnsmasqPid = -1; - if (virAsprintf(&pidpath, "/proc/%d/exe", obj->dnsmasqPid) < 0) { - virReportOOMError(); - goto cleanup; - } - if (virFileLinkPointsTo(pidpath, DNSMASQ) == 0) - obj->dnsmasqPid = -1; - VIR_FREE(pidpath); - } + char *radvdpidbase; + + ignore_value(virPidFileReadIfAlive(NETWORK_PID_DIR, obj->def->name, + &obj->dnsmasqPid, DNSMASQ)); if (!(radvdpidbase = networkRadvdPidfileBasename(obj->def->name))) { virReportOOMError(); goto cleanup; } - if (virPidFileRead(NETWORK_PID_DIR, radvdpidbase, - &obj->radvdPid) == 0) { - /* Check that it's still alive */ - if (kill(obj->radvdPid, 0) != 0) - obj->radvdPid = -1; - if (virAsprintf(&pidpath, "/proc/%d/exe", obj->radvdPid) < 0) { - virReportOOMError(); - VIR_FREE(radvdpidbase); - goto cleanup; - } - if (virFileLinkPointsTo(pidpath, RADVD) == 0) - obj->radvdPid = -1; - VIR_FREE(pidpath); - } + ignore_value(virPidFileReadIfAlive(NETWORK_PID_DIR, radvdpidbase, + &obj->radvdPid, RADVD)); VIR_FREE(radvdpidbase); } } diff --git a/src/util/virpidfile.c b/src/util/virpidfile.c index 25c3272..c7adbfc 100644 --- a/src/util/virpidfile.c +++ b/src/util/virpidfile.c @@ -24,6 +24,7 @@ #include <config.h> #include <fcntl.h> +#include <signal.h> #include "virpidfile.h" #include "virfile.h" @@ -164,6 +165,99 @@ int virPidFileRead(const char *dir, } + +/** + * virPidFileReadPathIfAlive: + * @path: path to pidfile + * @pid: variable to return pid in + * @binpath: path of executable associated with the pidfile + * + * This will attempt to read a pid from @path, and store it + * in @pid. The @pid will only be set, however, if the + * pid in @path is running, and its executable path + * resolves to @binpath. This adds protection against + * recycling of previously reaped pids. + * + * Returns -errno upon error, or zero on successful + * reading of the pidfile. If the PID was not still + * alive, zero will be returned, but @pid will be + * set to -1. + */ +int virPidFileReadPathIfAlive(const char *path, + pid_t *pid, + const char *binpath) +{ + int rc; + char *procpath = NULL; + + rc = virPidFileReadPath(path, pid); + if (rc < 0) + return rc; + + /* Check that it's still alive */ + if (kill(*pid, 0) < 0) { + *pid = -1; + return 0; + } + + if (virAsprintf(&procpath, "/proc/%d/exe", *pid) < 0) { + *pid = -1; + return 0; + } +#ifdef __linux__ + if (virFileLinkPointsTo(procpath, binpath) == 0) + *pid = -1; +#endif + VIR_FREE(procpath); + + return 0; +} + + +/** + * virPidFileReadIfAlive: + * @dir: directory containing pidfile + * @name: base filename of pidfile + * @pid: variable to return pid in + * @binpath: path of executable associated with the pidfile + * + * This will attempt to read a pid from the pidfile @name + * in directory @dir, and store it in @pid. The @pid will + * only be set, however, if the pid in @name is running, + * and its executable path resolves to @binpath. This adds + * protection against recycling of previously reaped pids. + * + * Returns -errno upon error, or zero on successful + * reading of the pidfile. If the PID was not still + * alive, zero will be returned, but @pid will be + * set to -1. + */ +int virPidFileReadIfAlive(const char *dir, + const char *name, + pid_t *pid, + const char *binpath) +{ + int rc = 0; + char *pidfile = NULL; + + if (name == NULL || dir == NULL) { + rc = -EINVAL; + goto cleanup; + } + + if (!(pidfile = virPidFileBuildPath(dir, name))) { + rc = -ENOMEM; + goto cleanup; + } + + rc = virPidFileReadPathIfAlive(pidfile, pid, binpath); + +cleanup: + VIR_FREE(pidfile); + return rc; +} + + int virPidFileDeletePath(const char *pidfile) { int rc = 0; diff --git a/src/util/virpidfile.h b/src/util/virpidfile.h index e28a3c1..6659f1c 100644 --- a/src/util/virpidfile.h +++ b/src/util/virpidfile.h @@ -42,6 +42,14 @@ int virPidFileRead(const char *dir, const char *name, pid_t *pid) ATTRIBUTE_RETURN_CHECK; +int virPidFileReadPathIfAlive(const char *path, + pid_t *pid, + const char *binpath) ATTRIBUTE_RETURN_CHECK; +int virPidFileReadIfAlive(const char *dir, + const char *name, + pid_t *pid, + const char *binpath) ATTRIBUTE_RETURN_CHECK; + int virPidFileDeletePath(const char *path); int virPidFileDelete(const char *dir, const char *name); -- 1.7.6
On Fri, Aug 12, 2011 at 15:07:21 +0100, Daniel P. Berrange wrote:
From: "Daniel P. Berrange" <berrange@redhat.com>
In some cases the caller of virPidFileRead might like extra checks to determine whether the pid just read is really the one they are expecting. This adds virPidFileReadIfAlive which will check whether the pid is still alive with kill(0, -1), and (on linux only) will look at /proc/$PID/path ... diff --git a/src/util/virpidfile.c b/src/util/virpidfile.c index 25c3272..c7adbfc 100644 --- a/src/util/virpidfile.c +++ b/src/util/virpidfile.c ... +int virPidFileReadPathIfAlive(const char *path, + pid_t *pid, + const char *binpath) +{ + int rc; + char *procpath = NULL; + + rc = virPidFileReadPath(path, pid); + if (rc < 0) + return rc; + + /* Check that it's still alive */ + if (kill(*pid, 0) < 0) { + *pid = -1; + return 0; + } + + if (virAsprintf(&procpath, "/proc/%d/exe", *pid) < 0) { + *pid = -1; + return 0; + } +#ifdef __linux__ + if (virFileLinkPointsTo(procpath, binpath) == 0) + *pid = -1; +#endif + VIR_FREE(procpath); ^^^ three spaces here instead of four
Jirka
On 08/12/2011 08:07 AM, Daniel P. Berrange wrote:
From: "Daniel P. Berrange"<berrange@redhat.com>
In some cases the caller of virPidFileRead might like extra checks to determine whether the pid just read is really the one they are expecting. This adds virPidFileReadIfAlive which will check whether the pid is still alive with kill(0, -1), and (on linux only) will look at /proc/$PID/path
+ * Returns -errno upon error, or zero on successful + * reading of the pidfile. If the PID was not still + * alive, zero will be returned, but @pid will be + * set to -1. + */ +int virPidFileReadPathIfAlive(const char *path, + pid_t *pid, + const char *binpath) +{ + int rc; + char *procpath = NULL; + + rc = virPidFileReadPath(path, pid); + if (rc< 0) + return rc; + + /* Check that it's still alive */ + if (kill(*pid, 0)< 0) { + *pid = -1; + return 0; + }
This fails to compile on mingw, which lacks kill(). What's the best approach to fixing this, always fail with -ENOSYS, since right now we aren't making any use of pid files on mingw builds anyway? -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On Wed, Aug 17, 2011 at 11:34:34AM -0600, Eric Blake wrote:
On 08/12/2011 08:07 AM, Daniel P. Berrange wrote:
From: "Daniel P. Berrange"<berrange@redhat.com>
In some cases the caller of virPidFileRead might like extra checks to determine whether the pid just read is really the one they are expecting. This adds virPidFileReadIfAlive which will check whether the pid is still alive with kill(0, -1), and (on linux only) will look at /proc/$PID/path
+ * Returns -errno upon error, or zero on successful + * reading of the pidfile. If the PID was not still + * alive, zero will be returned, but @pid will be + * set to -1. + */ +int virPidFileReadPathIfAlive(const char *path, + pid_t *pid, + const char *binpath) +{ + int rc; + char *procpath = NULL; + + rc = virPidFileReadPath(path, pid); + if (rc< 0) + return rc; + + /* Check that it's still alive */ + if (kill(*pid, 0)< 0) { + *pid = -1; + return 0; + }
This fails to compile on mingw, which lacks kill(). What's the best approach to fixing this, always fail with -ENOSYS, since right now we aren't making any use of pid files on mingw builds anyway?
We already skip the /proc check on non-Linux. So I say we just skip the kill call on Win32. This stuff is only an extra sanity check upon loading the pidfile, so I think returning an error is too mean and we just skip the kill check, or write a win32 specific check for process ID. Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
Regression introduced in commit b7e5ca4. Mingw lacks kill(), but we were only using it for a sanity check; so we can go with one less check. Also, on OOM error, this function should outright fail rather than claim that the pid file was successfully read. * src/util/virpidfile.c (virPidFileReadPathIfAlive): Skip kill call where unsupported, and report error on OOM. ---
We already skip the /proc check on non-Linux. So I say we just skip the kill call on Win32. This stuff is only an extra sanity check Nupon loading the pidfile, so I think returning an error is too mean and we just skip the kill check
Done, and pushing under the build-breaker rule. src/util/virpidfile.c | 7 +++++-- 1 files changed, 5 insertions(+), 2 deletions(-) diff --git a/src/util/virpidfile.c b/src/util/virpidfile.c index e64b0b3..8206e1a 100644 --- a/src/util/virpidfile.c +++ b/src/util/virpidfile.c @@ -200,15 +200,18 @@ int virPidFileReadPathIfAlive(const char *path, if (rc < 0) return rc; - /* Check that it's still alive */ +#ifndef WIN32 + /* Check that it's still alive. Safe to skip this sanity check on + * mingw, which lacks kill(). */ if (kill(*pid, 0) < 0) { *pid = -1; return 0; } +#endif if (virAsprintf(&procpath, "/proc/%d/exe", *pid) < 0) { *pid = -1; - return 0; + return -1; } if (virFileIsLink(procpath) && -- 1.7.4.4
Regression introduced in commit 5d30db0. * src/rpc/virnetsocket.c (virNetSocketNewListenUNIX) [WIN32]: Use correct signature. --- Pushing under the build-breaker rule (thanks to ./autobuild.sh). src/rpc/virnetsocket.c | 1 + 1 files changed, 1 insertions(+), 0 deletions(-) diff --git a/src/rpc/virnetsocket.c b/src/rpc/virnetsocket.c index 82a11d8..275b70d 100644 --- a/src/rpc/virnetsocket.c +++ b/src/rpc/virnetsocket.c @@ -366,6 +366,7 @@ error: #else int virNetSocketNewListenUNIX(const char *path ATTRIBUTE_UNUSED, mode_t mask ATTRIBUTE_UNUSED, + uid_t user ATTRIBUTE_UNUSED, gid_t grp ATTRIBUTE_UNUSED, virNetSocketPtr *retsock ATTRIBUTE_UNUSED) { -- 1.7.4.4
On 08/17/2011 02:09 PM, Eric Blake wrote:
We already skip the /proc check on non-Linux. So I say we just skip the kill call on Win32. This stuff is only an extra sanity check Nupon loading the pidfile, so I think returning an error is too mean and we just skip the kill check Done, and pushing under the build-breaker rule.
src/util/virpidfile.c | 7 +++++-- 1 files changed, 5 insertions(+), 2 deletions(-)
diff --git a/src/util/virpidfile.c b/src/util/virpidfile.c index e64b0b3..8206e1a 100644 --- a/src/util/virpidfile.c +++ b/src/util/virpidfile.c @@ -200,15 +200,18 @@ int virPidFileReadPathIfAlive(const char *path, if (rc< 0) return rc;
- /* Check that it's still alive */ +#ifndef WIN32 + /* Check that it's still alive. Safe to skip this sanity check on + * mingw, which lacks kill(). */ if (kill(*pid, 0)< 0) { virKillProcess (src/util/util.c) handles WIN32 as well.
Stefan
On 08/17/2011 01:42 PM, Stefan Berger wrote:
- /* Check that it's still alive */ +#ifndef WIN32 + /* Check that it's still alive. Safe to skip this sanity check on + * mingw, which lacks kill(). */ if (kill(*pid, 0)< 0) { virKillProcess (src/util/util.c) handles WIN32 as well.
Good point - we should probably expand more existing clients of kill() to instead use virKillProcess. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
From: "Daniel P. Berrange" <berrange@redhat.com> In daemons using pidfiles to protect against concurrent execution there is a possibility that a crash may leave a stale pidfile on disk, which then prevents later restart of the daemon. To avoid this problem, introduce a pair of APIs which make use of virFileLock to ensure crash-safe & race condition-safe pidfile acquisition & releae * src/libvirt_private.syms, src/util/virpidfile.c, src/util/virpidfile.h: Add virPidFileAcquire and virPidFileRelease --- po/POTFILES.in | 1 + src/libvirt_private.syms | 4 + src/util/virpidfile.c | 156 ++++++++++++++++++++++++++++++++++++++++++++++ src/util/virpidfile.h | 12 ++++ 4 files changed, 173 insertions(+), 0 deletions(-) diff --git a/po/POTFILES.in b/po/POTFILES.in index 7bb08b0..b200f89 100644 --- a/po/POTFILES.in +++ b/po/POTFILES.in @@ -123,6 +123,7 @@ src/util/sysinfo.c src/util/util.c src/util/viraudit.c src/util/virfile.c +src/util/virpidfile.c src/util/virterror.c src/util/xml.c src/vbox/vbox_MSCOMGlue.c diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index fee3520..64b91ee 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1120,11 +1120,15 @@ virFileFdopen; # virpidfile.h +virPidFileAcquire; +virPidFileAcquirePath; virPidFileBuildPath; virPidFileRead; virPidFileReadIfAlive; virPidFileReadPath; virPidFileReadPathIfAlive; +virPidFileRelease; +virPidFileReleasePath; virPidFileWrite; virPidFileWritePath; virPidFileDelete; diff --git a/src/util/virpidfile.c b/src/util/virpidfile.c index c7adbfc..1a50aa2 100644 --- a/src/util/virpidfile.c +++ b/src/util/virpidfile.c @@ -25,13 +25,19 @@ #include <fcntl.h> #include <signal.h> +#include <sys/stat.h> #include "virpidfile.h" #include "virfile.h" #include "memory.h" #include "util.h" +#include "intprops.h" +#include "logging.h" +#include "virterror_internal.h" +#define VIR_FROM_THIS VIR_FROM_NONE + char *virPidFileBuildPath(const char *dir, const char* name) { char *pidfile; @@ -291,3 +297,153 @@ cleanup: VIR_FREE(pidfile); return rc; } + + + +int virPidFileAcquirePath(const char *path, + pid_t pid) +{ + int fd = -1; + char pidstr[INT_BUFSIZE_BOUND(pid)]; + verify(sizeof(pid_t) <= sizeof(unsigned int)); + + if (path[0] == '\0') + return 0; + + while (1) { + struct stat a, b; + if ((fd = open(path, O_WRONLY|O_CREAT, 0644)) < 0) { + virReportSystemError(errno, + _("Failed to open pid file '%s'"), + path); + return -1; + } + + if (virSetCloseExec(fd) < 0) { + virReportSystemError(errno, + _("Failed to set close-on-exec flag '%s'"), + path); + VIR_FORCE_CLOSE(fd); + return -1; + } + + if (fstat(fd, &b) < 0) { + virReportSystemError(errno, + _("Unable to check status of pid file '%s'"), + path); + VIR_FORCE_CLOSE(fd); + return -1; + } + + if (virFileLock(fd, false, 0, 1) < 0) { + virReportSystemError(errno, + _("Failed to acquire pid file '%s'"), + path); + VIR_FORCE_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) { + char ebuf[1024]; + VIR_DEBUG("Pid file '%s' disappeared: %s", + path, virStrerror(errno, ebuf, sizeof ebuf)); + VIR_FORCE_CLOSE(fd); + /* Someone else must be racing with us, so try agin */ + continue; + } + + if (a.st_ino == b.st_ino) + break; + + VIR_DEBUG("Pid file '%s' was recreated", path); + VIR_FORCE_CLOSE(fd); + /* Someone else must be racing with us, so try agin */ + } + + snprintf(pidstr, sizeof(pidstr), "%u", (unsigned int)pid); + + if (safewrite(fd, pidstr, strlen(pidstr)) < 0) { + virReportSystemError(errno, + _("Failed to write to pid file '%s'"), + path); + VIR_FORCE_CLOSE(fd); + } + + return fd; +} + + +int virPidFileAcquire(const char *dir, + const char *name, + pid_t pid) +{ + int rc = 0; + char *pidfile = NULL; + + if (name == NULL || dir == NULL) { + rc = -EINVAL; + goto cleanup; + } + + if (!(pidfile = virPidFileBuildPath(dir, name))) { + rc = -ENOMEM; + goto cleanup; + } + + rc = virPidFileAcquirePath(pidfile, pid); + +cleanup: + VIR_FREE(pidfile); + return rc; +} + + +int virPidFileReleasePath(const char *path, + int fd) +{ + int rc = 0; + /* + * We need to unlink before closing the FD to avoid + * a race, but Win32 won't let you unlink an open + * file handle. So on that platform we do the reverse + * and just have to live with the possible race. + */ +#ifdef WIN32 + VIR_FORCE_CLOSE(fd); + if (unlink(path) < 0 && errno != ENOENT) + rc = -errno; +#else + if (unlink(path) < 0 && errno != ENOENT) + rc = -errno; + VIR_FORCE_CLOSE(fd); +#endif + return rc; +} + + +int virPidFileRelease(const char *dir, + const char *name, + int fd) +{ + int rc = 0; + char *pidfile = NULL; + + if (name == NULL || dir == NULL) { + rc = -EINVAL; + goto cleanup; + } + + if (!(pidfile = virPidFileBuildPath(dir, name))) { + rc = -ENOMEM; + goto cleanup; + } + + rc = virPidFileReleasePath(pidfile, fd); + +cleanup: + VIR_FREE(pidfile); + return rc; +} diff --git a/src/util/virpidfile.h b/src/util/virpidfile.h index 6659f1c..dc44fc9 100644 --- a/src/util/virpidfile.h +++ b/src/util/virpidfile.h @@ -55,4 +55,16 @@ int virPidFileDelete(const char *dir, const char *name); +int virPidFileAcquirePath(const char *path, + pid_t pid) ATTRIBUTE_RETURN_CHECK; +int virPidFileAcquire(const char *dir, + const char *name, + pid_t pid) ATTRIBUTE_RETURN_CHECK; + +int virPidFileReleasePath(const char *path, + int fd); +int virPidFileRelease(const char *dir, + const char *name, + int fd); + #endif /* __VIR_PIDFILE_H__ */ -- 1.7.6
From: "Daniel P. Berrange" <berrange@redhat.com> Remove the current libvirtd pidfile handling code, in favour of calling out to the new APIs. This ensures libvirtd's pidfile handling is crashsafe This also means that the non-root libvirtd instances (for handling qemu:///session URIs) can now safely use pidfiles without racing * daemon/libvirtd.c: Switch to use virPidFileAcquire and virPidFileRelease --- daemon/libvirtd.c | 60 +++++++++------------------------------------------- 1 files changed, 11 insertions(+), 49 deletions(-) diff --git a/daemon/libvirtd.c b/daemon/libvirtd.c index 53f1002..b866a01 100644 --- a/daemon/libvirtd.c +++ b/daemon/libvirtd.c @@ -35,6 +35,7 @@ #include "libvirt_internal.h" #include "virterror_internal.h" #include "virfile.h" +#include "virpidfile.h" #define VIR_FROM_THIS VIR_FROM_QEMU @@ -259,44 +260,6 @@ static int daemonForkIntoBackground(const char *argv0) } } -static int daemonWritePidFile(const char *pidFile, const char *argv0) -{ - int fd; - FILE *fh; - char ebuf[1024]; - - if (pidFile[0] == '\0') - return 0; - - if ((fd = open(pidFile, O_WRONLY|O_CREAT|O_EXCL, 0644)) < 0) { - VIR_ERROR(_("Failed to open pid file '%s' : %s"), - pidFile, virStrerror(errno, ebuf, sizeof ebuf)); - return -1; - } - - if (!(fh = VIR_FDOPEN(fd, "w"))) { - VIR_ERROR(_("Failed to fdopen pid file '%s' : %s"), - pidFile, virStrerror(errno, ebuf, sizeof ebuf)); - VIR_FORCE_CLOSE(fd); - return -1; - } - - if (fprintf(fh, "%lu\n", (unsigned long)getpid()) < 0) { - VIR_ERROR(_("%s: Failed to write to pid file '%s' : %s"), - argv0, pidFile, virStrerror(errno, ebuf, sizeof ebuf)); - VIR_FORCE_FCLOSE(fh); - return -1; - } - - if (VIR_FCLOSE(fh) == EOF) { - VIR_ERROR(_("%s: Failed to close pid file '%s' : %s"), - argv0, pidFile, virStrerror(errno, ebuf, sizeof ebuf)); - return -1; - } - - return 0; -} - static int daemonPidFilePath(bool privileged, @@ -1261,6 +1224,7 @@ int main(int argc, char **argv) { char *remote_config_file = NULL; int statuswrite = -1; int ret = 1; + int pid_file_fd = -1; char *pid_file = NULL; char *sock_file = NULL; char *sock_file_ro = NULL; @@ -1378,7 +1342,7 @@ int main(int argc, char **argv) { if (daemonSetupLogging(config, privileged, verbose, godaemon) < 0) exit(EXIT_FAILURE); - if (!pid_file && privileged && + if (!pid_file && daemonPidFilePath(privileged, &pid_file) < 0) exit(EXIT_FAILURE); @@ -1405,14 +1369,6 @@ int main(int argc, char **argv) { } } - /* If we have a pidfile set, claim it now, exiting if already taken */ - if (pid_file != NULL && - daemonWritePidFile(pid_file, argv[0]) < 0) { - VIR_FREE(pid_file); /* Prevent unlinking of someone else's pid ! */ - ret = VIR_DAEMON_ERR_PIDFILE; - goto cleanup; - } - /* Ensure the rundir exists (on tmpfs on some systems) */ if (privileged) { const char *rundir = LOCALSTATEDIR "/run/libvirt"; @@ -1432,6 +1388,12 @@ int main(int argc, char **argv) { umask(old_umask); } + /* Try to claim the pidfile, exiting if we can't */ + if ((pid_file_fd = virPidFileAcquirePath(pid_file, getpid())) < 0) { + ret = VIR_DAEMON_ERR_PIDFILE; + goto cleanup; + } + use_polkit_dbus = config->auth_unix_rw == REMOTE_AUTH_POLKIT || config->auth_unix_ro == REMOTE_AUTH_POLKIT; if (!(srv = virNetServerNew(config->min_workers, @@ -1570,8 +1532,8 @@ cleanup: } VIR_FORCE_CLOSE(statuswrite); } - if (pid_file) - unlink (pid_file); + if (pid_file_fd != -1) + virPidFileReleasePath(pid_file, pid_file_fd); VIR_FREE(sock_file); VIR_FREE(sock_file_ro); -- 1.7.6
On Fri, Aug 12, 2011 at 15:07:18 +0100, Daniel P. Berrange wrote:
An update to:
https://www.redhat.com/archives/libvir-list/2011-August/msg00349.html
Changes in v2: - Fix all docs & code bugs from previous review
ACK series with the indentation nit in 3/5 fixed. Jirka
participants (4)
-
Daniel P. Berrange -
Eric Blake -
Jiri Denemark -
Stefan Berger