Re: [PATCH] lib: Be consistent about vm->pid

The virDomainObj struct has @pid member where the domain's hypervisor PID is stored (e.g. QEMU/bhyve/libvirt_lxc/... PID). However, we are not consistent when it comes to shutoff state. Initially, because virDomainObjNew() uses g_new0() the @pid is initialized to 0. But when domain is shut off, some functions set it to -1 (virBhyveProcessStop, virCHProcessStop, qemuProcessStop, ..). In other places, the @pid is tested to be 0, on some other places it's tested for being negative and in the rest for being positive. To solve this inconsistency we can stick with either value, -1 or 0. I've chosen the latter as it's safer IMO. For instance if by mistake we'd kill(vm->pid, SIGTERM) we would kill ourselves instead of init's process group. Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; --- src/bhyve/bhyve_process.c | 8 ++++---- src/ch/ch_domain.c | 2 +- src/ch/ch_process.c | 2 +- src/conf/domain_conf.h | 2 +- src/hypervisor/domain_cgroup.c | 2 +- src/lxc/lxc_process.c | 6 +++--- src/openvz/openvz_conf.c | 2 +- src/qemu/qemu_domain.c | 2 +- src/qemu/qemu_process.c | 4 ++-- tests/qemusecuritytest.c | 1 - 10 files changed, 15 insertions(+), 16 deletions(-) diff --git a/src/bhyve/bhyve_process.c b/src/bhyve/bhyve_process.c index 18002b559b..d46786d393 100644 --- a/src/bhyve/bhyve_process.c +++ b/src/bhyve/bhyve_process.c @@ -293,7 +293,7 @@ virBhyveProcessStop(struct _bhyveConn *driver, return 0; } - if (vm->pid <= 0) { + if (vm->pid == 0) { virReportError(VIR_ERR_INTERNAL_ERROR, _("Invalid PID %d for VM"), (int)vm->pid); @@ -329,7 +329,7 @@ virBhyveProcessStop(struct _bhyveConn *driver, bhyveProcessAutoDestroy); virDomainObjSetState(vm, VIR_DOMAIN_SHUTOFF, reason); - vm->pid = -1; + vm->pid = 0; vm->def->id = -1; bhyveProcessStopHook(vm, VIR_HOOK_BHYVE_OP_RELEASE); @@ -344,7 +344,7 @@ virBhyveProcessStop(struct _bhyveConn *driver, int virBhyveProcessShutdown(virDomainObj *vm) { - if (vm->pid <= 0) { + if (vm->pid == 0) { virReportError(VIR_ERR_INTERNAL_ERROR, _("Invalid PID %d for VM"), (int)vm->pid); @@ -433,7 +433,7 @@ virBhyveProcessReconnect(virDomainObj *vm, if (!virDomainObjIsActive(vm)) return 0; - if (!vm->pid) + if (vm->pid == 0) return 0; virObjectLock(vm); diff --git a/src/ch/ch_domain.c b/src/ch/ch_domain.c index bb489a74e3..5ab526ba1b 100644 --- a/src/ch/ch_domain.c +++ b/src/ch/ch_domain.c @@ -405,7 +405,7 @@ virCHDomainGetMachineName(virDomainObj *vm) virCHDriver *driver = priv->driver; char *ret = NULL; - if (vm->pid > 0) { + if (vm->pid != 0) { ret = virSystemdGetMachineNameByPID(vm->pid); if (!ret) virResetLastError(); diff --git a/src/ch/ch_process.c b/src/ch/ch_process.c index 977082d585..4247902bcf 100644 --- a/src/ch/ch_process.c +++ b/src/ch/ch_process.c @@ -574,7 +574,7 @@ virCHProcessStop(virCHDriver *driver G_GNUC_UNUSED, vm->def->name); } - vm->pid = -1; + vm->pid = 0; vm->def->id = -1; g_clear_pointer(&priv->machineName, g_free); diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 6906db4af5..e7e0f24443 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -3059,7 +3059,7 @@ struct _virDomainObj { virObjectLockable parent; virCond cond; - pid_t pid; + pid_t pid; /* 0 for no PID, avoid negative values like -1 */ virDomainStateReason state; unsigned int autostart : 1; diff --git a/src/hypervisor/domain_cgroup.c b/src/hypervisor/domain_cgroup.c index 8072465615..7265325406 100644 --- a/src/hypervisor/domain_cgroup.c +++ b/src/hypervisor/domain_cgroup.c @@ -517,7 +517,7 @@ virDomainCgroupSetupCgroup(const char *prefix, bool privileged, char *machineName) { - if (!vm->pid) { + if (vm->pid == 0) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Cannot setup cgroups until process is started")); return -1; diff --git a/src/lxc/lxc_process.c b/src/lxc/lxc_process.c index 9722d8e1de..d162812a74 100644 --- a/src/lxc/lxc_process.c +++ b/src/lxc/lxc_process.c @@ -217,7 +217,7 @@ static void virLXCProcessCleanup(virLXCDriver *driver, lxcProcessRemoveDomainStatus(cfg, vm); virDomainObjSetState(vm, VIR_DOMAIN_SHUTOFF, reason); - vm->pid = -1; + vm->pid = 0; vm->def->id = -1; if (!!g_atomic_int_dec_and_test(&driver->nactive) && driver->inhibitCallback) @@ -892,7 +892,7 @@ int virLXCProcessStop(virLXCDriver *driver, _("Some processes refused to die")); return -1; } - } else if (vm->pid > 0) { + } else if (vm->pid != 0) { /* If cgroup doesn't exist, just try cleaning up the * libvirt_lxc process */ if (virProcessKillPainfully(vm->pid, true) < 0) { @@ -1033,7 +1033,7 @@ virLXCProcessReadLogOutputData(virDomainObj *vm, bool isdead = false; char *eol; - if (vm->pid <= 0 || + if (vm->pid == 0 || (kill(vm->pid, 0) == -1 && errno == ESRCH)) isdead = true; diff --git a/src/openvz/openvz_conf.c b/src/openvz/openvz_conf.c index 191c79e1e2..79ab786355 100644 --- a/src/openvz/openvz_conf.c +++ b/src/openvz/openvz_conf.c @@ -528,7 +528,7 @@ int openvzLoadDomains(struct openvz_driver *driver) if (STREQ(status, "stopped")) { virDomainObjSetState(dom, VIR_DOMAIN_SHUTOFF, VIR_DOMAIN_SHUTOFF_UNKNOWN); - dom->pid = -1; + dom->pid = 0; } else { virDomainObjSetState(dom, VIR_DOMAIN_RUNNING, VIR_DOMAIN_RUNNING_UNKNOWN); diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 31e60c7359..8ebf152d95 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -10741,7 +10741,7 @@ qemuDomainGetMachineName(virDomainObj *vm) virQEMUDriver *driver = priv->driver; char *ret = NULL; - if (vm->pid > 0) { + if (vm->pid != 0) { ret = virSystemdGetMachineNameByPID(vm->pid); if (!ret) virResetLastError(); diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 07acb1c427..cbcc480089 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -8239,7 +8239,7 @@ void qemuProcessStop(virQEMUDriver *driver, g_clear_pointer(&vm->deprecations, g_free); vm->ndeprecations = 0; vm->taint = 0; - vm->pid = -1; + vm->pid = 0; virDomainObjSetState(vm, VIR_DOMAIN_SHUTOFF, reason); for (i = 0; i < vm->def->niothreadids; i++) vm->def->iothreadids[i]->thread_id = 0; @@ -8969,7 +8969,7 @@ qemuProcessReconnectHelper(virDomainObj *obj, g_autofree char *name = NULL; /* If the VM was inactive, we don't need to reconnect */ - if (!obj->pid) + if (obj->pid == 0) return 0; data = g_new0(struct qemuProcessReconnectData, 1); diff --git a/tests/qemusecuritytest.c b/tests/qemusecuritytest.c index 924c625a4c..4e2343b7d7 100644 --- a/tests/qemusecuritytest.c +++ b/tests/qemusecuritytest.c @@ -54,7 +54,6 @@ prepareObjects(virQEMUDriver *driver, if (!(vm = virDomainObjNew(driver->xmlopt))) return -1; - vm->pid = -1; priv = vm->privateData; priv->chardevStdioLogd = false; priv->rememberOwner = true;