[libvirt] [PATCH 0/3] Fix a couple of coverity found issues
The first two are coverity found and the last was "extra" as I looked in the code. John Ferlan (3): tests: Return failures immediately in virErrorTestMsgs remote: Resolve resource leak remote: Handle xdr char ** data return fields more consistently src/remote/remote_daemon_dispatch.c | 42 +++++++++-------------------- tests/virerrortest.c | 11 ++++---- 2 files changed, 18 insertions(+), 35 deletions(-) -- 2.17.2
Rather than chancing calling strchr when @err_noinfo == NULL or calling virErrorTestMsgFormatInfoOne when @err_info == NULL, both of which would fail, let's just return -1 immediately when we encounter an error. Signed-off-by: John Ferlan <jferlan@redhat.com> --- tests/virerrortest.c | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/tests/virerrortest.c b/tests/virerrortest.c index eebf259343..48b4848b44 100644 --- a/tests/virerrortest.c +++ b/tests/virerrortest.c @@ -58,7 +58,6 @@ virErrorTestMsgs(const void *opaque ATTRIBUTE_UNUSED) const char *err_noinfo; const char *err_info; size_t i; - int ret = 0; for (i = 1; i < VIR_ERR_NUMBER_LAST; i++) { err_noinfo = virErrorMsg(i, NULL); @@ -66,25 +65,25 @@ virErrorTestMsgs(const void *opaque ATTRIBUTE_UNUSED) if (!err_noinfo) { VIR_TEST_VERBOSE("\nmissing string without info for error id %zu\n", i); - ret = -1; + return -1; } if (!err_info) { VIR_TEST_VERBOSE("\nmissing string with info for error id %zu\n", i); - ret = -1; + return -1; } if (strchr(err_noinfo, '%')) { VIR_TEST_VERBOSE("\nerror message id %zu contains formatting characters: '%s'\n", i, err_noinfo); - ret = -1; + return -1; } if (virErrorTestMsgFormatInfoOne(err_info) < 0) - ret = -1; + return -1; } - return ret; + return 0; } -- 2.17.2
On Mon, Dec 17, 2018 at 09:55:46 -0500, John Ferlan wrote:
Rather than chancing calling strchr when @err_noinfo == NULL or calling virErrorTestMsgFormatInfoOne when @err_info == NULL, both of which would fail, let's just return -1 immediately when we encounter an error.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- tests/virerrortest.c | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-)
This would make the test hard to diagnose as it spits out one failure at time. I prefer you add the checks explicitly.
On 12/18/18 5:34 AM, Peter Krempa wrote:
On Mon, Dec 17, 2018 at 09:55:46 -0500, John Ferlan wrote:
Rather than chancing calling strchr when @err_noinfo == NULL or calling virErrorTestMsgFormatInfoOne when @err_info == NULL, both of which would fail, let's just return -1 immediately when we encounter an error.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- tests/virerrortest.c | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-)
This would make the test hard to diagnose as it spits out one failure at time. I prefer you add the checks explicitly.
To avoid wasting steps/time w/ multiple iterations, you would rather have the "err_noinfo &&" be a gate to strchr and "err_info &&" be a gate to virErrorTestMsgFormatInfoOne? So that the only diffs are: - if (strchr(err_noinfo, '%')) { + if (err_noinfo && strchr(err_noinfo, '%')) { and - if (virErrorTestMsgFormatInfoOne(err_info) < 0) + if (err_info && virErrorTestMsgFormatInfoOne(err_info) < 0) Tks - John
On Tue, Dec 18, 2018 at 08:21:08 -0500, John Ferlan wrote:
On 12/18/18 5:34 AM, Peter Krempa wrote:
On Mon, Dec 17, 2018 at 09:55:46 -0500, John Ferlan wrote:
Rather than chancing calling strchr when @err_noinfo == NULL or calling virErrorTestMsgFormatInfoOne when @err_info == NULL, both of which would fail, let's just return -1 immediately when we encounter an error.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- tests/virerrortest.c | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-)
This would make the test hard to diagnose as it spits out one failure at time. I prefer you add the checks explicitly.
To avoid wasting steps/time w/ multiple iterations, you would rather have the "err_noinfo &&" be a gate to strchr and "err_info &&" be a gate to virErrorTestMsgFormatInfoOne?
Yes exactly.
So that the only diffs are:
- if (strchr(err_noinfo, '%')) { + if (err_noinfo && strchr(err_noinfo, '%')) {
and
- if (virErrorTestMsgFormatInfoOne(err_info) < 0) + if (err_info && virErrorTestMsgFormatInfoOne(err_info) < 0)
Tks -
ACK to that version
Using a combination of VIR_ALLOC and VIR_STRDUP into a local variable and then jumping to error on the VIR_STRDUP before assiging it into the @data would cause a memory leak. Let's just avoid that by assiging directly into @data. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/remote/remote_daemon_dispatch.c | 17 ++++++----------- 1 file changed, 6 insertions(+), 11 deletions(-) diff --git a/src/remote/remote_daemon_dispatch.c b/src/remote/remote_daemon_dispatch.c index 51bc055564..5cefc8480e 100644 --- a/src/remote/remote_daemon_dispatch.c +++ b/src/remote/remote_daemon_dispatch.c @@ -749,7 +749,6 @@ remoteRelayDomainEventDiskChange(virConnectPtr conn, { daemonClientEventCallbackPtr callback = opaque; remote_domain_event_disk_change_msg data; - char **oldSrcPath_p = NULL, **newSrcPath_p = NULL; if (callback->callbackID < 0 || !remoteRelayDomainEventCheckACL(callback->client, conn, dom)) @@ -762,17 +761,15 @@ remoteRelayDomainEventDiskChange(virConnectPtr conn, /* build return data */ memset(&data, 0, sizeof(data)); if (oldSrcPath && - ((VIR_ALLOC(oldSrcPath_p) < 0) || - VIR_STRDUP(*oldSrcPath_p, oldSrcPath) < 0)) + ((VIR_ALLOC(data.oldSrcPath) < 0) || + VIR_STRDUP(*(data.oldSrcPath), oldSrcPath) < 0)) goto error; if (newSrcPath && - ((VIR_ALLOC(newSrcPath_p) < 0) || - VIR_STRDUP(*newSrcPath_p, newSrcPath) < 0)) + ((VIR_ALLOC(data.newSrcPath) < 0) || + VIR_STRDUP(*(data.newSrcPath), newSrcPath) < 0)) goto error; - data.oldSrcPath = oldSrcPath_p; - data.newSrcPath = newSrcPath_p; if (VIR_STRDUP(data.devAlias, devAlias) < 0) goto error; data.reason = reason; @@ -1779,7 +1776,6 @@ remoteRelayDomainQemuMonitorEvent(virConnectPtr conn, { daemonClientEventCallbackPtr callback = opaque; qemu_domain_monitor_event_msg data; - char **details_p = NULL; if (callback->callbackID < 0 || !remoteRelayDomainQemuMonitorEventCheckACL(callback->client, conn, @@ -1797,10 +1793,9 @@ remoteRelayDomainQemuMonitorEvent(virConnectPtr conn, data.seconds = seconds; data.micros = micros; if (details && - ((VIR_ALLOC(details_p) < 0) || - VIR_STRDUP(*details_p, details) < 0)) + ((VIR_ALLOC(data.details) < 0) || + VIR_STRDUP(*(data.details), details) < 0)) goto error; - data.details = details_p; if (make_nonnull_domain(&data.dom, dom) < 0) goto error; -- 2.17.2
On 12/17/18 3:55 PM, John Ferlan wrote:
Using a combination of VIR_ALLOC and VIR_STRDUP into a local variable and then jumping to error on the VIR_STRDUP before assiging it into the @data would cause a memory leak. Let's just avoid that by assiging directly into @data.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/remote/remote_daemon_dispatch.c | 17 ++++++----------- 1 file changed, 6 insertions(+), 11 deletions(-)
ACK Michal
For consistency, handle the @data "char **" (or remote_string) assignments and processing similarly between various APIs Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/remote/remote_daemon_dispatch.c | 25 +++++++------------------ 1 file changed, 7 insertions(+), 18 deletions(-) diff --git a/src/remote/remote_daemon_dispatch.c b/src/remote/remote_daemon_dispatch.c index 5cefc8480e..fcd602304f 100644 --- a/src/remote/remote_daemon_dispatch.c +++ b/src/remote/remote_daemon_dispatch.c @@ -1366,7 +1366,6 @@ remoteRelayDomainEventMetadataChange(virConnectPtr conn, { daemonClientEventCallbackPtr callback = opaque; remote_domain_event_callback_metadata_change_msg data; - char **nsurip; if (callback->callbackID < 0 || !remoteRelayDomainEventCheckACL(callback->client, conn, dom)) @@ -1380,13 +1379,9 @@ remoteRelayDomainEventMetadataChange(virConnectPtr conn, data.type = type; if (nsuri) { - if (VIR_ALLOC(nsurip) < 0) - return -1; - if (VIR_STRDUP(*nsurip, nsuri) < 0) { - VIR_FREE(nsurip); - return -1; - } - data.nsuri = nsurip; + if (VIR_ALLOC(data.nsuri) < 0 || + VIR_STRDUP(*(data.nsuri), nsuri) < 0) + goto error; } if (make_nonnull_domain(&data.dom, dom) < 0) @@ -1432,9 +1427,8 @@ remoteRelayDomainEventBlockThreshold(virConnectPtr conn, if (VIR_STRDUP(data.dev, dev) < 0) goto error; if (path) { - if (VIR_ALLOC(data.path) < 0) - goto error; - if (VIR_STRDUP(*(data.path), path) < 0) + if (VIR_ALLOC(data.path) < 0 || + VIR_STRDUP(*(data.path), path) < 0) goto error; } data.threshold = threshold; @@ -4006,14 +4000,9 @@ remoteDispatchNodeDeviceGetParent(virNetServerPtr server ATTRIBUTE_UNUSED, ret->parentName = NULL; } else { /* remoteDispatchClientRequest will free this. */ - char **parent_p; - if (VIR_ALLOC(parent_p) < 0) + if (VIR_ALLOC(ret->parentName) < 0 || + VIR_STRDUP(*(ret->parentName), parent) < 0) goto cleanup; - if (VIR_STRDUP(*parent_p, parent) < 0) { - VIR_FREE(parent_p); - goto cleanup; - } - ret->parentName = parent_p; } rv = 0; -- 2.17.2
On 12/17/18 3:55 PM, John Ferlan wrote:
For consistency, handle the @data "char **" (or remote_string) assignments and processing similarly between various APIs
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/remote/remote_daemon_dispatch.c | 25 +++++++------------------ 1 file changed, 7 insertions(+), 18 deletions(-)
ACK Michal
participants (3)
-
John Ferlan -
Michal Privoznik -
Peter Krempa