[PATCH] lib: Be consistent about vm->pid
Jonathon Jongsma
jjongsma at redhat.com
Tue May 31 19:26:53 UTC 2022
Reviewed-by: Jonathon Jongsma <jjongsma at redhat.com>
On 5/26/22 5:42 AM, Michal Privoznik wrote:
> 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 at redhat.com>
> ---
> 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;
More information about the libvir-list
mailing list