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@redhat.com>
Reviewed-by: Jonathon Jongsma <jjongsma@redhat.com>

diff --git a/src/bhyve/bhyve_process.c b/src/bhyve/bhyve_process.c
index 18002b559b59ad1052fefe492e74f2ff5eb118fc..d46786d393cfe3eef288d0402869666c78bb63b7 100644 (file)
--- 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 bb489a74e3f53b934e0e1b0df14b4044f6714efa..5ab526ba1bc35b3ebb44e9154f4f2bd2e81b866d 100644 (file)
--- 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 977082d5857867e96c38ea28339e58c8ef7f90c9..4247902bcf4a8537abda9364a360d7c4309434f5 100644 (file)
--- 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 6906db4af5042eaaaf744bc384f9bad04cc70304..e7e0f24443b5847795f5ae90e0fc5cb42f3f3a15 100644 (file)
--- 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 8072465615c8e0410aaca7a9b0329977b3efeb5f..72653254063bfb257bdc9c5e25a267573e2487ef 100644 (file)
--- 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 9722d8e1de66a538d7c13c18e6b435319d83fe82..d162812a741f93052e918387db974b6b55c5ec69 100644 (file)
--- 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 191c79e1e2de07433f874e4611be41c679b2d032..79ab7863551bb39fdc50bb85562eb2483e1f3508 100644 (file)
--- 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 31e60c73597c271725c36986d29875bd33c0fdc2..8ebf152d95ada3dc6d6e9713c6bd2d762e109288 100644 (file)
--- 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 c6d965d7209e542d00b37651cc6874a679d36ccb..1593ca79334370e2b09ee1d6cdc3420673486a04 100644 (file)
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -8233,7 +8233,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;
@@ -8963,7 +8963,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 924c625a4c62179ad0ab494fb2d2aa657925970d..4e2343b7d76c36e911d7af5dc39d4a6a13306592 100644 (file)
--- 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;