[libvirt] [PATCH v2 0/3] Fix a couple of recently found coverity issues
v1: http://www.redhat.com/archives/libvir-list/2016-May/msg01611.html Although initially ACK'd by Erik, Jan pointed out something with one of the patches and well this just lagged on my todo list. Since then a change was made to lxc_driver.c (commit id '306b3a850') which caused a merge conflict. So rather than push without checking first, I figure I'll make a v2. Patch 1 was already ACK'd Patch 2 is new to remove persistentAddrs from qemu Patch 3 is altered... I think the return from virDomainObjListRemove will cause the 'vm' to be NULL anyway; however, since the next called API virDomainObjEndAPI checks for that - it seems that making that call won't hurt. I also moved the check to after endjob, so that some sort of "failure" and goto cleanup doesn't unexpectedly remove a transient domain. John Ferlan (3): qemu: Remove dead code qemu: Remove unused persistentAddrs lxc: Fix lxcDomainDestroyFlags endjob processing src/lxc/lxc_driver.c | 6 ++---- src/qemu/qemu_domain.h | 1 - src/qemu/qemu_domain_address.c | 17 ++++------------- src/qemu/qemu_process.c | 18 ++++++++---------- 4 files changed, 14 insertions(+), 28 deletions(-) -- 2.5.5
Since commit id '20a0fa8e' removed the QEMU_CAPS_DEVICE, Coverity notes that it's no longer possible to have 'addrs' be NULL when checking for a live domain since qemuDomainPCIAddressSetCreate would have jumped to cleanup if addrs was NULL. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_domain_address.c | 14 +++++--------- 1 file changed, 5 insertions(+), 9 deletions(-) diff --git a/src/qemu/qemu_domain_address.c b/src/qemu/qemu_domain_address.c index 7bd8ee5..95c11fa 100644 --- a/src/qemu/qemu_domain_address.c +++ b/src/qemu/qemu_domain_address.c @@ -1614,15 +1614,11 @@ qemuDomainAssignPCIAddresses(virDomainDefPtr def, if (obj && obj->privateData) { priv = obj->privateData; - if (addrs) { - /* if this is the live domain object, we persist the PCI addresses*/ - virDomainPCIAddressSetFree(priv->pciaddrs); - priv->persistentAddrs = 1; - priv->pciaddrs = addrs; - addrs = NULL; - } else { - priv->persistentAddrs = 0; - } + /* if this is the live domain object, we persist the PCI addresses*/ + virDomainPCIAddressSetFree(priv->pciaddrs); + priv->persistentAddrs = 1; + priv->pciaddrs = addrs; + addrs = NULL; } ret = 0; -- 2.5.5
On 25.05.2016 00:53, John Ferlan wrote:
Since commit id '20a0fa8e' removed the QEMU_CAPS_DEVICE, Coverity notes that it's no longer possible to have 'addrs' be NULL when checking for a live domain since qemuDomainPCIAddressSetCreate would have jumped to cleanup if addrs was NULL.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_domain_address.c | 14 +++++--------- 1 file changed, 5 insertions(+), 9 deletions(-)
diff --git a/src/qemu/qemu_domain_address.c b/src/qemu/qemu_domain_address.c index 7bd8ee5..95c11fa 100644 --- a/src/qemu/qemu_domain_address.c +++ b/src/qemu/qemu_domain_address.c @@ -1614,15 +1614,11 @@ qemuDomainAssignPCIAddresses(virDomainDefPtr def,
if (obj && obj->privateData) { priv = obj->privateData; - if (addrs) { - /* if this is the live domain object, we persist the PCI addresses*/ - virDomainPCIAddressSetFree(priv->pciaddrs); - priv->persistentAddrs = 1; - priv->pciaddrs = addrs; - addrs = NULL; - } else { - priv->persistentAddrs = 0; - } + /* if this is the live domain object, we persist the PCI addresses*/
While touching this code, can you please help my CDO [*] and insert a space at the end of the comment?
+ virDomainPCIAddressSetFree(priv->pciaddrs); + priv->persistentAddrs = 1; + priv->pciaddrs = addrs; + addrs = NULL; }
ret = 0;
* CDO is something like OCD, except letters are in alphabetical order. Like they should be. Michal
Based on some digital archaeology performed by jtomko, it's been determined that the persistentAddrs variable is no longer necessary... The variable was added by: commit 141dea6bc7222107c2357acb68066baea5b26df3 CommitDate: 2010-02-12 17:25:52 +0000 Add persistence of PCI addresses to QEMU Where it was set to 0 on domain startup if qemu did not support the QEMUD_CMD_FLAG_DEVICE capability, to clear the addresses at shutdown, because QEMU might make up different ones next time. As of commit f5dd58a6088cfc6e8bd354b693d399807a8ec395 CommitDate: 2012-07-11 11:19:05 +0200 qemu: Extended qemuDomainAssignAddresses to be callable from everywhere. this was broken, when the persistentAddrs = 0 assignment was moved inside qemuDomainAssignPCIAddresses and while it pretends to check for !QEMU_CAPS_DEVICE, its parent qemuDomainAssignAddresses is only called if QEMU_CAPS_DEVICE is present. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_domain.h | 1 - src/qemu/qemu_domain_address.c | 5 ----- src/qemu/qemu_process.c | 18 ++++++++---------- 3 files changed, 8 insertions(+), 16 deletions(-) diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h index f99f0bb..8176bf5 100644 --- a/src/qemu/qemu_domain.h +++ b/src/qemu/qemu_domain.h @@ -190,7 +190,6 @@ struct _qemuDomainObjPrivate { virDomainPCIAddressSetPtr pciaddrs; virDomainCCWAddressSetPtr ccwaddrs; virDomainVirtioSerialAddrSetPtr vioserialaddrs; - int persistentAddrs; virQEMUCapsPtr qemuCaps; char *lockState; diff --git a/src/qemu/qemu_domain_address.c b/src/qemu/qemu_domain_address.c index 95c11fa..6e166b9 100644 --- a/src/qemu/qemu_domain_address.c +++ b/src/qemu/qemu_domain_address.c @@ -149,7 +149,6 @@ qemuDomainAssignVirtioSerialAddresses(virDomainDefPtr def, priv = obj->privateData; /* if this is the live domain object, we persist the addresses */ virDomainVirtioSerialAddrSetFree(priv->vioserialaddrs); - priv->persistentAddrs = 1; priv->vioserialaddrs = addrs; addrs = NULL; } @@ -382,11 +381,8 @@ qemuDomainAssignS390Addresses(virDomainDefPtr def, if (addrs) { /* if this is the live domain object, we persist the CCW addresses*/ virDomainCCWAddressSetFree(priv->ccwaddrs); - priv->persistentAddrs = 1; priv->ccwaddrs = addrs; addrs = NULL; - } else { - priv->persistentAddrs = 0; } } ret = 0; @@ -1616,7 +1612,6 @@ qemuDomainAssignPCIAddresses(virDomainDefPtr def, priv = obj->privateData; /* if this is the live domain object, we persist the PCI addresses*/ virDomainPCIAddressSetFree(priv->pciaddrs); - priv->persistentAddrs = 1; priv->pciaddrs = addrs; addrs = NULL; } diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index b2669c0..0baa34a 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -5754,16 +5754,14 @@ void qemuProcessStop(virQEMUDriverPtr driver, priv->qemuDevices = NULL; virDomainDefClearDeviceAliases(vm->def); - if (!priv->persistentAddrs) { - virDomainDefClearPCIAddresses(vm->def); - virDomainPCIAddressSetFree(priv->pciaddrs); - priv->pciaddrs = NULL; - virDomainDefClearCCWAddresses(vm->def); - virDomainCCWAddressSetFree(priv->ccwaddrs); - priv->ccwaddrs = NULL; - virDomainVirtioSerialAddrSetFree(priv->vioserialaddrs); - priv->vioserialaddrs = NULL; - } + virDomainDefClearPCIAddresses(vm->def); + virDomainPCIAddressSetFree(priv->pciaddrs); + priv->pciaddrs = NULL; + virDomainDefClearCCWAddresses(vm->def); + virDomainCCWAddressSetFree(priv->ccwaddrs); + priv->ccwaddrs = NULL; + virDomainVirtioSerialAddrSetFree(priv->vioserialaddrs); + priv->vioserialaddrs = NULL; qemuHostdevReAttachDomainDevices(driver, vm->def); -- 2.5.5
On Tue, May 24, 2016 at 06:53:17PM -0400, John Ferlan wrote:
Based on some digital archaeology performed by jtomko, it's been determined that the persistentAddrs variable is no longer necessary...
The variable was added by: commit 141dea6bc7222107c2357acb68066baea5b26df3 CommitDate: 2010-02-12 17:25:52 +0000 Add persistence of PCI addresses to QEMU
Where it was set to 0 on domain startup if qemu did not support the QEMUD_CMD_FLAG_DEVICE capability, to clear the addresses at shutdown, because QEMU might make up different ones next time.
As of commit f5dd58a6088cfc6e8bd354b693d399807a8ec395 CommitDate: 2012-07-11 11:19:05 +0200 qemu: Extended qemuDomainAssignAddresses to be callable from everywhere.
this was broken, when the persistentAddrs = 0 assignment was moved inside qemuDomainAssignPCIAddresses and while it pretends to check for !QEMU_CAPS_DEVICE, its parent qemuDomainAssignAddresses is only called if QEMU_CAPS_DEVICE is present.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_domain.h | 1 - src/qemu/qemu_domain_address.c | 5 ----- src/qemu/qemu_process.c | 18 ++++++++---------- 3 files changed, 8 insertions(+), 16 deletions(-)
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index b2669c0..0baa34a 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -5754,16 +5754,14 @@ void qemuProcessStop(virQEMUDriverPtr driver, priv->qemuDevices = NULL;
virDomainDefClearDeviceAliases(vm->def); - if (!priv->persistentAddrs) {
persistentAddrs is always 1 here, this code can be dropped completely Jan
- virDomainDefClearPCIAddresses(vm->def); - virDomainPCIAddressSetFree(priv->pciaddrs); - priv->pciaddrs = NULL; - virDomainDefClearCCWAddresses(vm->def); - virDomainCCWAddressSetFree(priv->ccwaddrs); - priv->ccwaddrs = NULL; - virDomainVirtioSerialAddrSetFree(priv->vioserialaddrs); - priv->vioserialaddrs = NULL; - } + virDomainDefClearPCIAddresses(vm->def); + virDomainPCIAddressSetFree(priv->pciaddrs); + priv->pciaddrs = NULL; + virDomainDefClearCCWAddresses(vm->def); + virDomainCCWAddressSetFree(priv->ccwaddrs); + priv->ccwaddrs = NULL; + virDomainVirtioSerialAddrSetFree(priv->vioserialaddrs); + priv->vioserialaddrs = NULL;
qemuHostdevReAttachDomainDevices(driver, vm->def);
-- 2.5.5
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Commit id '15ccb0dbf' added job functions for the lxc driver; however, for shutdown and nonpersistent path, the vm was removed from the domain object list and the vm pointer cleared before the endjob. Adjust the code to perform the endjob first and then perform the ObjListRemove as long as the vm wasn't NULL. This follows more closely models from qemu and libxl Found by Coverity (FORWARD_NULL) Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/lxc/lxc_driver.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c index d690886..9e03f1f 100644 --- a/src/lxc/lxc_driver.c +++ b/src/lxc/lxc_driver.c @@ -1541,13 +1541,11 @@ lxcDomainDestroyFlags(virDomainPtr dom, VIR_DOMAIN_EVENT_STOPPED_DESTROYED); priv->doneStopEvent = true; virDomainAuditStop(vm, "destroyed"); - if (!vm->persistent) { - virDomainObjListRemove(driver->domains, vm); - vm = NULL; - } endjob: virLXCDomainObjEndJob(driver, vm); + if (!vm->persistent) + virDomainObjListRemove(driver->domains, vm); cleanup: virDomainObjEndAPI(&vm); -- 2.5.5
On 25.05.2016 00:53, John Ferlan wrote:
Commit id '15ccb0dbf' added job functions for the lxc driver; however, for shutdown and nonpersistent path, the vm was removed from the domain object list and the vm pointer cleared before the endjob.
Adjust the code to perform the endjob first and then perform the ObjListRemove as long as the vm wasn't NULL. This follows more closely models from qemu and libxl
Found by Coverity (FORWARD_NULL)
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/lxc/lxc_driver.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c index d690886..9e03f1f 100644 --- a/src/lxc/lxc_driver.c +++ b/src/lxc/lxc_driver.c @@ -1541,13 +1541,11 @@ lxcDomainDestroyFlags(virDomainPtr dom, VIR_DOMAIN_EVENT_STOPPED_DESTROYED); priv->doneStopEvent = true; virDomainAuditStop(vm, "destroyed"); - if (!vm->persistent) { - virDomainObjListRemove(driver->domains, vm); - vm = NULL; - }
endjob: virLXCDomainObjEndJob(driver, vm); + if (!vm->persistent) + virDomainObjListRemove(driver->domains, vm);
cleanup: virDomainObjEndAPI(&vm);
Okay, I guess this makes sense. I had to think twice when I saw this patch. But if a transient inactive domain is destroyed, it should not had been in the list in the first place. Michal
On 25.05.2016 00:53, John Ferlan wrote:
v1: http://www.redhat.com/archives/libvir-list/2016-May/msg01611.html
Although initially ACK'd by Erik, Jan pointed out something with one of the patches and well this just lagged on my todo list. Since then a change was made to lxc_driver.c (commit id '306b3a850') which caused a merge conflict.
So rather than push without checking first, I figure I'll make a v2.
Patch 1 was already ACK'd Patch 2 is new to remove persistentAddrs from qemu Patch 3 is altered... I think the return from virDomainObjListRemove will cause the 'vm' to be NULL anyway; however, since the next called API virDomainObjEndAPI checks for that - it seems that making that call won't hurt. I also moved the check to after endjob, so that some sort of "failure" and goto cleanup doesn't unexpectedly remove a transient domain.
John Ferlan (3): qemu: Remove dead code qemu: Remove unused persistentAddrs lxc: Fix lxcDomainDestroyFlags endjob processing
src/lxc/lxc_driver.c | 6 ++---- src/qemu/qemu_domain.h | 1 - src/qemu/qemu_domain_address.c | 17 ++++------------- src/qemu/qemu_process.c | 18 ++++++++---------- 4 files changed, 14 insertions(+), 28 deletions(-)
ACK series. Michal
participants (3)
-
John Ferlan -
Ján Tomko -
Michal Privoznik