8:55 a.m.
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
8:55 a.m.
New subject: [libvirt] [PATCH 1/3] tests: Return failures immediately in virErrorTestMsgs
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(a)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
4:34 a.m.
New subject: [libvirt] [PATCH 1/3] tests: Return failures immediately in virErrorTestMsgs
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(a)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.
7:21 a.m.
New subject: [libvirt] [PATCH 1/3] tests: Return failures immediately in virErrorTestMsgs
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(a)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
7:39 a.m.
New subject: [libvirt] [PATCH 1/3] tests: Return failures immediately in virErrorTestMsgs
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(a)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
8:55 a.m.
New subject: [libvirt] [PATCH 2/3] remote: Resolve resource leak
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(a)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
7:40 a.m.
New subject: [libvirt] [PATCH 2/3] remote: Resolve resource leak
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(a)redhat.com>
---
src/remote/remote_daemon_dispatch.c | 17 ++++++-----------
1 file changed, 6 insertions(+), 11 deletions(-)
ACK
Michal
8:55 a.m.
New subject: [libvirt] [PATCH 3/3] remote: Handle xdr char ** data return fields more consistently
For consistency, handle the @data "char **" (or remote_string)
assignments and processing similarly between various APIs
Signed-off-by: John Ferlan <jferlan(a)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
7:40 a.m.
New subject: [libvirt] [PATCH 3/3] remote: Handle xdr char ** data return fields more consistently
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(a)redhat.com>
---
src/remote/remote_daemon_dispatch.c | 25 +++++++------------------
1 file changed, 7 insertions(+), 18 deletions(-)
ACK
Michal
2202
days inactive
2203
days old
8 comments
3 participants
participants (3)
-
John Ferlan -
Michal Privoznik -
Peter Krempa