On 09/05/14 00:26, John Ferlan wrote: wow, this was the real bug here. The original code would use the "field" value of the first element in the params array. Very easy to overlook. ACK. Peter

On 09/05/14 00:26, John Ferlan wrote: This unfortunately breaks the remote driver impl of GetAllDomainStats. As it seems that the limit parameter isn't used for automatically allocated arrays and I expected that it is I'll need either fix the remote impl of the stats function or add support for checking the limit here. And I probably prefer option 2, fixing remoteDeserializeTypedParameters to use limit correctly even for auto-alloced typed parameters. Peter

Since cd4d547576a4f0371d1d4d4e0ca6db124c5ba257 Coverity notes that setting 'ret = -3' prior to the unconditional setting of 'ret = 0' will cause the value to be UNUSED. Since the comment indicates that it is expect to allow the code to continue, just remove the ret = -3 setting. Signed-off-by: John Ferlan <jferlan(a)redhat.com&gt; --- src/storage/storage_backend_fs.c | 1 - 1 file changed, 1 deletion(-) diff --git a/src/storage/storage_backend_fs.c b/src/storage/storage_backend_fs.c index b8f907a..0f8ceee 100644 --- a/src/storage/storage_backend_fs.c +++ b/src/storage/storage_backend_fs.c @@ -128,7 +128,6 @@ virStorageBackendProbeTarget(virStorageSourcePtr target, virReportError(VIR_ERR_INTERNAL_ERROR, _("cannot probe backing volume format: %s"), target->backingStore->path); - ret = -3; } else { target->backingStore->format = rc; } -- 1.9.3

Handle a few places where Coverity complains about the value being unused. For two of them (Close cases) - the comments above the close indicate there is no harm to ignore the error - so added an ignore_value. For the other condition, added an rc check like other callers. Signed-off-by: John Ferlan <jferlan(a)redhat.com&gt; --- src/vbox/vbox_common.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/src/vbox/vbox_common.c b/src/vbox/vbox_common.c index b9858ee..10a3205 100644 --- a/src/vbox/vbox_common.c +++ b/src/vbox/vbox_common.c @@ -4340,6 +4340,11 @@ static int vboxCloseDisksRecursively(virDomainPtr dom, char *location) PRUnichar *childLocationUtf = NULL; char *childLocation = NULL; rc = gVBoxAPI.UIMedium.GetLocation(childMedium, &childLocationUtf); + if (NS_FAILED(rc)) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Unable to get childMedium location")); + goto cleanup; + } VBOX_UTF16_TO_UTF8(childLocationUtf, &childLocation); VBOX_UTF16_FREE(childLocationUtf); if (vboxCloseDisksRecursively(dom, childLocation) < 0) { @@ -4762,7 +4767,7 @@ vboxSnapshotRedefine(virDomainPtr dom, * succeed, unless there is still another machine which uses the * medium. No harm done if we ignore the error. */ - rc = gVBoxAPI.UIMedium.Close(medium); + ignore_value(gVBoxAPI.UIMedium.Close(medium)); } VBOX_UTF8_FREE(locationUtf8); } @@ -6982,7 +6987,7 @@ vboxDomainSnapshotDeleteMetadataOnly(virDomainSnapshotPtr snapshot) * reference in a sane order, which means that closing will normally * succeed, unless there is still another machine which uses the * medium. No harm done if we ignore the error. */ - rc = gVBoxAPI.UIMedium.Close(medium); + ignore_value(gVBoxAPI.UIMedium.Close(medium)); } VBOX_UTF16_FREE(locationUtf16); VBOX_UTF8_FREE(locationUtf8); -- 1.9.3

