On Thu, Jul 11, 2013 at 08:34:01AM -0400, John Ferlan wrote: ACK 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 :|

Recent changes uncovered a NEGATIVE_RETURNS in the return from sysconf() when processing a for loop in virtTestCaptureProgramExecChild() --- tests/testutils.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/tests/testutils.c b/tests/testutils.c index ec0fe52..2fdf7b8 100644 --- a/tests/testutils.c +++ b/tests/testutils.c @@ -281,6 +281,9 @@ void virtTestCaptureProgramExecChild(const char *const argv[], goto cleanup; open_max = sysconf(_SC_OPEN_MAX); + if (open_max < 0) + goto cleanup; + for (i = 0; i < open_max; i++) { if (i != stdinfd && i != pipefd) { -- 1.8.1.4

Recent changes uncovered a pair of NEGATIVE_RETURNS when processing the 'nnames' in 'vshDomainListCollect' in the for loop due to possible -1 value. --- tools/virsh-domain-monitor.c | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/tools/virsh-domain-monitor.c b/tools/virsh-domain-monitor.c index 7af765e..bd853ba 100644 --- a/tools/virsh-domain-monitor.c +++ b/tools/virsh-domain-monitor.c @@ -1399,6 +1399,7 @@ vshDomainListCollect(vshControl *ctl, unsigned int flags) vshDomainListPtr list = vshMalloc(ctl, sizeof(*list)); size_t i; int ret; + int rc; int *ids = NULL; int nids = 0; char **names = NULL; @@ -1467,16 +1468,17 @@ fallback: if (!VSH_MATCH(VIR_CONNECT_LIST_DOMAINS_FILTERS_ACTIVE) || VSH_MATCH(VIR_CONNECT_LIST_DOMAINS_INACTIVE)) { - if ((nnames = virConnectNumOfDefinedDomains(ctl->conn)) < 0) { + if ((rc = virConnectNumOfDefinedDomains(ctl->conn)) < 0) { vshError(ctl, "%s", _("Failed to list inactive domains")); goto cleanup; } - if (nnames) { + if (rc) { + nnames = rc; names = vshMalloc(ctl, sizeof(char *) * nnames); - if ((nnames = virConnectListDefinedDomains(ctl->conn, names, - nnames)) < 0) { + if ((rc = virConnectListDefinedDomains(ctl->conn, names, + nnames)) < 0) { vshError(ctl, "%s", _("Failed to list inactive domains")); goto cleanup; } -- 1.8.1.4

Recent changes uncovered FORWARD_NULL and NEGATIVE_RETURNS problems with the processing of the 'nActiveIfaces' and 'nInactiveIfaces' and their associated allocated arrays in 'vshInterfaceListCollect' due to the possibility of returning -1 in a call and using the return value as a for loop index end condition. --- tools/virsh-interface.c | 24 +++++++++++++----------- 1 file changed, 13 insertions(+), 11 deletions(-) diff --git a/tools/virsh-interface.c b/tools/virsh-interface.c index 9fdd36e..5ee16a3 100644 --- a/tools/virsh-interface.c +++ b/tools/virsh-interface.c @@ -199,6 +199,7 @@ vshInterfaceListCollect(vshControl *ctl, int nActiveIfaces = 0; int nInactiveIfaces = 0; int nAllIfaces = 0; + int rc; /* try the list with flags support (0.10.2 and later) */ if ((ret = virConnectListAllInterfaces(ctl->conn, @@ -222,16 +223,17 @@ fallback: vshResetLibvirtError(); if (flags & VIR_CONNECT_LIST_INTERFACES_ACTIVE) { - nActiveIfaces = virConnectNumOfInterfaces(ctl->conn); - if (nActiveIfaces < 0) { + rc = virConnectNumOfInterfaces(ctl->conn); + if (rc < 0) { vshError(ctl, "%s", _("Failed to list active interfaces")); goto cleanup; } - if (nActiveIfaces) { + if (rc) { + nActiveIfaces = rc; activeNames = vshMalloc(ctl, sizeof(char *) * nActiveIfaces); - if ((nActiveIfaces = virConnectListInterfaces(ctl->conn, activeNames, - nActiveIfaces)) < 0) { + if ((rc = virConnectListInterfaces(ctl->conn, activeNames, + nActiveIfaces)) < 0) { vshError(ctl, "%s", _("Failed to list active interfaces")); goto cleanup; } @@ -239,17 +241,17 @@ fallback: } if (flags & VIR_CONNECT_LIST_INTERFACES_INACTIVE) { - nInactiveIfaces = virConnectNumOfDefinedInterfaces(ctl->conn); - if (nInactiveIfaces < 0) { + rc = virConnectNumOfDefinedInterfaces(ctl->conn); + if (rc < 0) { vshError(ctl, "%s", _("Failed to list inactive interfaces")); goto cleanup; } - if (nInactiveIfaces) { + if (rc) { + nInactiveIfaces = rc; inactiveNames = vshMalloc(ctl, sizeof(char *) * nInactiveIfaces); - if ((nInactiveIfaces = - virConnectListDefinedInterfaces(ctl->conn, inactiveNames, - nInactiveIfaces)) < 0) { + if ((rc = virConnectListDefinedInterfaces(ctl->conn, inactiveNames, + nInactiveIfaces)) < 0) { vshError(ctl, "%s", _("Failed to list inactive interfaces")); goto cleanup; } -- 1.8.1.4

Recent changes uncovered FORWARD_NULL and NEGATIVE_RETURNS problems with the processing of the 'ndevices' and its associated allocated arrays in 'vshNodeDeviceListCollect' due to the possibility of returning -1 in a call and using the returned value as a for loop index end condition. --- tools/virsh-nodedev.c | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/tools/virsh-nodedev.c b/tools/virsh-nodedev.c index 2eb1979..d22e1c6 100644 --- a/tools/virsh-nodedev.c +++ b/tools/virsh-nodedev.c @@ -234,6 +234,7 @@ vshNodeDeviceListCollect(vshControl *ctl, size_t deleted = 0; int ndevices = 0; char **names = NULL; + int rc; /* try the list with flags support (0.10.2 and later) */ if ((ret = virConnectListAllNodeDevices(ctl->conn, @@ -256,19 +257,20 @@ fallback: /* fall back to old method (0.10.1 and older) */ vshResetLibvirtError(); - ndevices = virNodeNumOfDevices(ctl->conn, NULL, 0); - if (ndevices < 0) { + rc = virNodeNumOfDevices(ctl->conn, NULL, 0); + if (rc < 0) { vshError(ctl, "%s", _("Failed to count node devices")); goto cleanup; } - if (ndevices == 0) + if (rc == 0) return list; + ndevices = rc; names = vshMalloc(ctl, sizeof(char *) * ndevices); - ndevices = virNodeListDevices(ctl->conn, NULL, names, ndevices, 0); - if (ndevices < 0) { + rc = virNodeListDevices(ctl->conn, NULL, names, ndevices, 0); + if (rc < 0) { vshError(ctl, "%s", _("Failed to list node devices")); goto cleanup; } -- 1.8.1.4

The switch statement in 'virStorageBackendCreateQemuImgOpts' used the for loop end condition 'VIR_STORAGE_FILE_FEATURE_LAST' as a possible value, but since that cannot happen Coverity spits out a DEADCODE message. Adding the Coverity tag just removes the Coverity message --- src/storage/storage_backend.c | 1 + 1 file changed, 1 insertion(+) diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c index 5d06eb6..8d5880e 100644 --- a/src/storage/storage_backend.c +++ b/src/storage/storage_backend.c @@ -663,6 +663,7 @@ virStorageBackendCreateQemuImgOpts(char **opts, } break; + /* coverity[dead_error_begin] */ case VIR_STORAGE_FILE_FEATURE_LAST: ; } -- 1.8.1.4

On Thu, Jul 11, 2013 at 08:34:06AM -0400, John Ferlan wrote: Wow, I tend to think that this code is terribly over engineered. We really don't need to have a for loop with a nested case. The entire thing can be replaced with something much simpler. 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 Thu, Jul 11, 2013 at 10:48:50AM -0400, John Ferlan wrote: Well, ok, lets go with your first patch for now. ACK 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 Thu, Jul 11, 2013 at 08:34:07AM -0400, John Ferlan wrote: Slight preference for just changing the termination condition in the for() loop to include "last_processed_hostdev_vf != -1" 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 :|