From b7d2d4af2bd5813f227c0a0557ccd2a53b95f49a Mon Sep 17 00:00:00 2001 From: Michal Privoznik Date: Thu, 6 Oct 2016 16:54:41 +0200 Subject: [PATCH] src: Treat PID as signed This initially started as a fix of some debug printing in virCgroupDetect. However it turned out that other places suffer from the similar problem. While dealing with pids, esp. in cases where we cannot use pid_t for ABI stability reasons, we often chose an unsigned integer type. This makes no sense as pid_t is signed. Also, new syntax-check rule is introduced so we won't repeat this mistake. Signed-off-by: Michal Privoznik --- cfg.mk | 8 ++++++++ src/locking/lock_driver_sanlock.c | 4 ++-- src/lxc/lxc_controller.c | 4 ++-- src/lxc/lxc_domain.c | 8 ++++---- src/lxc/lxc_driver.c | 4 ++-- src/lxc/lxc_monitor.c | 3 +-- src/lxc/lxc_process.c | 8 ++++---- src/qemu/qemu_process.c | 12 ++++++------ src/util/vircgroup.c | 24 ++++++++++++------------ src/util/viridentity.c | 2 +- src/util/virprocess.c | 14 ++++++-------- src/util/virsystemd.c | 8 ++++---- src/util/virutil.c | 4 ++-- 13 files changed, 54 insertions(+), 49 deletions(-) diff --git a/cfg.mk b/cfg.mk index cc17ec0cf0..03099df2e0 100644 --- a/cfg.mk +++ b/cfg.mk @@ -580,6 +580,11 @@ sc_prohibit_int_assign_bool: halt='use bool type for boolean values' \ $(_sc_search_regexp) +sc_prohibit_unsigned_pid: + @prohibit='\ [^,=;(]+pid' \ + halt='use signed type for pid values' \ + $(_sc_search_regexp) + # Many of the function names below came from this filter: # git grep -B2 '\<_('|grep -E '\.c- *[[:alpha:]_][[:alnum:]_]* ?\(.*[,;]$' \ # |sed 's/.*\.c- *//'|perl -pe 's/ ?\(.*//'|sort -u \ @@ -1213,6 +1218,9 @@ exclude_file_name_regexp--sc_prohibit_include_public_headers_brackets = \ exclude_file_name_regexp--sc_prohibit_int_ijk = \ ^(src/remote_protocol-structs|src/remote/remote_protocol\.x|cfg\.mk|include/libvirt/libvirt.+|src/admin_protocol-structs|src/admin/admin_protocol\.x)$$ +exclude_file_name_regexp--sc_prohibit_unsigned_pid = \ + ^(include/libvirt/.*\.h|src/(qemu/qemu_driver\.c|driver-hypervisor\.h|libvirt(-[a-z]*)?\.c|.*\.x|util/vir(polkit|systemd)\.c)|tests/virpolkittest\.c|tools/virsh-domain\.c)$$ + exclude_file_name_regexp--sc_prohibit_getenv = \ ^tests/.*\.[ch]$$ diff --git a/src/locking/lock_driver_sanlock.c b/src/locking/lock_driver_sanlock.c index 332c2de96a..280219f72b 100644 --- a/src/locking/lock_driver_sanlock.c +++ b/src/locking/lock_driver_sanlock.c @@ -87,7 +87,7 @@ struct _virLockManagerSanlockPrivate { char *vm_name; unsigned char vm_uuid[VIR_UUID_BUFLEN]; unsigned int vm_id; - unsigned int vm_pid; + int vm_pid; unsigned int flags; bool hasRWDisks; int res_count; @@ -494,7 +494,7 @@ static int virLockManagerSanlockNew(virLockManagerPtr lock, if (VIR_STRDUP(priv->vm_name, param->value.str) < 0) goto error; } else if (STREQ(param->key, "pid")) { - priv->vm_pid = param->value.ui; + priv->vm_pid = param->value.iv; } else if (STREQ(param->key, "id")) { priv->vm_id = param->value.ui; } else if (STREQ(param->key, "uri")) { diff --git a/src/lxc/lxc_controller.c b/src/lxc/lxc_controller.c index 8c581df3df..508bc3e6c4 100644 --- a/src/lxc/lxc_controller.c +++ b/src/lxc/lxc_controller.c @@ -1022,7 +1022,7 @@ static void virLXCControllerSignalChildIO(virNetDaemonPtr dmn, int status; ret = waitpid(-1, &status, WNOHANG); - VIR_DEBUG("Got sig child %d vs %lld", ret, (unsigned long long)ctrl->initpid); + VIR_DEBUG("Got sig child %d vs %lld", ret, (long long) ctrl->initpid); if (ret == ctrl->initpid) { virNetDaemonQuit(dmn); virMutexLock(&lock); @@ -2328,7 +2328,7 @@ virLXCControllerEventSendInit(virLXCControllerPtr ctrl, { virLXCMonitorInitEventMsg msg; - VIR_DEBUG("Init pid %llu", (unsigned long long)initpid); + VIR_DEBUG("Init pid %lld", (long long) initpid); memset(&msg, 0, sizeof(msg)); msg.initpid = initpid; diff --git a/src/lxc/lxc_domain.c b/src/lxc/lxc_domain.c index 9027c25640..3a7404f407 100644 --- a/src/lxc/lxc_domain.c +++ b/src/lxc/lxc_domain.c @@ -328,8 +328,8 @@ virLXCDomainObjPrivateXMLFormat(virBufferPtr buf, { virLXCDomainObjPrivatePtr priv = vm->privateData; - virBufferAsprintf(buf, "\n", - (unsigned long long)priv->initpid); + virBufferAsprintf(buf, "\n", + (long long) priv->initpid); return 0; } @@ -340,9 +340,9 @@ virLXCDomainObjPrivateXMLParse(xmlXPathContextPtr ctxt, virDomainDefParserConfigPtr config ATTRIBUTE_UNUSED) { virLXCDomainObjPrivatePtr priv = vm->privateData; - unsigned long long thepid; + long long thepid; - if (virXPathULongLong("string(./init[1]/@pid)", ctxt, &thepid) < 0) { + if (virXPathLongLong("string(./init[1]/@pid)", ctxt, &thepid) < 0) { VIR_WARN("Failed to load init pid from state %s", virGetLastErrorMessage()); priv->initpid = 0; diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c index cf30a6638b..3137018a73 100644 --- a/src/lxc/lxc_driver.c +++ b/src/lxc/lxc_driver.c @@ -3438,7 +3438,7 @@ lxcDomainShutdownFlags(virDomainPtr dom, errno != ESRCH) { virReportSystemError(errno, _("Unable to send SIGTERM to init pid %llu"), - (unsigned long long)priv->initpid); + (long long) priv->initpid); goto endjob; } } @@ -3521,7 +3521,7 @@ lxcDomainReboot(virDomainPtr dom, errno != ESRCH) { virReportSystemError(errno, _("Unable to send SIGTERM to init pid %llu"), - (unsigned long long)priv->initpid); + (long long) priv->initpid); goto endjob; } } diff --git a/src/lxc/lxc_monitor.c b/src/lxc/lxc_monitor.c index 57f40987e4..d828d528a6 100644 --- a/src/lxc/lxc_monitor.c +++ b/src/lxc/lxc_monitor.c @@ -105,8 +105,7 @@ virLXCMonitorHandleEventInit(virNetClientProgramPtr prog ATTRIBUTE_UNUSED, virLXCMonitorPtr mon = opaque; virLXCMonitorInitEventMsg *msg = evdata; - VIR_DEBUG("Event init %llu", - (unsigned long long)msg->initpid); + VIR_DEBUG("Event init %lld", (long long) msg->initpid); if (mon->cb.initNotify) mon->cb.initNotify(mon, (pid_t)msg->initpid, mon->vm); } diff --git a/src/lxc/lxc_process.c b/src/lxc/lxc_process.c index bce6a2f70e..07da3d3d06 100644 --- a/src/lxc/lxc_process.c +++ b/src/lxc/lxc_process.c @@ -740,8 +740,8 @@ virLXCProcessGetNsInode(pid_t pid, struct stat sb; int ret = -1; - if (virAsprintf(&path, "/proc/%llu/ns/%s", - (unsigned long long)pid, nsname) < 0) + if (virAsprintf(&path, "/proc/%lld/ns/%s", + (long long) pid, nsname) < 0) goto cleanup; if (stat(path, &sb) < 0) { @@ -776,8 +776,8 @@ static void virLXCProcessMonitorInitNotify(virLXCMonitorPtr mon ATTRIBUTE_UNUSED priv->initpid = initpid; if (virLXCProcessGetNsInode(initpid, "pid", &inode) < 0) { - VIR_WARN("Cannot obtain pid NS inode for %llu: %s", - (unsigned long long)initpid, + VIR_WARN("Cannot obtain pid NS inode for %lld: %s", + (long long) initpid, virGetLastErrorMessage()); virResetLastError(); } diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index bdf8800305..85795345d1 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -5444,8 +5444,8 @@ qemuProcessLaunch(virConnectPtr conn, _("Domain %s didn't show up"), vm->def->name); rv = -1; } - VIR_DEBUG("QEMU vm=%p name=%s running with pid=%llu", - vm, vm->def->name, (unsigned long long)vm->pid); + VIR_DEBUG("QEMU vm=%p name=%s running with pid=%lld", + vm, vm->def->name, (long long) vm->pid); } else { VIR_DEBUG("QEMU vm=%p name=%s failed to spawn", vm, vm->def->name); @@ -5836,9 +5836,9 @@ qemuProcessKill(virDomainObjPtr vm, unsigned int flags) { int ret; - VIR_DEBUG("vm=%p name=%s pid=%llu flags=%x", + VIR_DEBUG("vm=%p name=%s pid=%lld flags=%x", vm, vm->def->name, - (unsigned long long)vm->pid, flags); + (long long) vm->pid, flags); if (!(flags & VIR_QEMU_PROCESS_KILL_NOCHECK)) { if (!virDomainObjIsActive(vm)) { @@ -5916,10 +5916,10 @@ void qemuProcessStop(virQEMUDriverPtr driver, char *timestamp; virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); - VIR_DEBUG("Shutting down vm=%p name=%s id=%d pid=%llu, " + VIR_DEBUG("Shutting down vm=%p name=%s id=%d pid=%lld, " "reason=%s, asyncJob=%s, flags=%x", vm, vm->def->name, vm->def->id, - (unsigned long long)vm->pid, + (long long) vm->pid, virDomainShutoffReasonTypeToString(reason), qemuDomainAsyncJobTypeToString(asyncJob), flags); diff --git a/src/util/vircgroup.c b/src/util/vircgroup.c index 8b52816adf..24917e7150 100644 --- a/src/util/vircgroup.c +++ b/src/util/vircgroup.c @@ -548,13 +548,13 @@ virCgroupDetectPlacement(virCgroupPtr group, char *procfile; VIR_DEBUG("Detecting placement for pid %lld path %s", - (unsigned long long)pid, path); + (long long) pid, path); if (pid == -1) { if (VIR_STRDUP(procfile, "/proc/self/cgroup") < 0) goto cleanup; } else { - if (virAsprintf(&procfile, "/proc/%llu/cgroup", - (unsigned long long)pid) < 0) + if (virAsprintf(&procfile, "/proc/%lld/cgroup", + (long long) pid) < 0) goto cleanup; } @@ -732,11 +732,12 @@ virCgroupDetect(virCgroupPtr group, return -1; } - VIR_DEBUG("Detected mount/mapping %zu:%s at %s in %s for pid %llu", i, + VIR_DEBUG("Detected mount/mapping %zu:%s at %s in %s for pid %lld", + i, virCgroupControllerTypeToString(i), group->controllers[i].mountPoint, group->controllers[i].placement, - (unsigned long long)pid); + (long long) pid); } return 0; @@ -1235,8 +1236,7 @@ virCgroupAddTaskController(virCgroupPtr group, pid_t pid, int controller) return -1; } - return virCgroupSetValueU64(group, controller, "tasks", - (unsigned long long)pid); + return virCgroupSetValueI64(group, controller, "tasks", pid); } @@ -3506,8 +3506,8 @@ virCgroupKillInternal(virCgroupPtr group, int signum, virHashTablePtr pids) goto cleanup; } else { while (!feof(fp)) { - unsigned long pid_value; - if (fscanf(fp, "%lu", &pid_value) != 1) { + long pid_value; + if (fscanf(fp, "%ld", &pid_value) != 1) { if (feof(fp)) break; virReportSystemError(errno, @@ -3518,12 +3518,12 @@ virCgroupKillInternal(virCgroupPtr group, int signum, virHashTablePtr pids) if (virHashLookup(pids, (void*)pid_value)) continue; - VIR_DEBUG("pid=%lu", pid_value); + VIR_DEBUG("pid=%ld", pid_value); /* Cgroups is a Linux concept, so this cast is safe. */ if (kill((pid_t)pid_value, signum) < 0) { if (errno != ESRCH) { virReportSystemError(errno, - _("Failed to kill process %lu"), + _("Failed to kill process %ld"), pid_value); goto cleanup; } @@ -3553,7 +3553,7 @@ virCgroupKillInternal(virCgroupPtr group, int signum, virHashTablePtr pids) static uint32_t virCgroupPidCode(const void *name, uint32_t seed) { - unsigned long pid_value = (unsigned long)(intptr_t)name; + long pid_value = (long)(intptr_t)name; return virHashCodeGen(&pid_value, sizeof(pid_value), seed); } diff --git a/src/util/viridentity.c b/src/util/viridentity.c index 9b8ba4ae24..52a0a30a45 100644 --- a/src/util/viridentity.c +++ b/src/util/viridentity.c @@ -505,7 +505,7 @@ int virIdentitySetUNIXProcessID(virIdentityPtr ident, { char *val; int ret; - if (virAsprintf(&val, "%llu", (unsigned long long)pid) < 0) + if (virAsprintf(&val, "%lld", (long long) pid) < 0) return -1; ret = virIdentitySetAttr(ident, VIR_IDENTITY_ATTR_UNIX_PROCESS_ID, diff --git a/src/util/virprocess.c b/src/util/virprocess.c index 8b9f9c5d60..718c4a2664 100644 --- a/src/util/virprocess.c +++ b/src/util/virprocess.c @@ -608,8 +608,7 @@ int virProcessGetPids(pid_t pid, size_t *npids, pid_t **pids) *npids = 0; *pids = NULL; - if (virAsprintf(&taskPath, "/proc/%llu/task", - (unsigned long long)pid) < 0) + if (virAsprintf(&taskPath, "/proc/%llu/task", (long long) pid) < 0) goto cleanup; if (virDirOpen(&dir, taskPath) < 0) @@ -657,7 +656,7 @@ int virProcessGetNamespaces(pid_t pid, int fd; if (virAsprintf(&nsfile, "/proc/%llu/ns/%s", - (unsigned long long)pid, + (long long) pid, ns[i]) < 0) goto cleanup; @@ -968,8 +967,7 @@ int virProcessGetStartTime(pid_t pid, int len; char **tokens = NULL; - if (virAsprintf(&filename, "/proc/%llu/stat", - (unsigned long long)pid) < 0) + if (virAsprintf(&filename, "/proc/%llu/stat", (long long) pid) < 0) return -1; if ((len = virFileReadAll(filename, 1024, &buf)) < 0) @@ -1051,8 +1049,8 @@ int virProcessGetStartTime(pid_t pid, { static int warned; if (virAtomicIntInc(&warned) == 1) { - VIR_WARN("Process start time of pid %llu not available on this platform", - (unsigned long long)pid); + VIR_WARN("Process start time of pid %lld not available on this platform", + (long long) pid); } *timestamp = 0; return 0; @@ -1069,7 +1067,7 @@ static int virProcessNamespaceHelper(int errfd, int fd = -1; int ret = -1; - if (virAsprintf(&path, "/proc/%llu/ns/mnt", (unsigned long long)pid) < 0) + if (virAsprintf(&path, "/proc/%lld/ns/mnt", (long long) pid) < 0) goto cleanup; if ((fd = open(path, O_RDONLY)) < 0) { diff --git a/src/util/virsystemd.c b/src/util/virsystemd.c index 604dcdd0ad..0219db6ec4 100644 --- a/src/util/virsystemd.c +++ b/src/util/virsystemd.c @@ -210,8 +210,8 @@ virSystemdGetMachineNameByPID(pid_t pid) if (virDBusMessageRead(reply, "o", &object) < 0) goto cleanup; - VIR_DEBUG("Domain with pid %llu has object path '%s'", - (unsigned long long)pid, object); + VIR_DEBUG("Domain with pid %lld has object path '%s'", + (long long) pid, object); if (virDBusCallMethod(conn, &reply, NULL, "org.freedesktop.machine1", @@ -226,8 +226,8 @@ virSystemdGetMachineNameByPID(pid_t pid) if (virDBusMessageRead(reply, "v", "s", &name) < 0) goto cleanup; - VIR_DEBUG("Domain with pid %llu has machine name '%s'", - (unsigned long long)pid, name); + VIR_DEBUG("Domain with pid %lld has machine name '%s'", + (long long) pid, name); cleanup: VIR_FREE(object); diff --git a/src/util/virutil.c b/src/util/virutil.c index b57a1956c4..2d46b7ee3b 100644 --- a/src/util/virutil.c +++ b/src/util/virutil.c @@ -2582,8 +2582,8 @@ virGetListenFDs(void) } if ((pid_t)procid != getpid()) { - VIR_DEBUG("LISTEN_PID %s is not for us %llu", - pidstr, (unsigned long long)getpid()); + VIR_DEBUG("LISTEN_PID %s is not for us %lld", + pidstr, (long long) getpid()); return 0; } -- 2.39.5