[libvirt] [PATCH 0/5] Some rearrangement & additions to pid file handling/locking
This series is a collection of changes related to pidfiles. First we define some wrappers around fcntl()'s lock file capability. Then all existing pidfile APIs from src/util/util.h are moved into a new file src/util/virpidfile.h. Next a new API for doing a sanity check on a PID (using /proc/$PID/exe) is introduced. Then two clever functions for doing race free, crash-safe acquisition & release of pidfiles are introduced. Finally libvirtd is converted to use the new pidfile acquisition API.
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..1346c1a 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 the 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 will @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
On Wed, Aug 10, 2011 at 16:37:24 +0100, Daniel P. Berrange wrote:
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..1346c1a 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 the a shared lock will be acquired, s/the // ^^^^^
+ * 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 will @fd is closed. The lock s/will/when/ I guess ^^^^
+ * 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 The documentation builder doesn't want ':' after "Returns" IIRC. Ah, but this is not a public API and documentation won't be generated from this block anyway so it doesn't really matter.
+ */ +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 */
ACK with nits fixed. Jirka
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
On Wed, Aug 10, 2011 at 16:37:25 +0100, Daniel P. Berrange wrote:
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
ACK Jirka
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 virPidFileReadIfValid 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 | 58 +++++++++++++++++++++++++++++++++++++++++++ src/util/virpidfile.h | 8 ++++++ 4 files changed, 74 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..dc92868 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,63 @@ int virPidFileRead(const char *dir, } +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; +} + + +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 Wed, Aug 10, 2011 at 16:37:26 +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 virPidFileReadIfValid 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
The commit message talks about *IfValid while the code implements *IfAlive. I think we should be consistent :-) ACK Jirka
Eh, I forgot to add some more notes... ...
diff --git a/src/util/virpidfile.c b/src/util/virpidfile.c index 25c3272..dc92868 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,63 @@ int virPidFileRead(const char *dir, }
+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);
Indentation. And anyway, what about implementing the second half of this function as follows: #ifdef __linux__ if (binpath) { char *procpath = NULL; if (virAsprintf(&procpath, "/proc/%d/exe", *pid) < 0 || !virFileLinkPointsTo(procpath, binpath)) *pid = -1; VIR_FREE(procpath); } #endif Building procpath makes little sense on non-linux and checking binpath first allows callers to use *IfAlive for checking pid liveliness without binpath checks (in case any caller wants to do so) and allows us to move procpath declaration into the same ifdef block :-)
+ + return 0; +}
I think we should also document that both *IfAlive functions return -errno if the pid file cannot be read or doesn't contain integer value. If it contains a number but such process is not running or doesn't correspond to binpath, the return value is 0 but pid is -1. Jirka
On Fri, Aug 12, 2011 at 02:10:00PM +0200, Jiri Denemark wrote:
Eh, I forgot to add some more notes...
...
diff --git a/src/util/virpidfile.c b/src/util/virpidfile.c index 25c3272..dc92868 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,63 @@ int virPidFileRead(const char *dir, }
+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);
Indentation.
What's wrong with indentation ??
And anyway, what about implementing the second half of this function as follows:
#ifdef __linux__ if (binpath) { char *procpath = NULL;
if (virAsprintf(&procpath, "/proc/%d/exe", *pid) < 0 || !virFileLinkPointsTo(procpath, binpath)) *pid = -1; VIR_FREE(procpath); } #endif
I didn't do that since then 'binpath' will generate an compile warning about unused parameters on non-linux. The extra alloc isn't really worth worrying about IMHO.
+ + return 0; +}
I think we should also document that both *IfAlive functions return -errno if the pid file cannot be read or doesn't contain integer value. If it contains a number but such process is not running or doesn't correspond to binpath, the return value is 0 but pid is -1.
Yep, 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 :|
On Fri, Aug 12, 2011 at 14:46:35 +0100, Daniel P. Berrange wrote:
On Fri, Aug 12, 2011 at 02:10:00PM +0200, Jiri Denemark wrote: ...
+ 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);
Indentation.
What's wrong with indentation ??
There are three (not four) spaces in front of VIR_FREE(). Jirka
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 | 142 ++++++++++++++++++++++++++++++++++++++++++++++ src/util/virpidfile.h | 12 ++++ 4 files changed, 159 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 dc92868..005c95d 100644 --- a/src/util/virpidfile.c +++ b/src/util/virpidfile.c @@ -25,11 +25,15 @@ #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" char *virPidFileBuildPath(const char *dir, const char* name) @@ -255,3 +259,141 @@ cleanup: VIR_FREE(pidfile); return rc; } + + + +int virPidFileAcquirePath(const char *path, + pid_t pid) +{ + int fd = -1; + char ebuf[1024]; + 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) { + VIR_ERROR(_("Failed to open pid file '%s' : %s"), + path, virStrerror(errno, ebuf, sizeof ebuf)); + return -1; + } + + if (fstat(fd, &b) < 0) { + VIR_ERROR(_("Unable to check status of pid file '%s': %s"), + path, virStrerror(errno, ebuf, sizeof ebuf)); + VIR_FORCE_CLOSE(fd); + return -1; + } + + if (virFileLock(fd, false, 0, 1) < 0) { + VIR_ERROR(_("Failed to acquire pid file '%s' : %s"), + path, virStrerror(errno, ebuf, sizeof ebuf)); + 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) { + 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) { + VIR_ERROR(_("Failed to write to pid file '%s' : %s"), + path, virStrerror(errno, ebuf, sizeof ebuf)); + 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
On Wed, Aug 10, 2011 at 16:37:27 +0100, Daniel P. Berrange wrote:
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 | 142 ++++++++++++++++++++++++++++++++++++++++++++++ src/util/virpidfile.h | 12 ++++ 4 files changed, 159 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 dc92868..005c95d 100644 --- a/src/util/virpidfile.c +++ b/src/util/virpidfile.c @@ -25,11 +25,15 @@
#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"
char *virPidFileBuildPath(const char *dir, const char* name) @@ -255,3 +259,141 @@ cleanup: VIR_FREE(pidfile); return rc; } + + + +int virPidFileAcquirePath(const char *path, + pid_t pid) +{ + int fd = -1; + char ebuf[1024]; + 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) { + VIR_ERROR(_("Failed to open pid file '%s' : %s"), + path, virStrerror(errno, ebuf, sizeof ebuf));
Hmm, why not virReportSystemError(errno, ...) everywhere instead of VIR_ERROR and virStrerror? Also I think we should call virSetCloseExec on the file descriptor for a little bit of extra safety. ... ACK with the comments addressed. Jirka
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 Wed, Aug 10, 2011 at 16:37:28 +0100, Daniel P. Berrange wrote:
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(-)
ACK Jirka
On Wed, Aug 10, 2011 at 04:37:23PM +0100, Daniel P. Berrange wrote:
This series is a collection of changes related to pidfiles. First we define some wrappers around fcntl()'s lock file capability. Then all existing pidfile APIs from src/util/util.h are moved into a new file src/util/virpidfile.h. Next a new API for doing a sanity check on a PID (using /proc/$PID/exe) is introduced. Then two clever functions for doing race free, crash-safe acquisition & release of pidfiles are introduced. Finally libvirtd is converted to use the new pidfile acquisition API.
Any takers.....most of the series is easy to review... 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 :|
participants (2)
-
Daniel P. Berrange -
Jiri Denemark