[libvirt] [PATCH 0/3] Resolve recent/new Coverity warnings
For virlog.c and commandtest.c - it seems recent changes within the modules have allowed Coverity to dig deeper and find different issues. Not sure why Coverity has that "ability"... Since I was making changes, figured I'd clean up the lxc_controller fd leak as well - these should clean the Jenkins build environment. John Ferlan (3): Coverity: Resolve a CHECKED_RETURN message Coverity: Resolve a FORWARD_NULL Coverity: Resolve a RESOURCE_LEAK src/lxc/lxc_controller.c | 14 ++++++++++---- src/util/virlog.c | 2 +- tests/commandtest.c | 1 + 3 files changed, 12 insertions(+), 5 deletions(-) -- 1.8.5.3
Recent changes to the module seemed to have caused Coverity to find a new issue regarding the failure to check the return from a sendmsg. The code doesn't seem to care about the return status, so just added an ignore_value to keep Coverity quiet. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/util/virlog.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/util/virlog.c b/src/util/virlog.c index d0afd10..056950e 100644 --- a/src/util/virlog.c +++ b/src/util/virlog.c @@ -1003,7 +1003,7 @@ virLogOutputToJournald(virLogSourcePtr source, mh.msg_controllen = cmsg->cmsg_len; - sendmsg(journalfd, &mh, MSG_NOSIGNAL); + ignore_value(sendmsg(journalfd, &mh, MSG_NOSIGNAL)); cleanup: VIR_LOG_CLOSE(buffd); -- 1.8.5.3
On 03/25/2014 12:00 PM, John Ferlan wrote:
Recent changes to the module seemed to have caused Coverity to find a new issue regarding the failure to check the return from a sendmsg. The code doesn't seem to care about the return status, so just added an ignore_value to keep Coverity quiet.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/util/virlog.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
ACK - logging is one of the few places where we don't care if the sendmsg succeeded (what are we going to do if it failed - log a failure message through the failed interface? :)
diff --git a/src/util/virlog.c b/src/util/virlog.c index d0afd10..056950e 100644 --- a/src/util/virlog.c +++ b/src/util/virlog.c @@ -1003,7 +1003,7 @@ virLogOutputToJournald(virLogSourcePtr source,
mh.msg_controllen = cmsg->cmsg_len;
- sendmsg(journalfd, &mh, MSG_NOSIGNAL); + ignore_value(sendmsg(journalfd, &mh, MSG_NOSIGNAL));
cleanup: VIR_LOG_CLOSE(buffd);
-- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Recent changes in the module seemed to have caused Coverity to reanalyze certain parts of the code. Previously the code was modified via commit id '11a11812' to resolve a different error (perhaps DEADCODE). Up through commit id '7b3f1f8c' there were no issues. The new error indicats the 'outbuf' was checked for NULL and then complains because of the dereference. Adding checks for non-NULL prior to the deref resulted in a DEADCODE message. So, resolve using an sa_assert() to keep Coverity quiet especially since it doesn't understand that outbuf will change as a result of a successful virCommandRun() call. Signed-off-by: John Ferlan <jferlan@redhat.com> --- tests/commandtest.c | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/commandtest.c b/tests/commandtest.c index c5c3a9a..7d2161c 100644 --- a/tests/commandtest.c +++ b/tests/commandtest.c @@ -690,6 +690,7 @@ static int test17(const void *unused ATTRIBUTE_UNUSED) goto cleanup; } + sa_assert(outbuf); if (*outbuf) { puts("output buffer is not an allocated empty string"); goto cleanup; -- 1.8.5.3
On 03/25/2014 12:00 PM, John Ferlan wrote:
Recent changes in the module seemed to have caused Coverity to reanalyze certain parts of the code. Previously the code was modified via commit id '11a11812' to resolve a different error (perhaps DEADCODE). Up through commit id '7b3f1f8c' there were no issues.
The new error indicats the 'outbuf' was checked for NULL and then complains because of the dereference. Adding checks for non-NULL prior to the deref resulted in a DEADCODE message.
So, resolve using an sa_assert() to keep Coverity quiet especially since it doesn't understand that outbuf will change as a result of a successful virCommandRun() call.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- tests/commandtest.c | 1 + 1 file changed, 1 insertion(+)
ACK
diff --git a/tests/commandtest.c b/tests/commandtest.c index c5c3a9a..7d2161c 100644 --- a/tests/commandtest.c +++ b/tests/commandtest.c @@ -690,6 +690,7 @@ static int test17(const void *unused ATTRIBUTE_UNUSED) goto cleanup; }
+ sa_assert(outbuf); if (*outbuf) { puts("output buffer is not an allocated empty string"); goto cleanup;
-- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On error the lofd would have been leaked. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/lxc/lxc_controller.c | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/src/lxc/lxc_controller.c b/src/lxc/lxc_controller.c index 800a306..928a43d 100644 --- a/src/lxc/lxc_controller.c +++ b/src/lxc/lxc_controller.c @@ -383,6 +383,7 @@ static int virLXCControllerSetupLoopDeviceDisk(virDomainDiskDefPtr disk) int lofd; char *loname = NULL; const char *src = virDomainDiskGetSource(disk); + int ret = -1; if ((lofd = virFileLoopDeviceAssociate(src, &loname)) < 0) return -1; @@ -395,13 +396,18 @@ static int virLXCControllerSetupLoopDeviceDisk(virDomainDiskDefPtr disk) * the rest of container setup 'just works' */ virDomainDiskSetType(disk, VIR_DOMAIN_DISK_TYPE_BLOCK); - if (virDomainDiskSetSource(disk, loname) < 0) { - VIR_FREE(loname); - return -1; - } + if (virDomainDiskSetSource(disk, loname) < 0) + goto cleanup; + + ret = 0; + + cleanup: VIR_FREE(loname); + if (ret == -1) + VIR_FORCE_CLOSE(lofd); return lofd; + } -- 1.8.5.3
On 03/25/2014 12:00 PM, John Ferlan wrote:
On error the lofd would have been leaked.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/lxc/lxc_controller.c | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-)
+ + cleanup: VIR_FREE(loname); + if (ret == -1)
I might have written 'if (ret < 0)' as that is slightly faster on many machines, but what you have works. ACK. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On Tue, Mar 25, 2014 at 03:00:02PM -0600, Eric Blake wrote:
On 03/25/2014 12:00 PM, John Ferlan wrote:
On error the lofd would have been leaked.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/lxc/lxc_controller.c | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-)
+ + cleanup: VIR_FREE(loname); + if (ret == -1)
I might have written 'if (ret < 0)' as that is slightly faster on many machines, but what you have works.
As a coding style point we use 'ret < 0' in the vast majority of places, so we should prefer that over 'ret == -1' Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On 03/25/2014 02:00 PM, John Ferlan wrote:
For virlog.c and commandtest.c - it seems recent changes within the modules have allowed Coverity to dig deeper and find different issues. Not sure why Coverity has that "ability"... Since I was making changes, figured I'd clean up the lxc_controller fd leak as well - these should clean the Jenkins build environment.
John Ferlan (3): Coverity: Resolve a CHECKED_RETURN message Coverity: Resolve a FORWARD_NULL Coverity: Resolve a RESOURCE_LEAK
src/lxc/lxc_controller.c | 14 ++++++++++---- src/util/virlog.c | 2 +- tests/commandtest.c | 1 + 3 files changed, 12 insertions(+), 5 deletions(-)
Pushed I did change 3/3 to use (ret < 0), my brain was just copying what it saw in virFileLoopDeviceAssociate() Tks, John
participants (3)
-
Daniel P. Berrange -
Eric Blake -
John Ferlan