Coverity complains that checking for !domlist after setting doms = domlist and making a deref of doms just above It seems the call in question was intended to me made in the case that 'doms' was passed in and not when the virDomainObjListExport() call allocated domlist and already called virConnectGetAllDomainStatsCheckACL(). Thus rather than check for !domlist - check that "doms != domlist" in order to avoid the Coverity message. Signed-off-by: John Ferlan <jferlan(a)redhat.com&gt; --- src/qemu/qemu_driver.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index f52f00d..362d1ab 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -17363,7 +17363,7 @@ qemuConnectGetAllDomainStats(virConnectPtr conn, if (!(dom = qemuDomObjFromDomain(doms[i]))) continue; - if (!domlist && + if (doms != domlist && !virConnectGetAllDomainStatsCheckACL(conn, dom->def)) continue; -- 1.9.3

Coverity complains that when multiplying to 32 bit values that eventually will be stored in a 64 bit value that it's possible the math could overflow unless one of the values being multiplied is type cast to the proper size. Signed-off-by: John Ferlan <jferlan(a)redhat.com&gt; --- src/storage/storage_backend_disk.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/storage/storage_backend_disk.c b/src/storage/storage_backend_disk.c index cb6a8d5..abab1e1 100644 --- a/src/storage/storage_backend_disk.c +++ b/src/storage/storage_backend_disk.c @@ -560,7 +560,7 @@ virStorageBackendDiskPartBoundaries(virStoragePoolObjPtr pool, unsigned long long extraBytes = 0; unsigned long long alignedAllocation = allocation; virStoragePoolSourceDevicePtr dev = &pool->def->source.devices[0]; - unsigned long long cylinderSize = dev->geometry.heads * + unsigned long long cylinderSize = (unsigned long long)dev->geometry.heads * dev->geometry.sectors * SECTOR_SIZE; VIR_DEBUG("find free area: allocation %llu, cyl size %llu", allocation, -- 1.9.3

Since 0766783abbe8bbc9ea686c2c3149f4c0ac139e19 Coverity complains that the EDIT_FREE definition results in DEADCODE. As it turns out with the change to use the EDIT_FREE macro the call to vir*Free() wouldn't be necessary nor would it happen... Prior code to above commitid would : vir*Ptr foo = NULL; ... foo = vir*GetXMLDesc() ... vir*Free(foo); foo = vir*DefineXML() ... And thus the free was needed. With the change to use EDIT_FREE the same code changed to: vir*Ptr foo = NULL; vir*Ptr foo_edited = NULL; ... foo = vir*GetXMLDesc() ... if (foo_edited) vir*Free(foo_edited); foo_edited = vir*DefineXML() ... However, foo_edited could never be set in the code path - even with all the goto's since the only way for it to be set is if vir*DefineXML() succeeds in which case the code to allow a retry (and thus all the goto's) never leaves foo_edited set All error paths lead to "cleanup:" which causes both foo and foo_edited to call the respective vir*Free() routines if set. Signed-off-by: John Ferlan <jferlan(a)redhat.com&gt; --- tools/virsh-domain.c | 5 ----- tools/virsh-edit.c | 9 --------- tools/virsh-interface.c | 3 --- tools/virsh-network.c | 3 --- tools/virsh-nwfilter.c | 3 --- tools/virsh-pool.c | 3 --- tools/virsh-snapshot.c | 3 --- 7 files changed, 29 deletions(-) diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index 68d49d6..1cdb596 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -4032,7 +4032,6 @@ cmdSaveImageEdit(vshControl *ctl, const vshCmd *cmd) ret = true; goto edit_cleanup; #define EDIT_DEFINE \ (virDomainSaveImageDefineXML(ctl->conn, file, doc_edited, define_flags) == 0) -#define EDIT_FREE /* */ #include "virsh-edit.c" vshPrint(ctl, _("State file %s edited.\n"), file); @@ -7162,7 +7161,6 @@ cmdMetadata(vshControl *ctl, const vshCmd *cmd) #define EDIT_DEFINE \ (virDomainSetMetadata(dom, VIR_DOMAIN_METADATA_ELEMENT, doc_edited, \ key, uri, flags) == 0) -#define EDIT_FREE /* nothing */ #include "virsh-edit.c" vshPrint("%s\n", _("Metadata modified")); @@ -10656,9 +10654,6 @@ cmdEdit(vshControl *ctl, const vshCmd *cmd) ret = true; goto edit_cleanup; #define EDIT_DEFINE \ (dom_edited = virDomainDefineXML(ctl->conn, doc_edited)) -#define EDIT_FREE \ - if (dom_edited) \ - virDomainFree(dom_edited); #include "virsh-edit.c" vshPrint(ctl, _("Domain %s XML configuration edited.\n"), diff --git a/tools/virsh-edit.c b/tools/virsh-edit.c index 5d35c55..10298f6 100644 --- a/tools/virsh-edit.c +++ b/tools/virsh-edit.c @@ -40,9 +40,6 @@ * For example: * #define EDIT_DEFINE (dom_edited = virDomainDefineXML(ctl->conn, doc_edited)) * - * EDIT_FREE - statement which vir*Free()-s object defined by EDIT_DEFINE, e.g: - * #define EDIT_FREE if (dom_edited) virDomainFree(dom_edited); - * * Michal Privoznik <mprivozn(a)redhat.com&gt; */ @@ -58,10 +55,6 @@ # error Missing EDIT_DEFINE definition #endif -#ifndef EDIT_FREE -# error Missing EDIT_FREE definition -#endif - do { char *tmp = NULL; char *doc = NULL; @@ -116,7 +109,6 @@ do { } /* Everything checks out, so redefine the object. */ - EDIT_FREE; if (!msg && !(EDIT_DEFINE)) { msg = _("Failed."); } @@ -162,4 +154,3 @@ do { #undef EDIT_GET_XML #undef EDIT_NOT_CHANGED #undef EDIT_DEFINE -#undef EDIT_FREE diff --git a/tools/virsh-interface.c b/tools/virsh-interface.c index d4ec854..6cacaf1 100644 --- a/tools/virsh-interface.c +++ b/tools/virsh-interface.c @@ -128,9 +128,6 @@ cmdInterfaceEdit(vshControl *ctl, const vshCmd *cmd) ret = true; goto edit_cleanup; #define EDIT_DEFINE \ (iface_edited = virInterfaceDefineXML(ctl->conn, doc_edited, 0)) -#define EDIT_FREE \ - if (iface_edited) \ - virInterfaceFree(iface_edited); #include "virsh-edit.c" vshPrint(ctl, _("Interface %s XML configuration edited.\n"), diff --git a/tools/virsh-network.c b/tools/virsh-network.c index 9497472..5fe4b32 100644 --- a/tools/virsh-network.c +++ b/tools/virsh-network.c @@ -1111,9 +1111,6 @@ cmdNetworkEdit(vshControl *ctl, const vshCmd *cmd) ret = true; goto edit_cleanup; #define EDIT_DEFINE \ (network_edited = virNetworkDefineXML(ctl->conn, doc_edited)) -#define EDIT_FREE \ - if (network_edited) \ - virNetworkFree(network_edited); #include "virsh-edit.c" vshPrint(ctl, _("Network %s XML configuration edited.\n"), diff --git a/tools/virsh-nwfilter.c b/tools/virsh-nwfilter.c index 6e6e21b..ca54c9d 100644 --- a/tools/virsh-nwfilter.c +++ b/tools/virsh-nwfilter.c @@ -428,9 +428,6 @@ cmdNWFilterEdit(vshControl *ctl, const vshCmd *cmd) ret = true; goto edit_cleanup; #define EDIT_DEFINE \ (nwfilter_edited = virNWFilterDefineXML(ctl->conn, doc_edited)) -#define EDIT_FREE \ - if (nwfilter_edited) \ - virNWFilterFree(nwfilter); #include "virsh-edit.c" vshPrint(ctl, _("Network filter %s XML configuration edited.\n"), diff --git a/tools/virsh-pool.c b/tools/virsh-pool.c index 80313b1..0b8dba5 100644 --- a/tools/virsh-pool.c +++ b/tools/virsh-pool.c @@ -1767,9 +1767,6 @@ cmdPoolEdit(vshControl *ctl, const vshCmd *cmd) ret = true; goto edit_cleanup; #define EDIT_DEFINE \ (pool_edited = virStoragePoolDefineXML(ctl->conn, doc_edited, 0)) -#define EDIT_FREE \ - if (pool_edited) \ - virStoragePoolFree(pool_edited); #include "virsh-edit.c" vshPrint(ctl, _("Pool %s XML configuration edited.\n"), diff --git a/tools/virsh-snapshot.c b/tools/virsh-snapshot.c index 3ecb3de..885ad22 100644 --- a/tools/virsh-snapshot.c +++ b/tools/virsh-snapshot.c @@ -589,9 +589,6 @@ cmdSnapshotEdit(vshControl *ctl, const vshCmd *cmd) (strstr(doc, "<state>disk-snapshot</state>") ? \ define_flags |= VIR_DOMAIN_SNAPSHOT_CREATE_DISK_ONLY : 0), \ edited = virDomainSnapshotCreateXML(dom, doc_edited, define_flags) -#define EDIT_FREE \ - if (edited) \ - virDomainSnapshotFree(edited); #include "virsh-edit.c" edited_name = virDomainSnapshotGetName(edited); -- 1.9.3

Adjust the parentheses in/for the waitpid loops; otherwise, Coverity points out: (1) Event assignment: Assigning: "waitret" = "waitpid(pid, &status, 0) == -1" (2) Event between: At condition "waitret == -1", the value of "waitret" must be between 0 and 1. (3) Event dead_error_condition: The condition "waitret == -1" cannot be true. (4) Event dead_error_begin: Execution cannot reach this statement: "ret = -*__errno_location();". Signed-off-by: John Ferlan <jferlan(a)redhat.com&gt; --- src/util/virfile.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/src/util/virfile.c b/src/util/virfile.c index cfb6cc1..b602144 100644 --- a/src/util/virfile.c +++ b/src/util/virfile.c @@ -2072,8 +2072,7 @@ virFileOpenForked(const char *path, int openflags, mode_t mode, } /* wait for child to complete, and retrieve its exit code */ - while ((waitret = waitpid(pid, &status, 0) == -1) - && (errno == EINTR)); + while ((waitret = waitpid(pid, &status, 0)) == -1 && (errno == EINTR)); if (waitret == -1) { ret = -errno; virReportSystemError(errno, @@ -2290,7 +2289,7 @@ virDirCreate(const char *path, if (pid) { /* parent */ /* wait for child to complete, and retrieve its exit code */ VIR_FREE(groups); - while ((waitret = waitpid(pid, &status, 0) == -1) && (errno == EINTR)); + while ((waitret = waitpid(pid, &status, 0)) == -1 && (errno == EINTR)); if (waitret == -1) { ret = -errno; virReportSystemError(errno, -- 1.9.3

Coverity points out that by using EMPTYSTR(type) we are guarding against the possibility that it could be NULL; however, based on how 'type' was initialized to NULL, then either "ipv4", "ipv6", or "" - there is no way it could be NULL. Since "-" is supposed to mean something empty in a field - remove the initialization to NULL and use it as the ending else rather than using "". Also changed the name from 'type' to 'typestr'. Signed-off-by: John Ferlan <jferlan(a)redhat.com&gt; --- tools/virsh-network.c | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/tools/virsh-network.c b/tools/virsh-network.c index 5fe4b32..f505c14 100644 --- a/tools/virsh-network.c +++ b/tools/virsh-network.c @@ -1360,7 +1360,7 @@ cmdNetworkDHCPLeases(vshControl *ctl, const vshCmd *cmd) "---------------------------------------------------------"); for (i = 0; i < nleases; i++) { - const char *type = NULL; + const char *typestr; char *cidr_format = NULL; virNetworkDHCPLeasePtr lease = leases[i]; time_t expirytime_tmp = lease->expirytime; @@ -1369,14 +1369,15 @@ cmdNetworkDHCPLeases(vshControl *ctl, const vshCmd *cmd) ts = *localtime_r(&expirytime_tmp, &ts); strftime(expirytime, sizeof(expirytime), "%Y-%m-%d %H:%M:%S", &ts); - type = (lease->type == VIR_IP_ADDR_TYPE_IPV4) ? "ipv4" : - (lease->type == VIR_IP_ADDR_TYPE_IPV6) ? "ipv6" : ""; + typestr = (lease->type == VIR_IP_ADDR_TYPE_IPV4) ? "ipv4" : + (lease->type == VIR_IP_ADDR_TYPE_IPV6) ? "ipv6" : NULL; ignore_value(virAsprintf(&cidr_format, "%s/%d", lease->ipaddr, lease->prefix)); vshPrintExtra(ctl, " %-20s %-18s %-9s %-25s %-15s %s\n", - expirytime, EMPTYSTR(lease->mac), EMPTYSTR(type), cidr_format, + expirytime, EMPTYSTR(lease->mac), + EMPTYSTR(typestr), cidr_format, EMPTYSTR(lease->hostname), EMPTYSTR(lease->clientid)); } -- 1.9.3

Add another 'dead_code_begin' - victims of our own coding practices Signed-off-by: John Ferlan <jferlan(a)redhat.com&gt; --- src/qemu/qemu_command.c | 1 + 1 file changed, 1 insertion(+) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 1ca98fb..6bba4a4 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -6297,6 +6297,7 @@ qemuBuildCpuArgStr(virQEMUDriverPtr driver, virBufferAddLit(&buf, ",kvm=off"); break; + /* coverity[dead_error_begin] */ case VIR_DOMAIN_KVM_LAST: break; } -- 1.9.3

Coverity complains that the various checks for autoincrement and changed variables are DEADCODE - seems to me to be a false positive - so mark it. Signed-off-by: John Ferlan <jferlan(a)redhat.com&gt; --- tests/virstringtest.c | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/tests/virstringtest.c b/tests/virstringtest.c index 7c25b22..10fad2c 100644 --- a/tests/virstringtest.c +++ b/tests/virstringtest.c @@ -162,10 +162,12 @@ testStrdup(const void *data ATTRIBUTE_UNUSED) virFilePrintf(stderr, "unexpected strdup result %d, expected 1\n", value); goto cleanup; } + /* coverity[dead_error_begin] */ if (i != 1) { virFilePrintf(stderr, "unexpected side effects i=%zu, expected 1\n", i); goto cleanup; } + /* coverity[dead_error_begin] */ if (j != 1) { virFilePrintf(stderr, "unexpected side effects j=%zu, expected 1\n", j); goto cleanup; @@ -182,14 +184,17 @@ testStrdup(const void *data ATTRIBUTE_UNUSED) virFilePrintf(stderr, "unexpected strdup result %d, expected 0\n", value); goto cleanup; } + /* coverity[dead_error_begin] */ if (i != 2) { virFilePrintf(stderr, "unexpected side effects i=%zu, expected 2\n", i); goto cleanup; } + /* coverity[dead_error_begin] */ if (j != 2) { virFilePrintf(stderr, "unexpected side effects j=%zu, expected 2\n", j); goto cleanup; } + /* coverity[dead_error_begin] */ if (k != 1) { virFilePrintf(stderr, "unexpected side effects k=%zu, expected 1\n", k); goto cleanup; -- 1.9.3

Coverity points out that if 'dom' isn't returned from virDomainQemuAttach, then the code already jumps to cleanup, so there was no need for the subsequent if (dom != NULL) check. I moved the error message about failure into the goto cleanup on failure and then removed the if (dom != NULL) Signed-off-by: John Ferlan <jferlan(a)redhat.com&gt; --- tools/virsh-domain.c | 16 +++++++--------- 1 file changed, 7 insertions(+), 9 deletions(-) diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index 1cdb596..f732a6e 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -8260,18 +8260,16 @@ cmdQemuAttach(vshControl *ctl, const vshCmd *cmd) goto cleanup; } - if (!(dom = virDomainQemuAttach(ctl->conn, pid_value, flags))) - goto cleanup; - - if (dom != NULL) { - vshPrint(ctl, _("Domain %s attached to pid %u\n"), - virDomainGetName(dom), pid_value); - virDomainFree(dom); - ret = true; - } else { + if (!(dom = virDomainQemuAttach(ctl->conn, pid_value, flags))) { vshError(ctl, _("Failed to attach to pid %u"), pid_value); + goto cleanup; } + vshPrint(ctl, _("Domain %s attached to pid %u\n"), + virDomainGetName(dom), pid_value); + virDomainFree(dom); + ret = true; + cleanup: return ret; } -- 1.9.3

If the virJSONValueNewObject() fails, then rather than going to error and getting a Coverity false positive since it doesn't seem to understand the relationship between nkeywords, keywords, and values and seems to believe calling qemuFreeKeywords will cause a NULL deref - just return NULL Signed-off-by: John Ferlan <jferlan(a)redhat.com&gt; --- src/qemu/qemu_monitor_json.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index 62e7d5d..b0bc5fc 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -617,7 +617,7 @@ qemuMonitorJSONKeywordStringToJSON(const char *str, const char *firstkeyword) size_t i; if (!(ret = virJSONValueNewObject())) - goto error; + return NULL; if (qemuParseKeywords(str, &keywords, &values, &nkeywords, 1) < 0) goto error; -- 1.9.3

On 09/09/2014 03:36 AM, Wang Rui wrote: Yes, but for some reason once qemuParseKeywords() is called Coverity has "resolved" that things will be OK. This one is a false positive and perhaps a bug in the Coverity parser. The return NULL is the way to avoid the condition. Filing a Coverity bug is an option, but changing the logic is just quicker. I think it gets flagged because Coverity perhaps prescans the code and "flags" all callers to qemuFreeKeywords as potentially having a FORWARD_NULL. It doesn't seem to pick up the fact that nkeywords is initialized to 0 (along with keywords and values being set to NULL). It's perhaps focusing on the array addresses rather than the logic of using nkeywords to access those arrays. John

If we jump to cleanup before allocating 'result', then the call to virBlkioDeviceArrayClear() could dereference result Signed-off-by: John Ferlan <jferlan(a)redhat.com&gt; --- src/lxc/lxc_driver.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c index f93360f..e5b6662 100644 --- a/src/lxc/lxc_driver.c +++ b/src/lxc/lxc_driver.c @@ -2222,8 +2222,10 @@ lxcDomainParseBlkioDeviceStr(char *blkioDeviceStr, const char *type, _("unable to parse blkio device '%s' '%s'"), type, blkioDeviceStr); cleanup: - virBlkioDeviceArrayClear(result, ndevices); - VIR_FREE(result); + if (result) { + virBlkioDeviceArrayClear(result, ndevices); + VIR_FREE(result); + } return -1; } -- 1.9.3

If we jump to cleanup before allocating the 'result', then the call to virBlkioDeviceArrayClear will deref result causing a problem. Signed-off-by: John Ferlan <jferlan(a)redhat.com&gt; --- src/qemu/qemu_driver.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 362d1ab..373b4d7 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -7682,8 +7682,10 @@ qemuDomainParseBlkioDeviceStr(char *blkioDeviceStr, const char *type, _("unable to parse blkio device '%s' '%s'"), type, blkioDeviceStr); cleanup: - virBlkioDeviceArrayClear(result, ndevices); - VIR_FREE(result); + if (result) { + virBlkioDeviceArrayClear(result, ndevices); + VIR_FREE(result); + } return -1; } -- 1.9.3

If the VIR_STRDUP(exptime,...) fails, then we will jump to cleanup, no need to check if exptime is set which causes Coverity to issue a complaint in the virStrToLong_ll call because there wasn't a check for a NULL value while there was one for the reference right after the VIR_STRDUP(). Signed-off-by: John Ferlan <jferlan(a)redhat.com&gt; --- src/network/leaseshelper.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/network/leaseshelper.c b/src/network/leaseshelper.c index c8543a2..5b3c9c3 100644 --- a/src/network/leaseshelper.c +++ b/src/network/leaseshelper.c @@ -180,8 +180,7 @@ main(int argc, char **argv) goto cleanup; /* Removed extraneous trailing space in DNSMASQ_LEASE_EXPIRES (dnsmasq < 2.52) */ - if (exptime && - exptime[strlen(exptime) - 1] == ' ') + if (exptime[strlen(exptime) - 1] == ' ') exptime[strlen(exptime) - 1] = '\0'; /* Check if it is an IPv6 lease */ -- 1.9.3

Perhaps a false positive, but since Coverity doesn't understand the relationship between the 'count' and the 'strings', rather than leave the chance the on input 'strings' is NULL and causes a deref - just check for it and return Signed-off-by: John Ferlan <jferlan(a)redhat.com&gt; --- src/util/virstring.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/util/virstring.c b/src/util/virstring.c index b14f785..935f0c6 100644 --- a/src/util/virstring.c +++ b/src/util/virstring.c @@ -198,6 +198,9 @@ virStringFreeListCount(char **strings, { size_t i; + if (!strings) + return; + for (i = 0; i < count; i++) VIR_FREE(strings[i]); -- 1.9.3

If the qemuMigrationEatCookie() fails to set mig, we jump to cleanup: which will call qemuMigrationCancelDriveMirror() without first checking if mig == NULL Signed-off-by: John Ferlan <jferlan(a)redhat.com&gt; --- src/qemu/qemu_migration.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index 9cfb77e..d6b0f29 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -3428,7 +3428,8 @@ qemuMigrationRun(virQEMUDriverPtr driver, orig_err = virSaveLastError(); /* cancel any outstanding NBD jobs */ - qemuMigrationCancelDriveMirror(mig, driver, vm); + if (mig) + qemuMigrationCancelDriveMirror(mig, driver, vm); if (spec->fwdType != MIGRATION_FWD_DIRECT) { if (iothread && qemuMigrationStopTunnel(iothread, ret < 0) < 0) -- 1.9.3

The code compares def->forwarders when deciding to return 0 at a couple of points, then uses "def->nfwds" as a way to index into the def->forwarders array. That reference results in Coverity complaining that def->forwarders being NULL was checked as part of an arithmetic OR operation where failure could be any one 5 conditions, but that is not checked when entering the loop to dereference the array. Changing the comparisons to use nfwds will clear the warnings Signed-off-by: John Ferlan <jferlan(a)redhat.com&gt; --- src/conf/network_conf.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c index 9571ee1..f013d6b 100644 --- a/src/conf/network_conf.c +++ b/src/conf/network_conf.c @@ -2360,7 +2360,7 @@ virNetworkDNSDefFormat(virBufferPtr buf, { size_t i, j; - if (!(def->forwardPlainNames || def->forwarders || def->nhosts || + if (!(def->forwardPlainNames || def->nfwds || def->nhosts || def->nsrvs || def->ntxts)) return 0; @@ -2376,7 +2376,7 @@ virNetworkDNSDefFormat(virBufferPtr buf, return -1; } virBufferAsprintf(buf, " forwardPlainNames='%s'", fwd); - if (!(def->forwarders || def->nhosts || def->nsrvs || def->ntxts)) { + if (!(def->nfwds || def->nhosts || def->nsrvs || def->ntxts)) { virBufferAddLit(buf, "/>\n"); return 0; } -- 1.9.3

In qemuProcessInitPCIAddresses() if qemuMonitorGetAllPCIAddresses() returns a negative (or zero) value, then no need to call the qemuProcessDetectPCIAddresses(). Signed-off-by: John Ferlan <jferlan(a)redhat.com&gt; --- src/qemu/qemu_process.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 38ed3fe..e48a6a1 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -2721,7 +2721,7 @@ qemuProcessInitPCIAddresses(virQEMUDriverPtr driver, { qemuDomainObjPrivatePtr priv = vm->privateData; int naddrs; - int ret; + int ret = -1; qemuMonitorPCIAddress *addrs = NULL; if (qemuDomainObjEnterMonitorAsync(driver, vm, asyncJob) < 0) @@ -2730,7 +2730,8 @@ qemuProcessInitPCIAddresses(virQEMUDriverPtr driver, &addrs); qemuDomainObjExitMonitor(driver, vm); - ret = qemuProcessDetectPCIAddresses(vm, addrs, naddrs); + if (naddrs > 0) + ret = qemuProcessDetectPCIAddresses(vm, addrs, naddrs); VIR_FREE(addrs); -- 1.9.3

If the virNumaGetNodeCPUs() call fails with -1, then jumping to cleanup with 'cpus == NULL' and calling virCapabilitiesClearHostNUMACellCPUTopology will cause issues. Signed-off-by: John Ferlan <jferlan(a)redhat.com&gt; --- src/nodeinfo.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/nodeinfo.c b/src/nodeinfo.c index 92a3718..2008ade 100644 --- a/src/nodeinfo.c +++ b/src/nodeinfo.c @@ -1933,7 +1933,7 @@ nodeCapsInitNUMA(virCapsPtr caps) ret = 0; cleanup: - if (topology_failed || ret < 0) + if ((topology_failed || ret < 0) && cpus) virCapabilitiesClearHostNUMACellCPUTopology(cpus, ncpus); virBitmapFree(cpumap); -- 1.9.3

Coverity notes that if virDomainGetCPUStats returns a negative value into 'nparams' then when we end up at cleanup, the call to virTypedParams will have issues Signed-off-by: John Ferlan <jferlan(a)redhat.com&gt; --- tools/virsh-domain.c | 1 + 1 file changed, 1 insertion(+) diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index f732a6e..6fe637b 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -6650,6 +6650,7 @@ cmdCPUStats(vshControl *ctl, const vshCmd *cmd) return ret; failed_stats: + nparams = 0; vshError(ctl, _("Failed to retrieve CPU statistics for domain '%s'"), virDomainGetName(dom)); goto cleanup; -- 1.9.3

Coverity notes that if the call to virBitmapParse() returns a negative value, then when we jump to the error label, the call to virCapabilitiesClearHostNUMACellCPUTopology() will have issues with the negative nb_cpus Signed-off-by: John Ferlan <jferlan(a)redhat.com&gt; --- src/xen/xend_internal.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/xen/xend_internal.c b/src/xen/xend_internal.c index d9e76fc..f1322c4 100644 --- a/src/xen/xend_internal.c +++ b/src/xen/xend_internal.c @@ -1112,7 +1112,8 @@ sexpr_to_xend_topology(const struct sexpr *root, virCapsPtr caps) parse_error: virReportError(VIR_ERR_XEN_CALL, "%s", _("topology syntax error")); error: - virCapabilitiesClearHostNUMACellCPUTopology(cpuInfo, nb_cpus); + if (nb_cpus > 0) + virCapabilitiesClearHostNUMACellCPUTopology(cpuInfo, nb_cpus); VIR_FREE(cpuInfo); return -1; } -- 1.9.3

Coverity notes that if qemuMonitorGetMachines() returns a negative nmachines value, then the code at the cleanup label will have issues. Signed-off-by: John Ferlan <jferlan(a)redhat.com&gt; --- src/qemu/qemu_capabilities.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index 854a9b8..a652f29 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -2285,7 +2285,7 @@ virQEMUCapsProbeQMPMachineTypes(virQEMUCapsPtr qemuCaps, size_t defIdx = 0; if ((nmachines = qemuMonitorGetMachines(mon, &machines)) < 0) - goto cleanup; + return -1; if (VIR_ALLOC_N(qemuCaps->machineTypes, nmachines) < 0) goto cleanup; -- 1.9.3

Coverity notes that if the virConnectListAllDomains returns a negative value then the loop at the cleanup label that ends on numDomains will have issues. Signed-off-by: John Ferlan <jferlan(a)redhat.com&gt; --- src/qemu/qemu_driver.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 373b4d7..5ceed92 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -1004,9 +1004,11 @@ qemuStateStop(void) ret = -1; cleanup: - for (i = 0; i < numDomains; i++) - virDomainFree(domains[i]); - VIR_FREE(domains); + if (domains) { + for (i = 0; i < numDomains; i++) + virDomainFree(domains[i]); + VIR_FREE(domains); + } VIR_FREE(flags); virObjectUnref(conn); virObjectUnref(cfg); -- 1.9.3

With all the changes in my previous foray into this code, I forgot to remove the libxlDomainEventQueue(driver, event); call inside the dom == NULL condition. Signed-off-by: John Ferlan <jferlan(a)redhat.com&gt; --- src/libxl/libxl_migration.c | 1 - 1 file changed, 1 deletion(-) diff --git a/src/libxl/libxl_migration.c b/src/libxl/libxl_migration.c index b5d8edf..0b562f7 100644 --- a/src/libxl/libxl_migration.c +++ b/src/libxl/libxl_migration.c @@ -527,7 +527,6 @@ libxlDomainMigrationFinish(virConnectPtr dconn, libxlDomainCleanup(driver, vm, VIR_DOMAIN_SHUTOFF_FAILED); event = virDomainEventLifecycleNewFromObj(vm, VIR_DOMAIN_EVENT_STOPPED, VIR_DOMAIN_EVENT_STOPPED_FAILED); - libxlDomainEventQueue(driver, event); } cleanup: -- 1.9.3