3:54 p.m.
I almost didn't want to do this due to the sheer volume, but figured
at the very least the bulk of these are resource leaks found by the
much pickier new coverity scanner.
After this there are "only" 70 issues found...
John Ferlan (19):
libxl_migration: Resolve Coverity NULL_RETURNS
daemon: Resolve Coverity NEGATIVE_RETURNS
domain_conf: Resolve Coverity RESOURCE_LEAK
cpu_x86: Resolve Coverity RESOURCE_LEAK
qemu_command: Resolve Coverity RESOURCE_LEAK
qemu_agent: Resolve Coverity RESOURCE_LEAK
libxl_domain: Resolve Coverity RESOURCE_LEAK
qemu_capabilities: Resolve Coverity RESOURCE_LEAK
network_conf: Resolve Coverity RESOURCE_LEAK
virsh-network: Resolve Coverity RESOURCE_LEAK
bridge_driver: Resolve Coverity RESOURCE_LEAK
libxl_migration: Resolve Coverity RESOURCE_LEAK
phyp_driver: Resolve Coverity RESOURCE_LEAK
qemu_driver: Resolve Coverity RESOURCE_LEAK
storage_conf: Resolve Coverity RESOURCE_LEAK
qemu_monitor: Resolve Coverity NESTING_INDENT_MISMATCH
domain_conf: Resolve Coverity DEADCODE
qemu_driver: Resolve Coverity DEADCODE
qemu_command: Resolve Coverity DEADCODE
daemon/remote.c | 24 ++++++++++++------------
src/conf/domain_conf.c | 28 ++++++++++++++++++++++++----
src/conf/network_conf.c | 2 ++
src/conf/storage_conf.c | 2 ++
src/cpu/cpu_x86.c | 15 ++++++++++-----
src/libxl/libxl_domain.c | 4 +++-
src/libxl/libxl_migration.c | 11 +++++++++--
src/network/bridge_driver.c | 1 +
src/phyp/phyp_driver.c | 1 +
src/qemu/qemu_agent.c | 6 ++++--
src/qemu/qemu_capabilities.c | 2 +-
src/qemu/qemu_command.c | 9 +++++----
src/qemu/qemu_driver.c | 8 ++++++++
src/qemu/qemu_monitor.c | 3 ++-
tools/virsh-network.c | 2 +-
15 files changed, 85 insertions(+), 33 deletions(-)
--
1.9.3
3:54 p.m.
New subject: [libvirt] [PATCH 01/19] libxl_migration: Resolve Coverity NULL_RETURNS
Coverity noted that all callers to libxlDomainEventQueue() could ensure
the second parameter (event) was true before calling except this case.
As I look at the code and how events are used - it seems that prior to
generating an event for the dom == NULL condition, the resume/suspend
event should be queue'd after the virDomainSaveStatus() call which will
goto cleanup and queue the saved event anyway.
Signed-off-by: John Ferlan <jferlan(a)redhat.com>
---
src/libxl/libxl_migration.c | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/src/libxl/libxl_migration.c b/src/libxl/libxl_migration.c
index dbb5a8f..53ae63a 100644
--- a/src/libxl/libxl_migration.c
+++ b/src/libxl/libxl_migration.c
@@ -512,6 +512,11 @@ libxlDomainMigrationFinish(virConnectPtr dconn,
if (virDomainSaveStatus(driver->xmlopt, cfg->stateDir, vm) < 0)
goto cleanup;
+ if (event) {
+ libxlDomainEventQueue(driver, event);
+ event = NULL;
+ }
+
dom = virGetDomain(dconn, vm->def->name, vm->def->uuid);
if (dom == NULL) {
@@ -519,7 +524,6 @@ libxlDomainMigrationFinish(virConnectPtr dconn,
libxlDomainCleanup(driver, vm, VIR_DOMAIN_SHUTOFF_FAILED);
event = virDomainEventLifecycleNewFromObj(vm, VIR_DOMAIN_EVENT_STOPPED,
VIR_DOMAIN_EVENT_STOPPED_FAILED);
- libxlDomainEventQueue(driver, event);
}
cleanup:
--
1.9.3
4:29 p.m.
New subject: [libvirt] [PATCH 01/19] libxl_migration: Resolve Coverity NULL_RETURNS
John Ferlan wrote:
Coverity noted that all callers to libxlDomainEventQueue() could
ensure
the second parameter (event) was true before calling except this case.
As I look at the code and how events are used - it seems that prior to
generating an event for the dom == NULL condition, the resume/suspend
event should be queue'd after the virDomainSaveStatus() call which will
goto cleanup and queue the saved event anyway.
Signed-off-by: John Ferlan <jferlan(a)redhat.com>
---
src/libxl/libxl_migration.c | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/src/libxl/libxl_migration.c b/src/libxl/libxl_migration.c
index dbb5a8f..53ae63a 100644
--- a/src/libxl/libxl_migration.c
+++ b/src/libxl/libxl_migration.c
@@ -512,6 +512,11 @@ libxlDomainMigrationFinish(virConnectPtr dconn,
if (virDomainSaveStatus(driver->xmlopt, cfg->stateDir, vm) < 0)
goto cleanup;
+ if (event) {
+ libxlDomainEventQueue(driver, event);
+ event = NULL;
+ }
+
dom = virGetDomain(dconn, vm->def->name, vm->def->uuid);
if (dom == NULL) {
@@ -519,7 +524,6 @@ libxlDomainMigrationFinish(virConnectPtr dconn,
libxlDomainCleanup(driver, vm, VIR_DOMAIN_SHUTOFF_FAILED);
event = virDomainEventLifecycleNewFromObj(vm, VIR_DOMAIN_EVENT_STOPPED,
VIR_DOMAIN_EVENT_STOPPED_FAILED);
- libxlDomainEventQueue(driver, event);
}
See my question in your first series about whether the dom == NULL check
is even needed. If not, I can send a patch to remove the check, in
which case this patch wouldn't be needed.
https://www.redhat.com/archives/libvir-list/2014-August/msg01399.html
Regards,
Jim
7:08 a.m.
New subject: [libvirt] [PATCH 01/19] libxl_migration: Resolve Coverity NULL_RETURNS
On 08/27/2014 05:29 PM, Jim Fehlig wrote:
John Ferlan wrote:
> Coverity noted that all callers to libxlDomainEventQueue() could ensure
> the second parameter (event) was true before calling except this case.
> As I look at the code and how events are used - it seems that prior to
> generating an event for the dom == NULL condition, the resume/suspend
> event should be queue'd after the virDomainSaveStatus() call which will
> goto cleanup and queue the saved event anyway.
>
> Signed-off-by: John Ferlan <jferlan(a)redhat.com>
> ---
> src/libxl/libxl_migration.c | 6 +++++-
> 1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/src/libxl/libxl_migration.c b/src/libxl/libxl_migration.c
> index dbb5a8f..53ae63a 100644
> --- a/src/libxl/libxl_migration.c
> +++ b/src/libxl/libxl_migration.c
> @@ -512,6 +512,11 @@ libxlDomainMigrationFinish(virConnectPtr dconn,
> if (virDomainSaveStatus(driver->xmlopt, cfg->stateDir, vm) < 0)
> goto cleanup;
>
> + if (event) {
> + libxlDomainEventQueue(driver, event);
> + event = NULL;
> + }
> +
> dom = virGetDomain(dconn, vm->def->name, vm->def->uuid);
>
> if (dom == NULL) {
> @@ -519,7 +524,6 @@ libxlDomainMigrationFinish(virConnectPtr dconn,
> libxlDomainCleanup(driver, vm, VIR_DOMAIN_SHUTOFF_FAILED);
> event = virDomainEventLifecycleNewFromObj(vm, VIR_DOMAIN_EVENT_STOPPED,
> VIR_DOMAIN_EVENT_STOPPED_FAILED);
> - libxlDomainEventQueue(driver, event);
> }
>
See my question in your first series about whether the dom == NULL check
is even needed. If not, I can send a patch to remove the check, in
which case this patch wouldn't be needed.
https://www.redhat.com/archives/libvir-list/2014-August/msg01399.html
I am going to remove this one from my series and let you handle it.
I would seem perhaps that the code was there to ensure that if either of
the calls to virDomainObjSetState() ended up resulting in 'dom' not
returning something from the virGetDomain(), then like perhaps the qemu
migration code, the "best choice" was to remove it. In particular I
looked at the second VIR_DOMAIN_SHUTOFF_FAILED in qemuMigrationFinish()
for a "comparison". I'm by far no libvirt migration processing expert
though...
Tks -
John
10:20 a.m.
New subject: [libvirt] [PATCH 01/19] libxl_migration: Resolve Coverity NULL_RETURNS
John Ferlan wrote:
On 08/27/2014 05:29 PM, Jim Fehlig wrote:
> John Ferlan wrote:
>
>> Coverity noted that all callers to libxlDomainEventQueue() could ensure
>> the second parameter (event) was true before calling except this case.
>> As I look at the code and how events are used - it seems that prior to
>> generating an event for the dom == NULL condition, the resume/suspend
>> event should be queue'd after the virDomainSaveStatus() call which will
>> goto cleanup and queue the saved event anyway.
>>
>> Signed-off-by: John Ferlan <jferlan(a)redhat.com>
>> ---
>> src/libxl/libxl_migration.c | 6 +++++-
>> 1 file changed, 5 insertions(+), 1 deletion(-)
>>
>> diff --git a/src/libxl/libxl_migration.c b/src/libxl/libxl_migration.c
>> index dbb5a8f..53ae63a 100644
>> --- a/src/libxl/libxl_migration.c
>> +++ b/src/libxl/libxl_migration.c
>> @@ -512,6 +512,11 @@ libxlDomainMigrationFinish(virConnectPtr dconn,
>> if (virDomainSaveStatus(driver->xmlopt, cfg->stateDir, vm) < 0)
>> goto cleanup;
>>
>> + if (event) {
>> + libxlDomainEventQueue(driver, event);
>> + event = NULL;
>> + }
>> +
>> dom = virGetDomain(dconn, vm->def->name, vm->def->uuid);
>>
>> if (dom == NULL) {
>> @@ -519,7 +524,6 @@ libxlDomainMigrationFinish(virConnectPtr dconn,
>> libxlDomainCleanup(driver, vm, VIR_DOMAIN_SHUTOFF_FAILED);
>> event = virDomainEventLifecycleNewFromObj(vm, VIR_DOMAIN_EVENT_STOPPED,
>> VIR_DOMAIN_EVENT_STOPPED_FAILED);
>> - libxlDomainEventQueue(driver, event);
>> }
>>
>>
> See my question in your first series about whether the dom == NULL check
> is even needed. If not, I can send a patch to remove the check, in
> which case this patch wouldn't be needed.
>
> https://www.redhat.com/archives/libvir-list/2014-August/msg01399.html
>
>
>
I am going to remove this one from my series and let you handle it.
After looking at the code again, I think it is safest to go with your patch.
I would seem perhaps that the code was there to ensure that if either
of
the calls to virDomainObjSetState() ended up resulting in 'dom' not
returning something from the virGetDomain(), then like perhaps the qemu
migration code, the "best choice" was to remove it.
Yep, agreed. I haven't convinced myself that it is impossible for
virGetDomain() to return a NULL domainPtr, but in the event it does I
agree it is best to remove the domain.
Regards,
Jim
2:02 p.m.
New subject: [libvirt] [PATCH 01/19] libxl_migration: Resolve Coverity NULL_RETURNS
On 08/28/2014 11:20 AM, Jim Fehlig wrote:
John Ferlan wrote:
>>
>
> I am going to remove this one from my series and let you handle it.
>
After looking at the code again, I think it is safest to go with your patch.
> I would seem perhaps that the code was there to ensure that if either of
> the calls to virDomainObjSetState() ended up resulting in 'dom' not
> returning something from the virGetDomain(), then like perhaps the qemu
> migration code, the "best choice" was to remove it.
Yep, agreed. I haven't convinced myself that it is impossible for
virGetDomain() to return a NULL domainPtr, but in the event it does I
agree it is best to remove the domain.
Regards,
Jim
Close enough to an ACK for me ;-)
I've pushed the change
Thanks
John
3:54 p.m.
New subject: [libvirt] [PATCH 02/19] daemon: Resolve Coverity NEGATIVE_RETURNS
In each of these cases, Coverity complains that the result count returned
on error paths would be -1 disregarding that the count and the corresponding
are "linked" together (it doesn't know that). Simple enough to check and
remove the warning
Signed-off-by: John Ferlan <jferlan(a)redhat.com>
---
daemon/remote.c | 24 ++++++++++++------------
1 file changed, 12 insertions(+), 12 deletions(-)
diff --git a/daemon/remote.c b/daemon/remote.c
index 9251576..3efe12b 100644
--- a/daemon/remote.c
+++ b/daemon/remote.c
@@ -1535,7 +1535,7 @@ remoteDispatchConnectListAllDomains(virNetServerPtr server
ATTRIBUTE_UNUSED,
cleanup:
if (rv < 0)
virNetMessageSaveError(rerr);
- if (doms) {
+ if (doms && ndomains > 0) {
for (i = 0; i < ndomains; i++)
virDomainFree(doms[i]);
VIR_FREE(doms);
@@ -4632,7 +4632,7 @@ remoteDispatchDomainGetDiskErrors(virNetServerPtr server
ATTRIBUTE_UNUSED,
virNetMessageSaveError(rerr);
if (dom)
virDomainFree(dom);
- if (errors) {
+ if (errors && len > 0) {
size_t i;
for (i = 0; i < len; i++)
VIR_FREE(errors[i].disk);
@@ -4698,7 +4698,7 @@ remoteDispatchDomainListAllSnapshots(virNetServerPtr server
ATTRIBUTE_UNUSED,
virNetMessageSaveError(rerr);
if (dom)
virDomainFree(dom);
- if (snaps) {
+ if (snaps && nsnaps > 0) {
for (i = 0; i < nsnaps; i++)
virDomainSnapshotFree(snaps[i]);
VIR_FREE(snaps);
@@ -4769,7 +4769,7 @@ remoteDispatchDomainSnapshotListAllChildren(virNetServerPtr server
ATTRIBUTE_UNU
virDomainSnapshotFree(snapshot);
if (dom)
virDomainFree(dom);
- if (snaps) {
+ if (snaps && nsnaps > 0) {
for (i = 0; i < nsnaps; i++)
virDomainSnapshotFree(snaps[i]);
VIR_FREE(snaps);
@@ -4828,7 +4828,7 @@ remoteDispatchConnectListAllStoragePools(virNetServerPtr server
ATTRIBUTE_UNUSED
cleanup:
if (rv < 0)
virNetMessageSaveError(rerr);
- if (pools) {
+ if (pools && npools > 0) {
for (i = 0; i < npools; i++)
virStoragePoolFree(pools[i]);
VIR_FREE(pools);
@@ -4891,7 +4891,7 @@ remoteDispatchStoragePoolListAllVolumes(virNetServerPtr server
ATTRIBUTE_UNUSED,
cleanup:
if (rv < 0)
virNetMessageSaveError(rerr);
- if (vols) {
+ if (vols && nvols > 0) {
for (i = 0; i < nvols; i++)
virStorageVolFree(vols[i]);
VIR_FREE(vols);
@@ -4952,7 +4952,7 @@ remoteDispatchConnectListAllNetworks(virNetServerPtr server
ATTRIBUTE_UNUSED,
cleanup:
if (rv < 0)
virNetMessageSaveError(rerr);
- if (nets) {
+ if (nets && nnets > 0) {
for (i = 0; i < nnets; i++)
virNetworkFree(nets[i]);
VIR_FREE(nets);
@@ -5011,7 +5011,7 @@ remoteDispatchConnectListAllInterfaces(virNetServerPtr server
ATTRIBUTE_UNUSED,
cleanup:
if (rv < 0)
virNetMessageSaveError(rerr);
- if (ifaces) {
+ if (ifaces && nifaces > 0) {
for (i = 0; i < nifaces; i++)
virInterfaceFree(ifaces[i]);
VIR_FREE(ifaces);
@@ -5070,7 +5070,7 @@ remoteDispatchConnectListAllNodeDevices(virNetServerPtr server
ATTRIBUTE_UNUSED,
cleanup:
if (rv < 0)
virNetMessageSaveError(rerr);
- if (devices) {
+ if (devices && ndevices > 0) {
for (i = 0; i < ndevices; i++)
virNodeDeviceFree(devices[i]);
VIR_FREE(devices);
@@ -5129,7 +5129,7 @@ remoteDispatchConnectListAllNWFilters(virNetServerPtr server
ATTRIBUTE_UNUSED,
cleanup:
if (rv < 0)
virNetMessageSaveError(rerr);
- if (filters) {
+ if (filters && nfilters > 0) {
for (i = 0; i < nfilters; i++)
virNWFilterFree(filters[i]);
VIR_FREE(filters);
@@ -5188,7 +5188,7 @@ remoteDispatchConnectListAllSecrets(virNetServerPtr server
ATTRIBUTE_UNUSED,
cleanup:
if (rv < 0)
virNetMessageSaveError(rerr);
- if (secrets) {
+ if (secrets && nsecrets > 0) {
for (i = 0; i < nsecrets; i++)
virSecretFree(secrets[i]);
VIR_FREE(secrets);
@@ -6373,7 +6373,7 @@ remoteDispatchNetworkGetDHCPLeases(virNetServerPtr server
ATTRIBUTE_UNUSED,
cleanup:
if (rv < 0)
virNetMessageSaveError(rerr);
- if (leases) {
+ if (leases && nleases > 0) {
for (i = 0; i < nleases; i++)
virNetworkDHCPLeaseFree(leases[i]);
VIR_FREE(leases);
--
1.9.3
3:54 p.m.
New subject: [libvirt] [PATCH 03/19] domain_conf: Resolve Coverity RESOURCE_LEAK
Resolve a few RESOURCE_LEAK's identified by Coverity
Signed-off-by: John Ferlan <jferlan(a)redhat.com>
---
src/conf/domain_conf.c | 15 +++++++++++----
1 file changed, 11 insertions(+), 4 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 48afb8c..c7dbc73 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -5654,7 +5654,8 @@ virDomainDiskDefParseXML(virDomainXMLOptionPtr xmlopt,
}
VIR_FREE(ready);
}
- } else if (xmlStrEqual(cur->name, BAD_CAST "auth")) {
+ } else if (!authdef &&
+ xmlStrEqual(cur->name, BAD_CAST "auth")) {
if (!(authdef = virStorageAuthDefParse(node->doc, cur)))
goto error;
if ((auth_secret_usage =
@@ -12069,15 +12070,17 @@ virDomainDefParseXML(xmlDocPtr xml,
goto error;
}
- if (vcpupin->vcpuid >= def->vcpus)
+ if (vcpupin->vcpuid >= def->vcpus) {
/* To avoid the regression when daemon loading
* domain confs, we can't simply error out if
* <vcpupin> nodes greater than current vcpus,
* ignoring them instead.
*/
VIR_WARN("Ignore vcpupin for not onlined vcpus");
- else
+ VIR_FREE(vcpupin);
+ } else {
def->cputune.vcpupin[def->cputune.nvcpupin++] = vcpupin;
+ }
}
VIR_FREE(nodes);
@@ -13127,6 +13130,7 @@ virDomainDefParseXML(xmlDocPtr xml,
virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
_("Can't add host USB device: "
"USB is disabled in this host"));
+ virDomainHostdevDefFree(hostdev);
goto error;
}
@@ -13266,6 +13270,7 @@ virDomainDefParseXML(xmlDocPtr xml,
virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
_("Can't add redirected USB device: "
"USB is disabled for this domain"));
+ virDomainRedirdevDefFree(redirdev);
goto error;
}
@@ -15127,8 +15132,10 @@ virDomainEmulatorPinAdd(virDomainDefPtr def,
emulatorpin->vcpuid = -1;
emulatorpin->cpumask = virBitmapNewData(cpumap, maplen);
- if (!emulatorpin->cpumask)
+ if (!emulatorpin->cpumask) {
+ virDomainVcpuPinDefFree(emulatorpin);
return -1;
+ }
def->cputune.emulatorpin = emulatorpin;
} else {
--
1.9.3
3:48 a.m.
New subject: [libvirt] [PATCH 03/19] domain_conf: Resolve Coverity RESOURCE_LEAK
On 08/27/2014 10:54 PM, John Ferlan wrote:
Resolve a few RESOURCE_LEAK's identified by Coverity
Signed-off-by: John Ferlan <jferlan(a)redhat.com>
---
src/conf/domain_conf.c | 15 +++++++++++----
1 file changed, 11 insertions(+), 4 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 48afb8c..c7dbc73 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -5654,7 +5654,8 @@ virDomainDiskDefParseXML(virDomainXMLOptionPtr xmlopt,
}
VIR_FREE(ready);
}
- } else if (xmlStrEqual(cur->name, BAD_CAST "auth")) {
+ } else if (!authdef &&
+ xmlStrEqual(cur->name, BAD_CAST "auth")) {
if (!(authdef = virStorageAuthDefParse(node->doc, cur)))
goto error;
if ((auth_secret_usage =
@@ -12069,15 +12070,17 @@ virDomainDefParseXML(xmlDocPtr xml,
goto error;
}
- if (vcpupin->vcpuid >= def->vcpus)
+ if (vcpupin->vcpuid >= def->vcpus) {
/* To avoid the regression when daemon loading
* domain confs, we can't simply error out if
* <vcpupin> nodes greater than current vcpus,
* ignoring them instead.
*/
VIR_WARN("Ignore vcpupin for not onlined vcpus");
- else
+ VIR_FREE(vcpupin);
virDomainVcpuPinDefFree should be used here as well as in the 'duplicate
vcpupin' case a few lines above.
ACK with that fixed.
Jan
3:54 p.m.
New subject: [libvirt] [PATCH 04/19] cpu_x86: Resolve Coverity RESOURCE_LEAK
Coverity determined that the copied 'oldguest' would be leaked for
both error and success paths.
Signed-off-by: John Ferlan <jferlan(a)redhat.com>
---
src/cpu/cpu_x86.c | 15 ++++++++++-----
1 file changed, 10 insertions(+), 5 deletions(-)
diff --git a/src/cpu/cpu_x86.c b/src/cpu/cpu_x86.c
index fb89086..b460e8d 100644
--- a/src/cpu/cpu_x86.c
+++ b/src/cpu/cpu_x86.c
@@ -2024,8 +2024,9 @@ static int
x86UpdateHostModel(virCPUDefPtr guest,
const virCPUDef *host)
{
- virCPUDefPtr oldguest;
+ virCPUDefPtr oldguest = NULL;
size_t i;
+ int ret = -1;
guest->match = VIR_CPU_MATCH_EXACT;
@@ -2037,20 +2038,24 @@ x86UpdateHostModel(virCPUDefPtr guest,
/* update the host model according to the desired configuration */
if (!(oldguest = virCPUDefCopy(guest)))
- return -1;
+ goto cleanup;
virCPUDefFreeModel(guest);
if (virCPUDefCopyModel(guest, host, true) < 0)
- return -1;
+ goto cleanup;
for (i = 0; i < oldguest->nfeatures; i++) {
if (virCPUDefUpdateFeature(guest,
oldguest->features[i].name,
oldguest->features[i].policy) < 0)
- return -1;
+ goto cleanup;
}
- return 0;
+ ret = 0;
+
+ cleanup:
+ virCPUDefFree(oldguest);
+ return ret;
}
--
1.9.3
3:54 p.m.
New subject: [libvirt] [PATCH 05/19] qemu_command: Resolve Coverity RESOURCE_LEAK
In qemuParseISCSIString() if an error was returned, then the call
to qemuParseDriveURIString() where the uri is free'd wouldn't be run
Signed-off-by: John Ferlan <jferlan(a)redhat.com>
---
src/qemu/qemu_command.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 8fb81a4..528f947 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -2795,6 +2795,7 @@ qemuParseISCSIString(virDomainDiskDefPtr def)
virReportError(VIR_ERR_INTERNAL_ERROR,
_("invalid name '%s' for iSCSI disk"),
def->src->path);
+ virURIFree(uri);
return -1;
}
}
--
1.9.3
3:54 p.m.
New subject: [libvirt] [PATCH 06/19] qemu_agent: Resolve Coverity RESOURCE_LEAK
Coverity found that on error paths, the 'arg' value wasn't be cleaned
up. Followed the example in qemuAgentSetVCPUs() where upon successful call
to qemuAgentCommand() the 'cpus' is set to NULL; otherwise, when cleanup
occurs the free the memory for 'arg'
Signed-off-by: John Ferlan <jferlan(a)redhat.com>
---
src/qemu/qemu_agent.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/src/qemu/qemu_agent.c b/src/qemu/qemu_agent.c
index a10954a..fe38f6d 100644
--- a/src/qemu/qemu_agent.c
+++ b/src/qemu/qemu_agent.c
@@ -1328,7 +1328,7 @@ int qemuAgentFSFreeze(qemuAgentPtr mon, const char **mountpoints,
unsigned int nmountpoints)
{
int ret = -1;
- virJSONValuePtr cmd, arg;
+ virJSONValuePtr cmd, arg = NULL;
virJSONValuePtr reply = NULL;
if (mountpoints && nmountpoints) {
@@ -1343,7 +1343,8 @@ int qemuAgentFSFreeze(qemuAgentPtr mon, const char **mountpoints,
}
if (!cmd)
- return -1;
+ goto cleanup;
+ arg = NULL;
if (qemuAgentCommand(mon, cmd, &reply, true,
VIR_DOMAIN_QEMU_AGENT_COMMAND_BLOCK) < 0)
@@ -1355,6 +1356,7 @@ int qemuAgentFSFreeze(qemuAgentPtr mon, const char **mountpoints,
}
cleanup:
+ virJSONValueFree(arg);
virJSONValueFree(cmd);
virJSONValueFree(reply);
return ret;
--
1.9.3
9:40 p.m.
New subject: [libvirt] [PATCH 06/19] qemu_agent: Resolve Coverity RESOURCE_LEAK
On 2014/8/28 4:54, John Ferlan wrote:
Coverity found that on error paths, the 'arg' value
wasn't be cleaned
up. Followed the example in qemuAgentSetVCPUs() where upon successful call
to qemuAgentCommand() the 'cpus' is set to NULL; otherwise, when cleanup
occurs the free the memory for 'arg'
Signed-off-by: John Ferlan <jferlan(a)redhat.com>
---
src/qemu/qemu_agent.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/src/qemu/qemu_agent.c b/src/qemu/qemu_agent.c
index a10954a..fe38f6d 100644
--- a/src/qemu/qemu_agent.c
+++ b/src/qemu/qemu_agent.c
@@ -1328,7 +1328,7 @@ int qemuAgentFSFreeze(qemuAgentPtr mon, const char **mountpoints,
unsigned int nmountpoints)
{
int ret = -1;
- virJSONValuePtr cmd, arg;
+ virJSONValuePtr cmd, arg = NULL;
virJSONValuePtr reply = NULL;
if (mountpoints && nmountpoints) {
@@ -1343,7 +1343,8 @@ int qemuAgentFSFreeze(qemuAgentPtr mon, const char **mountpoints,
}
if (!cmd)
- return -1;
+ goto cleanup;
+ arg = NULL;
Setting arg to NULL can also lead to memory leak.
It makes virJSONValueFree(arg) below invalid.
if (qemuAgentCommand(mon, cmd, &reply, true,
VIR_DOMAIN_QEMU_AGENT_COMMAND_BLOCK) < 0)
@@ -1355,6 +1356,7 @@ int qemuAgentFSFreeze(qemuAgentPtr mon, const char **mountpoints,
}
cleanup:
+ virJSONValueFree(arg);
virJSONValueFree(cmd);
virJSONValueFree(reply);
return ret;
4:03 a.m.
New subject: [libvirt] [PATCH 06/19] qemu_agent: Resolve Coverity RESOURCE_LEAK
On 08/28/2014 04:40 AM, Wang Rui wrote:
On 2014/8/28 4:54, John Ferlan wrote:
> Coverity found that on error paths, the 'arg' value wasn't be cleaned
> up. Followed the example in qemuAgentSetVCPUs() where upon successful call
> to qemuAgentCommand() the 'cpus' is set to NULL; otherwise, when cleanup
> occurs the free the memory for 'arg'
>
> Signed-off-by: John Ferlan <jferlan(a)redhat.com>
> ---
> src/qemu/qemu_agent.c | 6 ++++--
> 1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/src/qemu/qemu_agent.c b/src/qemu/qemu_agent.c
> index a10954a..fe38f6d 100644
> --- a/src/qemu/qemu_agent.c
> +++ b/src/qemu/qemu_agent.c
> @@ -1328,7 +1328,7 @@ int qemuAgentFSFreeze(qemuAgentPtr mon, const char
**mountpoints,
> unsigned int nmountpoints)
> {
> int ret = -1;
> - virJSONValuePtr cmd, arg;
> + virJSONValuePtr cmd, arg = NULL;
> virJSONValuePtr reply = NULL;
>
> if (mountpoints && nmountpoints) {
> @@ -1343,7 +1343,8 @@ int qemuAgentFSFreeze(qemuAgentPtr mon, const char
**mountpoints,
> }
>
> if (!cmd)
> - return -1;
> + goto cleanup;
> + arg = NULL;
Setting arg to NULL can also lead to memory leak.
It makes virJSONValueFree(arg) below invalid.
If qemuAgentMakeCommand succeeds, the 'arg' array is now owned by 'cmd'
and we
need to set it to NULL here to prevent double free.
ACK to the patch as-is.
Jan
5:25 a.m.
New subject: [libvirt] [PATCH 06/19] qemu_agent: Resolve Coverity RESOURCE_LEAK
On 2014/8/28 17:03, Ján Tomko wrote:
On 08/28/2014 04:40 AM, Wang Rui wrote:
> On 2014/8/28 4:54, John Ferlan wrote:
>> Coverity found that on error paths, the 'arg' value wasn't be
cleaned
>> up. Followed the example in qemuAgentSetVCPUs() where upon successful call
>> to qemuAgentCommand() the 'cpus' is set to NULL; otherwise, when cleanup
>> occurs the free the memory for 'arg'
>>
>> Signed-off-by: John Ferlan <jferlan(a)redhat.com>
>> ---
>> src/qemu/qemu_agent.c | 6 ++++--
>> 1 file changed, 4 insertions(+), 2 deletions(-)
>>
[...]
> Setting arg to NULL can also lead to memory leak.
> It makes virJSONValueFree(arg) below invalid.
If qemuAgentMakeCommand succeeds, the 'arg' array is now owned by 'cmd'
and we
need to set it to NULL here to prevent double free.
Oh, I got it. Thanks.
3:54 p.m.
New subject: [libvirt] [PATCH 07/19] libxl_domain: Resolve Coverity RESOURCE_LEAK
On the error path need to free the chrdef
Signed-off-by: John Ferlan <jferlan(a)redhat.com>
---
src/libxl/libxl_domain.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/src/libxl/libxl_domain.c b/src/libxl/libxl_domain.c
index cdac82c..557fc20 100644
--- a/src/libxl/libxl_domain.c
+++ b/src/libxl/libxl_domain.c
@@ -532,8 +532,10 @@ libxlDomainDefPostParse(virDomainDefPtr def,
chrdef->target.port = 0;
chrdef->targetType = VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_XEN;
- if (VIR_ALLOC_N(def->consoles, 1) < 0)
+ if (VIR_ALLOC_N(def->consoles, 1) < 0) {
+ virDomainChrDefFree(chrdef);
return -1;
+ }
def->nconsoles = 1;
def->consoles[0] = chrdef;
--
1.9.3
3:54 p.m.
New subject: [libvirt] [PATCH 08/19] qemu_capabilities: Resolve Coverity RESOURCE_LEAK
Coverity determined that on error path that 'mach' wouldn't be free'd
Since virCapabilitiesFreeGuestMachine() isn't globally available, we'll
insert first and then if the VIR_STRDUP's fail they it will eventually
cause the 'mach' to be freed in the error path
Signed-off-by: John Ferlan <jferlan(a)redhat.com>
---
src/qemu/qemu_capabilities.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
index ce899f2..fcd7b19 100644
--- a/src/qemu/qemu_capabilities.c
+++ b/src/qemu/qemu_capabilities.c
@@ -2106,6 +2106,7 @@ int virQEMUCapsGetMachineTypesCaps(virQEMUCapsPtr qemuCaps,
virCapsGuestMachinePtr mach;
if (VIR_ALLOC(mach) < 0)
goto error;
+ (*machines)[i] = mach;
if (qemuCaps->machineAliases[i]) {
if (VIR_STRDUP(mach->name, qemuCaps->machineAliases[i]) < 0 ||
VIR_STRDUP(mach->canonical, qemuCaps->machineTypes[i]) < 0)
@@ -2115,7 +2116,6 @@ int virQEMUCapsGetMachineTypesCaps(virQEMUCapsPtr qemuCaps,
goto error;
}
mach->maxCpus = qemuCaps->machineMaxCpus[i];
- (*machines)[i] = mach;
}
return 0;
--
1.9.3
3:54 p.m.
New subject: [libvirt] [PATCH 09/19] network_conf: Resolve Coverity RESOURCE_LEAK
Need to VIR_FREE the startip/endip we allocated for the error message
Signed-off-by: John Ferlan <jferlan(a)redhat.com>
---
src/conf/network_conf.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c
index dc25c6e..9571ee1 100644
--- a/src/conf/network_conf.c
+++ b/src/conf/network_conf.c
@@ -3627,6 +3627,8 @@ virNetworkDefUpdateIPDHCPRange(virNetworkDefPtr def,
def->name,
startip ? startip : "unknown",
endip ? endip : "unknown");
+ VIR_FREE(startip);
+ VIR_FREE(endip);
goto cleanup;
}
--
1.9.3
3:54 p.m.
New subject: [libvirt] [PATCH 10/19] virsh-network: Resolve Coverity RESOURCE_LEAK
Need to free 'xmlFromFile' on/for the error path when current was
returning false only
Signed-off-by: John Ferlan <jferlan(a)redhat.com>
---
tools/virsh-network.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/tools/virsh-network.c b/tools/virsh-network.c
index 578abe0..9497472 100644
--- a/tools/virsh-network.c
+++ b/tools/virsh-network.c
@@ -974,7 +974,7 @@ cmdNetworkUpdate(vshControl *ctl, const vshCmd *cmd)
if (current) {
if (live || config) {
vshError(ctl, "%s", _("--current must be specified
exclusively"));
- return false;
+ goto cleanup;
}
flags |= VIR_NETWORK_UPDATE_AFFECT_CURRENT;
} else {
--
1.9.3
3:54 p.m.
New subject: [libvirt] [PATCH 11/19] bridge_driver: Resolve Coverity RESOURCE_LEAK
In the error path the 'ipaddr' wasn't VIR_FREE'd before jumping to
cleanup
Signed-off-by: John Ferlan <jferlan(a)redhat.com>
---
src/network/bridge_driver.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
index 9491c51..4b3f07f 100644
--- a/src/network/bridge_driver.c
+++ b/src/network/bridge_driver.c
@@ -993,6 +993,7 @@ networkDnsmasqConfContents(virNetworkObjPtr network,
"(as described in
RFC1918/RFC3484/RFC4193)."),
ipaddr, (int)version / 1000000,
(int)(version % 1000000) / 1000);
+ VIR_FREE(ipaddr);
goto cleanup;
}
virBufferAsprintf(&configbuf, "listen-address=%s\n", ipaddr);
--
1.9.3
3:54 p.m.
New subject: [libvirt] [PATCH 12/19] libxl_migration: Resolve Coverity RESOURCE_LEAK
In libxlDomainMigrationPrepare() if the uri_in is false, then
'hostname' is allocated and used "generically" in the routine,
but not freed. Conversely, if uri_in is true, then a uri is
allocated and hostname is set to the uri->hostname value and
likewise generically used.
At function exit, hostname wasn't free'd in the !uri_in path,
so that was added. To just make it clearer on usage the else
path became the call to virURIFree() although I suppose technically
it didn't have to since it would be a call using (NULL)
Signed-off-by: John Ferlan <jferlan(a)redhat.com>
---
src/libxl/libxl_migration.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/src/libxl/libxl_migration.c b/src/libxl/libxl_migration.c
index 53ae63a..0b562f7 100644
--- a/src/libxl/libxl_migration.c
+++ b/src/libxl/libxl_migration.c
@@ -412,7 +412,10 @@ libxlDomainMigrationPrepare(virConnectPtr dconn,
}
done:
- virURIFree(uri);
+ if (!uri_in)
+ VIR_FREE(hostname);
+ else
+ virURIFree(uri);
if (vm)
virObjectUnlock(vm);
return ret;
--
1.9.3
3:54 p.m.
New subject: [libvirt] [PATCH 13/19] phyp_driver: Resolve Coverity RESOURCE_LEAK
Coverity determines that when jumping to the connected: label, the
addressinfo (ai) is not free'd.
Signed-off-by: John Ferlan <jferlan(a)redhat.com>
---
src/phyp/phyp_driver.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/src/phyp/phyp_driver.c b/src/phyp/phyp_driver.c
index aa1e105..ea1981a 100644
--- a/src/phyp/phyp_driver.c
+++ b/src/phyp/phyp_driver.c
@@ -946,6 +946,7 @@ openSSHSession(virConnectPtr conn, virConnectAuthPtr auth,
sock = socket(cur->ai_family, cur->ai_socktype, cur->ai_protocol);
if (sock >= 0) {
if (connect(sock, cur->ai_addr, cur->ai_addrlen) == 0) {
+ freeaddrinfo(ai);
goto connected;
}
VIR_FORCE_CLOSE(sock);
--
1.9.3
3:54 p.m.
New subject: [libvirt] [PATCH 14/19] qemu_driver: Resolve Coverity RESOURCE_LEAK
Coverity found that the 'buf' wasn't VIR_FREE'd at exit.
Signed-off-by: John Ferlan <jferlan(a)redhat.com>
---
src/qemu/qemu_driver.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 73959da..323957e 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -10516,6 +10516,7 @@ qemuDomainGetBlockInfo(virDomainPtr dom,
}
cleanup:
+ VIR_FREE(buf);
VIR_FREE(alias);
virStorageSourceFree(meta);
VIR_FORCE_CLOSE(fd);
--
1.9.3
3:54 p.m.
New subject: [libvirt] [PATCH 15/19] storage_conf: Resolve Coverity RESOURCE_LEAK
If there was a failure processing 'authdef' and the code went to cleanup
before the setting to source->auth, then it'd be leaked.
Signed-off-by: John Ferlan <jferlan(a)redhat.com>
---
src/conf/storage_conf.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c
index 5a16767..e72a869 100644
--- a/src/conf/storage_conf.c
+++ b/src/conf/storage_conf.c
@@ -661,6 +661,7 @@ virStoragePoolDefParseSource(xmlXPathContextPtr ctxt,
}
source->auth = authdef;
+ authdef = NULL;
}
source->vendor = virXPathString("string(./vendor/@name)", ctxt);
@@ -673,6 +674,7 @@ virStoragePoolDefParseSource(xmlXPathContextPtr ctxt,
VIR_FREE(port);
VIR_FREE(nodeset);
VIR_FREE(adapter_type);
+ virStorageAuthDefFree(authdef);
return ret;
}
--
1.9.3
3:54 p.m.
New subject: [libvirt] [PATCH 16/19] qemu_monitor: Resolve Coverity NESTING_INDENT_MISMATCH
The PROBE macro can expand to more than one line/statement - put curly
braces around the if statement to be safe
Signed-off-by: John Ferlan <jferlan(a)redhat.com>
---
src/qemu/qemu_monitor.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c
index d5ba08d..5b2952a 100644
--- a/src/qemu/qemu_monitor.c
+++ b/src/qemu/qemu_monitor.c
@@ -506,10 +506,11 @@ qemuMonitorIOWrite(qemuMonitorPtr mon)
mon->msg->txLength - mon->msg->txOffset,
done, errno);
- if (mon->msg->txFD != -1)
+ if (mon->msg->txFD != -1) {
PROBE(QEMU_MONITOR_IO_SEND_FD,
"mon=%p fd=%d ret=%d errno=%d",
mon, mon->msg->txFD, done, errno);
+ }
if (done < 0) {
if (errno == EAGAIN)
--
1.9.3
3:54 p.m.
New subject: [libvirt] [PATCH 17/19] domain_conf: Resolve Coverity DEADCODE
A bunch of a useless warnings brought on by our own doing.
Signed-off-by: John Ferlan <jferlan(a)redhat.com>
---
src/conf/domain_conf.c | 13 +++++++++++++
1 file changed, 13 insertions(+)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index c7dbc73..ce5aad6 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -2804,6 +2804,8 @@ virDomainDeviceInfoIterateInternal(virDomainDefPtr def,
return -1;
}
+ /* Coverity is not very happy with this - all dead_error_condition */
+#if !STATIC_ANALYSIS
/* This switch statement is here to trigger compiler warning when adding
* a new device type. When you are adding a new field to the switch you
* also have to add an iteration statement above. Otherwise the switch
@@ -2833,6 +2835,7 @@ virDomainDeviceInfoIterateInternal(virDomainDefPtr def,
case VIR_DOMAIN_DEVICE_RNG:
break;
}
+#endif
return 0;
}
@@ -12263,6 +12266,7 @@ virDomainDefParseXML(xmlDocPtr xml,
ctxt->node = node;
break;
+ /* coverity[dead_error_begin] */
case VIR_DOMAIN_FEATURE_LAST:
break;
}
@@ -12346,6 +12350,7 @@ virDomainDefParseXML(xmlDocPtr xml,
def->hyperv_features[feature] = value;
break;
+ /* coverity[dead_error_begin] */
case VIR_DOMAIN_HYPERV_LAST:
break;
}
@@ -12394,6 +12399,7 @@ virDomainDefParseXML(xmlDocPtr xml,
def->kvm_features[feature] = value;
break;
+ /* coverity[dead_error_begin] */
case VIR_DOMAIN_KVM_LAST:
break;
}
@@ -14406,6 +14412,7 @@ virDomainDefFeaturesCheckABIStability(virDomainDefPtr src,
}
break;
+ /* coverity[dead_error_begin] */
case VIR_DOMAIN_HYPERV_LAST:
break;
}
@@ -14429,6 +14436,7 @@ virDomainDefFeaturesCheckABIStability(virDomainDefPtr src,
break;
+ /* coverity[dead_error_begin] */
case VIR_DOMAIN_KVM_LAST:
break;
}
@@ -18253,6 +18261,7 @@ virDomainDefFormatInternal(virDomainDefPtr def,
virBufferAddLit(buf, "/>\n");
break;
+ /* coverity[dead_error_begin] */
case VIR_DOMAIN_HYPERV_LAST:
break;
}
@@ -18277,6 +18286,7 @@ virDomainDefFormatInternal(virDomainDefPtr def,
def->kvm_features[j]));
break;
+ /* coverity[dead_error_begin] */
case VIR_DOMAIN_KVM_LAST:
break;
}
@@ -18304,6 +18314,7 @@ virDomainDefFormatInternal(virDomainDefPtr def,
virBufferAddLit(buf, "</capabilities>\n");
break;
+ /* coverity[dead_error_begin] */
case VIR_DOMAIN_FEATURE_LAST:
break;
}
@@ -20239,6 +20250,7 @@ virDomainObjGetMetadata(virDomainObjPtr vm,
goto cleanup;
break;
+ /* coverity[dead_error_begin] */
case VIR_DOMAIN_METADATA_LAST:
break;
}
@@ -20324,6 +20336,7 @@ virDomainDefSetMetadata(virDomainDefPtr def,
}
break;
+ /* coverity[dead_error_begin] */
case VIR_DOMAIN_METADATA_LAST:
break;
}
--
1.9.3
3:54 p.m.
New subject: [libvirt] [PATCH 18/19] qemu_driver: Resolve Coverity DEADCODE
A bunch of false positives brought on by our own doings
Signed-off-by: John Ferlan <jferlan(a)redhat.com>
---
src/qemu/qemu_driver.c | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 323957e..2c27cd6 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -8167,6 +8167,7 @@ qemuDomainGetBlkioParameters(virDomainPtr dom,
goto cleanup;
break;
+ /* coverity[dead_error_begin] */
default:
break;
/* should not hit here */
@@ -8349,6 +8350,7 @@ qemuDomainGetBlkioParameters(virDomainPtr dom,
break;
+ /* coverity[dead_error_begin] */
default:
break;
/* should not hit here */
@@ -8601,6 +8603,7 @@ qemuDomainGetMemoryParameters(virDomainPtr dom,
goto cleanup;
break;
+ /* coverity[dead_error_begin] */
default:
break;
/* should not hit here */
@@ -8645,6 +8648,7 @@ qemuDomainGetMemoryParameters(virDomainPtr dom,
goto cleanup;
break;
+ /* coverity[dead_error_begin] */
default:
break;
/* should not hit here */
@@ -8942,6 +8946,7 @@ qemuDomainGetNumaParameters(virDomainPtr dom,
break;
+ /* coverity[dead_error_begin] */
default:
break;
/* should not hit here */
@@ -10118,6 +10123,7 @@ qemuDomainGetInterfaceParameters(virDomainPtr dom,
if (net->bandwidth && net->bandwidth->out)
params[i].value.ui = net->bandwidth->out->burst;
break;
+ /* coverity[dead_error_begin] */
default:
break;
/* should not hit here */
@@ -16188,6 +16194,7 @@ qemuDomainGetBlockIoTune(virDomainPtr dom,
reply.write_iops_sec) < 0)
goto endjob;
break;
+ /* coverity[dead_error_begin] */
default:
break;
}
--
1.9.3
3:54 p.m.
New subject: [libvirt] [PATCH 19/19] qemu_command: Resolve Coverity DEADCODE
One useless warning, but the other one rather pertinent. On entry
the 'trans' variable is initialized to VIR_DOMAIN_DISK_TRANS_DEFAULT.
When the "trans" was found in the parsing loop it def->geometry.trans
was assigned to the return from virDomainDiskGeometryTransTypeFromString
and then 'trans' was used to do the comparison to see if it was valid.
So remove 'trans' and use def->geometry.trans properly
Signed-off-by: John Ferlan <jferlan(a)redhat.com>
---
src/qemu/qemu_command.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 528f947..bf6285f 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -6237,6 +6237,7 @@ qemuBuildCpuArgStr(virQEMUDriverPtr driver,
def->hyperv_spinlocks);
break;
+ /* coverity[dead_error_begin] */
case VIR_DOMAIN_HYPERV_LAST:
break;
}
@@ -9835,7 +9836,6 @@ qemuParseCommandLineDisk(virDomainXMLOptionPtr xmlopt,
int idx = -1;
int busid = -1;
int unitid = -1;
- int trans = VIR_DOMAIN_DISK_TRANS_DEFAULT;
if (qemuParseKeywords(val,
&keywords,
@@ -10054,12 +10054,12 @@ qemuParseCommandLineDisk(virDomainXMLOptionPtr xmlopt,
} else if (STREQ(keywords[i], "trans")) {
def->geometry.trans =
virDomainDiskGeometryTransTypeFromString(values[i]);
- if ((trans < VIR_DOMAIN_DISK_TRANS_DEFAULT) ||
- (trans >= VIR_DOMAIN_DISK_TRANS_LAST)) {
+ if ((def->geometry.trans < VIR_DOMAIN_DISK_TRANS_DEFAULT) ||
+ (def->geometry.trans >= VIR_DOMAIN_DISK_TRANS_LAST)) {
virDomainDiskDefFree(def);
def = NULL;
virReportError(VIR_ERR_INTERNAL_ERROR,
- _("cannot parse translation
value'%s'"),
+ _("cannot parse translation value
'%s'"),
values[i]);
goto error;
}
--
1.9.3
8:38 p.m.
On 2014/8/28 4:54, John Ferlan wrote:
I almost didn't want to do this due to the sheer volume, but
figured
at the very least the bulk of these are resource leaks found by the
much pickier new coverity scanner.
After this there are "only" 70 issues found...
Nice. I did coverity scan yesterday by coincidence. There are 1400+
total errors on my scan environment. I began to analyze 300+ RESOURCE LEAK
errors. You said there were more than 140 on your side. So when you have
finished fixing errors on your side, I'm glad to check if some other
RESOURCE LEAK errors left and fix them
7:26 a.m.
On 08/27/2014 09:38 PM, Wang Rui wrote:
On 2014/8/28 4:54, John Ferlan wrote:
> I almost didn't want to do this due to the sheer volume, but figured
> at the very least the bulk of these are resource leaks found by the
> much pickier new coverity scanner.
>
> After this there are "only" 70 issues found...
Nice. I did coverity scan yesterday by coincidence. There are 1400+
total errors on my scan environment. I began to analyze 300+ RESOURCE LEAK
errors. You said there were more than 140 on your side. So when you have
finished fixing errors on your side, I'm glad to check if some other
RESOURCE LEAK errors left and fix them
I used "Coverity Static Analysis version 7.5.0 on Linux
3.15.8-200.fc20.x86_64 x86_64" that we have as a shared license of sorts
inside Red Hat.
I generally just take the defaults for the analysis "strength" (eg, the
--aggressiveness-level is the default of low rather than medium or high,
which I know from experience are much more intolerant).
My 141 count covered the gamut of issues and will be reduced to 72 with
these patches applied. Some of the remaining are sourced from the same
problem - some are false positives. It just takes time to go through
and figure that out and what the "best solution" may be.
Perhaps you are running with a different version or a different
aggressiveness especially to have 300+ resource leaks. That's fine -
the more found the better off we are.
John
4:27 a.m.
On 08/27/2014 10:54 PM, John Ferlan wrote:
I almost didn't want to do this due to the sheer volume, but
figured
at the very least the bulk of these are resource leaks found by the
much pickier new coverity scanner.
After this there are "only" 70 issues found...
John Ferlan (19):
libxl_migration: Resolve Coverity NULL_RETURNS
I still don't know how to answer this one.
ACK to the rest, see my reply to 3/19 for a minor tweak.
Jan
daemon: Resolve Coverity NEGATIVE_RETURNS
domain_conf: Resolve Coverity RESOURCE_LEAK
cpu_x86: Resolve Coverity RESOURCE_LEAK
qemu_command: Resolve Coverity RESOURCE_LEAK
qemu_agent: Resolve Coverity RESOURCE_LEAK
libxl_domain: Resolve Coverity RESOURCE_LEAK
qemu_capabilities: Resolve Coverity RESOURCE_LEAK
network_conf: Resolve Coverity RESOURCE_LEAK
virsh-network: Resolve Coverity RESOURCE_LEAK
bridge_driver: Resolve Coverity RESOURCE_LEAK
libxl_migration: Resolve Coverity RESOURCE_LEAK
phyp_driver: Resolve Coverity RESOURCE_LEAK
qemu_driver: Resolve Coverity RESOURCE_LEAK
storage_conf: Resolve Coverity RESOURCE_LEAK
qemu_monitor: Resolve Coverity NESTING_INDENT_MISMATCH
domain_conf: Resolve Coverity DEADCODE
qemu_driver: Resolve Coverity DEADCODE
qemu_command: Resolve Coverity DEADCODE
7:29 a.m.
On 08/27/2014 04:54 PM, John Ferlan wrote:
I almost didn't want to do this due to the sheer volume, but
figured
at the very least the bulk of these are resource leaks found by the
much pickier new coverity scanner.
After this there are "only" 70 issues found...
John Ferlan (19):
libxl_migration: Resolve Coverity NULL_RETURNS
daemon: Resolve Coverity NEGATIVE_RETURNS
domain_conf: Resolve Coverity RESOURCE_LEAK
cpu_x86: Resolve Coverity RESOURCE_LEAK
qemu_command: Resolve Coverity RESOURCE_LEAK
qemu_agent: Resolve Coverity RESOURCE_LEAK
libxl_domain: Resolve Coverity RESOURCE_LEAK
qemu_capabilities: Resolve Coverity RESOURCE_LEAK
network_conf: Resolve Coverity RESOURCE_LEAK
virsh-network: Resolve Coverity RESOURCE_LEAK
bridge_driver: Resolve Coverity RESOURCE_LEAK
libxl_migration: Resolve Coverity RESOURCE_LEAK
phyp_driver: Resolve Coverity RESOURCE_LEAK
qemu_driver: Resolve Coverity RESOURCE_LEAK
storage_conf: Resolve Coverity RESOURCE_LEAK
qemu_monitor: Resolve Coverity NESTING_INDENT_MISMATCH
domain_conf: Resolve Coverity DEADCODE
qemu_driver: Resolve Coverity DEADCODE
qemu_command: Resolve Coverity DEADCODE
daemon/remote.c | 24 ++++++++++++------------
src/conf/domain_conf.c | 28 ++++++++++++++++++++++++----
src/conf/network_conf.c | 2 ++
src/conf/storage_conf.c | 2 ++
src/cpu/cpu_x86.c | 15 ++++++++++-----
src/libxl/libxl_domain.c | 4 +++-
src/libxl/libxl_migration.c | 11 +++++++++--
src/network/bridge_driver.c | 1 +
src/phyp/phyp_driver.c | 1 +
src/qemu/qemu_agent.c | 6 ++++--
src/qemu/qemu_capabilities.c | 2 +-
src/qemu/qemu_command.c | 9 +++++----
src/qemu/qemu_driver.c | 8 ++++++++
src/qemu/qemu_monitor.c | 3 ++-
tools/virsh-network.c | 2 +-
15 files changed, 85 insertions(+), 33 deletions(-)
I removed patch 1 (letting Jim handle it)
I modified patch 3 (or now 2) to use virDomainVcpuPinDefFree()
I have pushed the series.
Thanks for the quick review -
John
3773
days inactive
3774
days old
31 comments
4 participants
participants (4)
-
Jim Fehlig -
John Ferlan -
Ján Tomko -
Wang Rui