6:18 p.m.
Newer Coverity (7.7.0) found a couple real issues and a few more
false positives. There's still a few more to be resolved, but still
trying to figure them out...
libxlDomainMigrationPrepare - claim is args is leaked. Although
it seems to be handled in libxlMigrateReceive or libxlDoMigrateReceive.
Don't know the code well enough to do proper triage.
qemuProcessStop - claim is usage of net->ifname in the condition
(vport->virtPortType == VIR_NETDEV_VPORT_PROFILE_OPENVSWITCH)
is using a NULL net->ifname that would have been VIR_FREE()'d
when case VIR_DOMAIN_NET_TYPE_DIRECT a few lines above.
qemuDomainBlockRebase - claim is overwriting 'dest' after call to
qemuDomainBlockCopyCommon causes resource leak. Although reading
code says otherwise, except of course if CopyCommon goes to cleanup
rather than endjob. Even changing the location doesn't resolve
the issue.
Fix the Coverity tag for the DEADCODE... It's always a coin-flip either
dead_error_begin or dead_error_condition - I gave Jiri bad advice as
to which one to use it seems.
John Ferlan (6):
Avoid Coverity FORWARD_NULL prior to strtok_r calls
tests: Resolve Coverity RESOURCE_LEAK
tests: Resolve Coverity RESOURCE_LEAK
virsh: Resolve Coverity DEADCODE
qemu: Resolve Coverity CHECKED_RETURN
qemu: Resolve Coverity RESOURCE_LEAK
src/esx/esx_vi.c | 1 +
src/libxl/libxl_conf.c | 1 +
src/openvz/openvz_conf.c | 2 ++
src/qemu/qemu_driver.c | 24 +++++++++++-------------
src/qemu/qemu_process.c | 6 +++---
src/xenapi/xenapi_utils.c | 1 +
tests/qemucaps2xmltest.c | 1 +
tests/virnetdaemontest.c | 2 ++
tools/virsh.c | 2 +-
9 files changed, 23 insertions(+), 17 deletions(-)
--
2.1.0
6:18 p.m.
New subject: [libvirt] [PATCH 1/6] Avoid Coverity FORWARD_NULL prior to strtok_r calls
The 'strtok_r' function requires passing a NULL as the first parameter
on subsequent calls in order to ensure the code picks up where it left
off on a previous call. However, Coverity doesn't quite realize this
and points out that if a NULL was passed in as the third argument it would
result in a possible NULL deref because the strtok_r function will assign
the third argument to the first in the call is NULL.
Adding an sa_assert() prior to each initial call quiets Coverity
Signed-off-by: John Ferlan <jferlan(a)redhat.com>
---
src/esx/esx_vi.c | 1 +
src/libxl/libxl_conf.c | 1 +
src/openvz/openvz_conf.c | 2 ++
src/xenapi/xenapi_utils.c | 1 +
4 files changed, 5 insertions(+)
diff --git a/src/esx/esx_vi.c b/src/esx/esx_vi.c
index af822b1..a57d753 100644
--- a/src/esx/esx_vi.c
+++ b/src/esx/esx_vi.c
@@ -1173,6 +1173,7 @@ esxVI_Context_LookupManagedObjectsByPath(esxVI_Context *ctx, const
char *path)
goto cleanup;
/* Lookup Datacenter */
+ sa_assert(tmp);
item = strtok_r(tmp, "/", &saveptr);
if (!item) {
diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c
index 35fd495..ab588e8 100644
--- a/src/libxl/libxl_conf.c
+++ b/src/libxl/libxl_conf.c
@@ -353,6 +353,7 @@ libxlCapsInitGuests(libxl_ctx *ctx, virCapsPtr caps)
/* Split capabilities string into tokens. strtok_r is OK here because
* we "own" the buffer. Parse out the features from each token.
*/
+ sa_assert(ver_info->capabilities);
for (str = ver_info->capabilities, nr_guest_archs = 0;
nr_guest_archs < sizeof(guest_archs) / sizeof(guest_archs[0])
&& (token = strtok_r(str, " ", &saveptr)) !=
NULL;
diff --git a/src/openvz/openvz_conf.c b/src/openvz/openvz_conf.c
index db0a9a7..003272e 100644
--- a/src/openvz/openvz_conf.c
+++ b/src/openvz/openvz_conf.c
@@ -138,6 +138,7 @@ openvzParseBarrierLimit(const char* value,
char *str;
int ret = -1;
+ sa_assert(value);
if (VIR_STRDUP(str, value) < 0)
goto error;
@@ -965,6 +966,7 @@ openvzGetVPSUUID(int vpsid, char *uuidstr, size_t len)
}
}
+ sa_assert(line);
iden = strtok_r(line, " ", &saveptr);
uuidbuf = strtok_r(NULL, "\n", &saveptr);
diff --git a/src/xenapi/xenapi_utils.c b/src/xenapi/xenapi_utils.c
index a80e084..6b710cd 100644
--- a/src/xenapi/xenapi_utils.c
+++ b/src/xenapi/xenapi_utils.c
@@ -301,6 +301,7 @@ getCpuBitMapfromString(char *mask, unsigned char *cpumap, int maplen)
int max_bits = maplen * 8;
char *num = NULL, *bp = NULL;
bzero(cpumap, maplen);
+ sa_assert(mask);
num = strtok_r(mask, ",", &bp);
while (num != NULL) {
if (virStrToLong_i(num, NULL, 10, &pos) < 0)
--
2.1.0
3:13 a.m.
New subject: [libvirt] [PATCH 1/6] Avoid Coverity FORWARD_NULL prior to strtok_r calls
On Wed, Sep 23, 2015 at 07:18:28PM -0400, John Ferlan wrote:
The 'strtok_r' function requires passing a NULL as the first
parameter
on subsequent calls in order to ensure the code picks up where it left
off on a previous call. However, Coverity doesn't quite realize this
and points out that if a NULL was passed in as the third argument it would
result in a possible NULL deref because the strtok_r function will assign
the third argument to the first in the call is NULL.
The description seems contradictory to the patch. From the asserts that
fix it, I'd assume Coverity has a problem with the first argument
possibly being NULL - usually obtained from a library function Coverity
doesn't understand.
Adding an sa_assert() prior to each initial call quiets Coverity
Thus rendering the coverity scan useless.
Signed-off-by: John Ferlan <jferlan(a)redhat.com>
---
src/esx/esx_vi.c | 1 +
src/libxl/libxl_conf.c | 1 +
src/openvz/openvz_conf.c | 2 ++
src/xenapi/xenapi_utils.c | 1 +
4 files changed, 5 insertions(+)
diff --git a/src/esx/esx_vi.c b/src/esx/esx_vi.c
index af822b1..a57d753 100644
--- a/src/esx/esx_vi.c
+++ b/src/esx/esx_vi.c
@@ -1173,6 +1173,7 @@ esxVI_Context_LookupManagedObjectsByPath(esxVI_Context *ctx, const
char *path)
goto cleanup;
/* Lookup Datacenter */
+ sa_assert(tmp);
This should be asserted in the caller. It seems priv->parsedUri->path
can be NULL in esxConnectToVCenter, but this function is not called in
that case.
item = strtok_r(tmp, "/", &saveptr);
if (!item) {
diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c
index 35fd495..ab588e8 100644
--- a/src/libxl/libxl_conf.c
+++ b/src/libxl/libxl_conf.c
@@ -353,6 +353,7 @@ libxlCapsInitGuests(libxl_ctx *ctx, virCapsPtr caps)
/* Split capabilities string into tokens. strtok_r is OK here because
* we "own" the buffer. Parse out the features from each token.
*/
+ sa_assert(ver_info->capabilities);
This one should be closer to the function that filled out the string.
for (str = ver_info->capabilities, nr_guest_archs = 0;
nr_guest_archs < sizeof(guest_archs) / sizeof(guest_archs[0])
&& (token = strtok_r(str, " ", &saveptr)) !=
NULL;
diff --git a/src/openvz/openvz_conf.c b/src/openvz/openvz_conf.c
index db0a9a7..003272e 100644
--- a/src/openvz/openvz_conf.c
+++ b/src/openvz/openvz_conf.c
@@ -138,6 +138,7 @@ openvzParseBarrierLimit(const char* value,
char *str;
int ret = -1;
+ sa_assert(value);
Same here.
if (VIR_STRDUP(str, value) < 0)
goto error;
@@ -965,6 +966,7 @@ openvzGetVPSUUID(int vpsid, char *uuidstr, size_t len)
}
}
+ sa_assert(line);
This one already is right after getline.
iden = strtok_r(line, " ", &saveptr);
uuidbuf = strtok_r(NULL, "\n", &saveptr);
diff --git a/src/xenapi/xenapi_utils.c b/src/xenapi/xenapi_utils.c
index a80e084..6b710cd 100644
--- a/src/xenapi/xenapi_utils.c
+++ b/src/xenapi/xenapi_utils.c
@@ -301,6 +301,7 @@ getCpuBitMapfromString(char *mask, unsigned char *cpumap, int
maplen)
int max_bits = maplen * 8;
char *num = NULL, *bp = NULL;
bzero(cpumap, maplen);
+ sa_assert(mask);
num = strtok_r(mask, ",", &bp);
while (num != NULL) {
if (virStrToLong_i(num, NULL, 10, &pos) < 0)
virBitmap* functions can be used instead.
Jan
8:21 a.m.
New subject: [libvirt] [PATCH 1/6] Avoid Coverity FORWARD_NULL prior to strtok_r calls
On 09/24/2015 04:13 AM, Ján Tomko wrote:
On Wed, Sep 23, 2015 at 07:18:28PM -0400, John Ferlan wrote:
> The 'strtok_r' function requires passing a NULL as the first parameter
> on subsequent calls in order to ensure the code picks up where it left
> off on a previous call. However, Coverity doesn't quite realize this
> and points out that if a NULL was passed in as the third argument it would
> result in a possible NULL deref because the strtok_r function will assign
> the third argument to the first in the call is NULL.
The description seems contradictory to the patch. From the asserts that
fix it, I'd assume Coverity has a problem with the first argument
possibly being NULL - usually obtained from a library function Coverity
doesn't understand.
Been a while since I wrote this one and I've forgotten the details -
what I do "see" is according to Coverity for each of the instances
below, the strtok_r ends up calling __strtok_r_1c for some reason.
Although there are other strtok_r calls which don't end up in that same
path (some in the same module. I'll dig a bit deeper on each.
John
>
> Adding an sa_assert() prior to each initial call quiets Coverity
>
Thus rendering the coverity scan useless.
> Signed-off-by: John Ferlan <jferlan(a)redhat.com>
> ---
> src/esx/esx_vi.c | 1 +
> src/libxl/libxl_conf.c | 1 +
> src/openvz/openvz_conf.c | 2 ++
> src/xenapi/xenapi_utils.c | 1 +
> 4 files changed, 5 insertions(+)
>
> diff --git a/src/esx/esx_vi.c b/src/esx/esx_vi.c
> index af822b1..a57d753 100644
> --- a/src/esx/esx_vi.c
> +++ b/src/esx/esx_vi.c
> @@ -1173,6 +1173,7 @@ esxVI_Context_LookupManagedObjectsByPath(esxVI_Context *ctx,
const char *path)
> goto cleanup;
>
> /* Lookup Datacenter */
> + sa_assert(tmp);
This should be asserted in the caller. It seems priv->parsedUri->path
can be NULL in esxConnectToVCenter, but this function is not called in
that case.
> item = strtok_r(tmp, "/", &saveptr);
>
> if (!item) {
> diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c
> index 35fd495..ab588e8 100644
> --- a/src/libxl/libxl_conf.c
> +++ b/src/libxl/libxl_conf.c
> @@ -353,6 +353,7 @@ libxlCapsInitGuests(libxl_ctx *ctx, virCapsPtr caps)
> /* Split capabilities string into tokens. strtok_r is OK here because
> * we "own" the buffer. Parse out the features from each token.
> */
> + sa_assert(ver_info->capabilities);
This one should be closer to the function that filled out the string.
> for (str = ver_info->capabilities, nr_guest_archs = 0;
> nr_guest_archs < sizeof(guest_archs) / sizeof(guest_archs[0])
> && (token = strtok_r(str, " ", &saveptr)) !=
NULL;
> diff --git a/src/openvz/openvz_conf.c b/src/openvz/openvz_conf.c
> index db0a9a7..003272e 100644
> --- a/src/openvz/openvz_conf.c
> +++ b/src/openvz/openvz_conf.c
> @@ -138,6 +138,7 @@ openvzParseBarrierLimit(const char* value,
> char *str;
> int ret = -1;
>
> + sa_assert(value);
Same here.
> if (VIR_STRDUP(str, value) < 0)
> goto error;
>
> @@ -965,6 +966,7 @@ openvzGetVPSUUID(int vpsid, char *uuidstr, size_t len)
> }
> }
>
> + sa_assert(line);
This one already is right after getline.
> iden = strtok_r(line, " ", &saveptr);
> uuidbuf = strtok_r(NULL, "\n", &saveptr);
>
> diff --git a/src/xenapi/xenapi_utils.c b/src/xenapi/xenapi_utils.c
> index a80e084..6b710cd 100644
> --- a/src/xenapi/xenapi_utils.c
> +++ b/src/xenapi/xenapi_utils.c
> @@ -301,6 +301,7 @@ getCpuBitMapfromString(char *mask, unsigned char *cpumap, int
maplen)
> int max_bits = maplen * 8;
> char *num = NULL, *bp = NULL;
> bzero(cpumap, maplen);
> + sa_assert(mask);
> num = strtok_r(mask, ",", &bp);
> while (num != NULL) {
> if (virStrToLong_i(num, NULL, 10, &pos) < 0)
virBitmap* functions can be used instead.
Jan
6:18 p.m.
New subject: [libvirt] [PATCH 2/6] tests: Resolve Coverity RESOURCE_LEAK
In the error path need to unref the 'caps' as well
Signed-off-by: John Ferlan <jferlan(a)redhat.com>
---
tests/qemucaps2xmltest.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/tests/qemucaps2xmltest.c b/tests/qemucaps2xmltest.c
index 6a5811c..7adde24 100644
--- a/tests/qemucaps2xmltest.c
+++ b/tests/qemucaps2xmltest.c
@@ -113,6 +113,7 @@ testGetCaps(char *capsData, const testQemuData *data)
error:
virObjectUnref(qemuCaps);
+ virObjectUnref(caps);
return NULL;
}
--
2.1.0
2:29 a.m.
New subject: [libvirt] [PATCH 2/6] tests: Resolve Coverity RESOURCE_LEAK
On Wed, Sep 23, 2015 at 07:18:29PM -0400, John Ferlan wrote:
In the error path need to unref the 'caps' as well
Signed-off-by: John Ferlan <jferlan(a)redhat.com>
---
tests/qemucaps2xmltest.c | 1 +
1 file changed, 1 insertion(+)
ACK
Jan
6:18 p.m.
New subject: [libvirt] [PATCH 3/6] tests: Resolve Coverity RESOURCE_LEAK
The cleanup path did not clear the reference for sk1 and sk2
Signed-off-by: John Ferlan <jferlan(a)redhat.com>
---
tests/virnetdaemontest.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/tests/virnetdaemontest.c b/tests/virnetdaemontest.c
index fb8a6c0..24cbd54 100644
--- a/tests/virnetdaemontest.c
+++ b/tests/virnetdaemontest.c
@@ -125,6 +125,8 @@ testCreateServer(const char *host, int family)
virObjectUnref(cln2);
virObjectUnref(svc1);
virObjectUnref(svc2);
+ virObjectUnref(sk1);
+ virObjectUnref(sk2);
return srv;
error:
--
2.1.0
2:29 a.m.
New subject: [libvirt] [PATCH 3/6] tests: Resolve Coverity RESOURCE_LEAK
On Wed, Sep 23, 2015 at 07:18:30PM -0400, John Ferlan wrote:
The cleanup path did not clear the reference for sk1 and sk2
Signed-off-by: John Ferlan <jferlan(a)redhat.com>
---
tests/virnetdaemontest.c | 2 ++
1 file changed, 2 insertions(+)
ACK
Jan
6:18 p.m.
New subject: [libvirt] [PATCH 4/6] virsh: Resolve Coverity DEADCODE
Use 'dead_error_condition' instead of 'dead_error_begin'
Signed-off-by: John Ferlan <jferlan(a)redhat.com>
---
tools/virsh.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/tools/virsh.c b/tools/virsh.c
index 76b5e9f..7484bed 100644
--- a/tools/virsh.c
+++ b/tools/virsh.c
@@ -118,7 +118,7 @@ virshCatchDisconnect(virConnectPtr conn,
case VIR_CONNECT_CLOSE_REASON_KEEPALIVE:
str = N_("Disconnected from %s due to keepalive timeout");
break;
- /* coverity[dead_error_begin] */
+ /* coverity[dead_error_condition] */
case VIR_CONNECT_CLOSE_REASON_CLIENT:
case VIR_CONNECT_CLOSE_REASON_LAST:
break;
--
2.1.0
2:47 a.m.
New subject: [libvirt] [PATCH 4/6] virsh: Resolve Coverity DEADCODE
On Wed, Sep 23, 2015 at 07:18:31PM -0400, John Ferlan wrote:
Use 'dead_error_condition' instead of
'dead_error_begin'
Just wondering - what is the difference?
Doesn't dead_error_begin require a dead_error_end?
Signed-off-by: John Ferlan <jferlan(a)redhat.com>
---
tools/virsh.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
ACK.
Jan
8:42 a.m.
New subject: [libvirt] [PATCH 4/6] virsh: Resolve Coverity DEADCODE
On 09/24/2015 03:47 AM, Ján Tomko wrote:
On Wed, Sep 23, 2015 at 07:18:31PM -0400, John Ferlan wrote:
> Use 'dead_error_condition' instead of 'dead_error_begin'
>
Just wondering - what is the difference?
Perhaps just the one condition and not a set of conditions or lines? Not
quite sure, but you know where to find the internal output and you'll
see the data spewed by Coverity about the deadcode.
Doesn't dead_error_begin require a dead_error_end?
No - once it's dead, it's not coming back to life.
John
> Signed-off-by: John Ferlan <jferlan(a)redhat.com>
> ---
> tools/virsh.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
ACK.
Jan
6:18 p.m.
New subject: [libvirt] [PATCH 5/6] qemu: Resolve Coverity CHECKED_RETURN
Coverity complains that return from virHookCall is not checked in
one place in qemuProcessStop. Since the comment notes that we cannot
stop the operation even it if fails, just added the ignore_value.
Signed-off-by: John Ferlan <jferlan(a)redhat.com>
---
src/qemu/qemu_process.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index b961f40..eb04883 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -5276,9 +5276,9 @@ void qemuProcessStop(virQEMUDriverPtr driver,
char *xml = qemuDomainDefFormatXML(driver, vm->def, 0);
/* we can't stop the operation even if the script raised an error */
- virHookCall(VIR_HOOK_DRIVER_QEMU, vm->def->name,
- VIR_HOOK_QEMU_OP_STOPPED, VIR_HOOK_SUBOP_END,
- NULL, xml, NULL);
+ ignore_value(virHookCall(VIR_HOOK_DRIVER_QEMU, vm->def->name,
+ VIR_HOOK_QEMU_OP_STOPPED, VIR_HOOK_SUBOP_END,
+ NULL, xml, NULL));
VIR_FREE(xml);
}
--
2.1.0
2:30 a.m.
New subject: [libvirt] [PATCH 5/6] qemu: Resolve Coverity CHECKED_RETURN
On Wed, Sep 23, 2015 at 07:18:32PM -0400, John Ferlan wrote:
Coverity complains that return from virHookCall is not checked in
one place in qemuProcessStop. Since the comment notes that we cannot
stop the operation even it if fails, just added the ignore_value.
Signed-off-by: John Ferlan <jferlan(a)redhat.com>
---
src/qemu/qemu_process.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
ACK
Jan
6:18 p.m.
New subject: [libvirt] [PATCH 6/6] qemu: Resolve Coverity RESOURCE_LEAK
This seemed to be more of a false positive as for some reason Coverity
was missing the "ret < 0" goto error condition and somehow believing that
event could be overwritten. At first I thought it was just the ret != 0
condition difference, but it wasn't.
In any case, make use of the recent change to qemuDomainEventQueue to
check event == NULL and just pass it as a parameter directly in the
error path. That avoids the error.
Signed-off-by: John Ferlan <jferlan(a)redhat.com>
---
src/qemu/qemu_driver.c | 24 +++++++++++-------------
1 file changed, 11 insertions(+), 13 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 2387cf3..3a98774 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -3155,7 +3155,7 @@ qemuDomainSaveMemory(virQEMUDriverPtr driver,
virFileWrapperFdFree(wrapperFd);
VIR_FREE(xml);
- if (ret != 0 && needUnlink)
+ if (ret < 0 && needUnlink)
unlink(path);
return ret;
@@ -3174,7 +3174,6 @@ qemuDomainSaveInternal(virQEMUDriverPtr driver, virDomainPtr dom,
char *xml = NULL;
bool was_running = false;
int ret = -1;
- int rc;
virObjectEventPtr event = NULL;
qemuDomainObjPrivatePtr priv = vm->privateData;
virCapsPtr caps;
@@ -3249,21 +3248,20 @@ qemuDomainSaveInternal(virQEMUDriverPtr driver, virDomainPtr dom,
/* Shut it down */
qemuProcessStop(driver, vm, VIR_DOMAIN_SHUTOFF_SAVED, 0);
virDomainAuditStop(vm, "saved");
- event = virDomainEventLifecycleNewFromObj(vm,
- VIR_DOMAIN_EVENT_STOPPED,
- VIR_DOMAIN_EVENT_STOPPED_SAVED);
+ event = virDomainEventLifecycleNewFromObj(vm, VIR_DOMAIN_EVENT_STOPPED,
+ VIR_DOMAIN_EVENT_STOPPED_SAVED);
endjob:
- if (ret != 0) {
+ if (ret < 0) {
if (was_running && virDomainObjIsActive(vm)) {
virErrorPtr save_err = virSaveLastError();
- rc = qemuProcessStartCPUs(driver, vm, dom->conn,
- VIR_DOMAIN_RUNNING_SAVE_CANCELED,
- QEMU_ASYNC_JOB_SAVE);
- if (rc < 0) {
+ if (qemuProcessStartCPUs(driver, vm, dom->conn,
+ VIR_DOMAIN_RUNNING_SAVE_CANCELED,
+ QEMU_ASYNC_JOB_SAVE) < 0) {
VIR_WARN("Unable to resume guest CPUs after save failure");
- event = virDomainEventLifecycleNewFromObj(vm,
- VIR_DOMAIN_EVENT_SUSPENDED,
-
VIR_DOMAIN_EVENT_SUSPENDED_API_ERROR);
+ qemuDomainEventQueue(driver,
+ virDomainEventLifecycleNewFromObj(vm,
+ VIR_DOMAIN_EVENT_SUSPENDED,
+ VIR_DOMAIN_EVENT_SUSPENDED_API_ERROR));
}
virSetError(save_err);
virFreeError(save_err);
--
2.1.0
2:45 a.m.
New subject: [libvirt] [PATCH 6/6] qemu: Resolve Coverity RESOURCE_LEAK
On Wed, Sep 23, 2015 at 07:18:33PM -0400, John Ferlan wrote:
This seemed to be more of a false positive as for some reason
Coverity
was missing the "ret < 0" goto error condition and somehow believing that
event could be overwritten. At first I thought it was just the ret != 0
condition difference, but it wasn't.
Then those changes are cosmetic and don't need to be in a commit fixing
the RESOURCE_LEAK.
In any case, make use of the recent change to qemuDomainEventQueue
to
check event == NULL and just pass it as a parameter directly in the
error path. That avoids the error.
Signed-off-by: John Ferlan <jferlan(a)redhat.com>
---
src/qemu/qemu_driver.c | 24 +++++++++++-------------
1 file changed, 11 insertions(+), 13 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 2387cf3..3a98774 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -3155,7 +3155,7 @@ qemuDomainSaveMemory(virQEMUDriverPtr driver,
virFileWrapperFdFree(wrapperFd);
VIR_FREE(xml);
- if (ret != 0 && needUnlink)
+ if (ret < 0 && needUnlink)
unlink(path);
return ret;
@@ -3174,7 +3174,6 @@ qemuDomainSaveInternal(virQEMUDriverPtr driver, virDomainPtr dom,
char *xml = NULL;
bool was_running = false;
int ret = -1;
- int rc;
virObjectEventPtr event = NULL;
qemuDomainObjPrivatePtr priv = vm->privateData;
virCapsPtr caps;
@@ -3249,21 +3248,20 @@ qemuDomainSaveInternal(virQEMUDriverPtr driver, virDomainPtr
dom,
/* Shut it down */
qemuProcessStop(driver, vm, VIR_DOMAIN_SHUTOFF_SAVED, 0);
virDomainAuditStop(vm, "saved");
- event = virDomainEventLifecycleNewFromObj(vm,
- VIR_DOMAIN_EVENT_STOPPED,
- VIR_DOMAIN_EVENT_STOPPED_SAVED);
+ event = virDomainEventLifecycleNewFromObj(vm, VIR_DOMAIN_EVENT_STOPPED,
+ VIR_DOMAIN_EVENT_STOPPED_SAVED);
endjob:
- if (ret != 0) {
+ if (ret < 0) {
if (was_running && virDomainObjIsActive(vm)) {
virErrorPtr save_err = virSaveLastError();
- rc = qemuProcessStartCPUs(driver, vm, dom->conn,
- VIR_DOMAIN_RUNNING_SAVE_CANCELED,
- QEMU_ASYNC_JOB_SAVE);
- if (rc < 0) {
+ if (qemuProcessStartCPUs(driver, vm, dom->conn,
+ VIR_DOMAIN_RUNNING_SAVE_CANCELED,
+ QEMU_ASYNC_JOB_SAVE) < 0) {
VIR_WARN("Unable to resume guest CPUs after save failure");
- event = virDomainEventLifecycleNewFromObj(vm,
- VIR_DOMAIN_EVENT_SUSPENDED,
-
VIR_DOMAIN_EVENT_SUSPENDED_API_ERROR);
+ qemuDomainEventQueue(driver,
+ virDomainEventLifecycleNewFromObj(vm,
+ VIR_DOMAIN_EVENT_SUSPENDED,
+ VIR_DOMAIN_EVENT_SUSPENDED_API_ERROR));
ACK to this hunk as the coverity fix, and the rest pushed separately.
Jan
8:58 a.m.
New subject: [libvirt] [PATCH 6/6] qemu: Resolve Coverity RESOURCE_LEAK
On 09/24/2015 03:45 AM, Ján Tomko wrote:
On Wed, Sep 23, 2015 at 07:18:33PM -0400, John Ferlan wrote:
> This seemed to be more of a false positive as for some reason Coverity
> was missing the "ret < 0" goto error condition and somehow believing
that
> event could be overwritten. At first I thought it was just the ret != 0
> condition difference, but it wasn't.
>
Then those changes are cosmetic and don't need to be in a commit fixing
the RESOURCE_LEAK.
OK - I've split this patch in two - cosmetic and resource_leak and
pushed the patches that have been ACK'd... I'll continue to plug through
issues noted from patch 1 along with the ones referenced in my cover letter.
Tks -
John
7:47 p.m.
On 09/23/2015 05:18 PM, John Ferlan wrote:
Newer Coverity (7.7.0) found a couple real issues and a few more
false positives. There's still a few more to be resolved, but still
trying to figure them out...
libxlDomainMigrationPrepare - claim is args is leaked. Although
it seems to be handled in libxlMigrateReceive or libxlDoMigrateReceive.
Don't know the code well enough to do proper triage.
args is a subclass of virObject. It is freed when refcnt reaches zero and the
dispose function is called.
args is created (refcnt=1) in libxlDomainMigrationPrepare() and added to the
virNetSocket IO callback. When the sender connects, the callback invokes
libxlMigrateReceive() passing args. libxlMigrateReceive() starts a thread to
receive the migration data (avoids blocking the event loop while receiving the
migration data :-)), passing args to the thread start function,
libxlDoMigrateReceive(). args is unref'ed in libxlMigrateReceive() if there are
any failures, otherwise args is unref'ed in libxlDoMigrateReceive().
Regards,
Jim
3320
days inactive
3329
days old
16 comments
3 participants
participants (3)
-
Jim Fehlig -
John Ferlan -
Ján Tomko