[libvirt] [PATCH 00/22] Resolve Coverity and build issues
Hopefully the quantity doesn't scare anyone off... These are mostly innocuous, but separated each out to appease the masses. The first patch fixes a recently uncovered Coverity warning regarding FORWARD_NULL of the 'formatStr'... According to an IRC response from pkrempa the format should be set up properly in qemuDomainSnapshotPrepare Patches 2-22 resolve various different build issues in the Coverity environment that sets "lv_cv_static_analysis=yes" during the build. This is 'reproducible' if one sets that as part of their autogen.sh processing - something that we should probably do in the CI build environment so we can get all those ATTRIBUTE_NONNULL checks to be run on a daily basis. John Ferlan (22): qemu: Always format formatStr for blockdev-snapshot-sync util: Remove NONNULL(1) for virNetDevGetName conf: Remove NONNULL(2) for virNetDevBandwidthParse conf: Remove NONNULL(1,2) for virNetDevBandwidthFormat conf: Remove NONNULL(1) for virDomainNumaGetNodeCount network: Remove null newBandwidth check from networkBandwidthUpdate qemu: Remove non null 'vm' check from qemuMonitorOpen qemu: Remove NONNULL(1) for qemu_monitor prototypes util: Change return argument for virBitmapParseUnlimited util: Remove NONNULL(1) for virBitmapParseUnlimited util: Remove NONNULL(2,3) for virHostdevReAttachPCIDevices util: Remove NONNULL(2,3) for virHostdevReAttachUSBDevices util: Remove NONNULL(2,3) for virHostdevReAttachSCSIVHostDevices util: Remove NONNULL(1) for virHostdevPrepareDomainDevices util: Remove NONNULL(1) for virHostdevReAttachDomainDevices util: Remove NONNULL(1) for virNetDevOpenvswitchSetMigrateData util: Remove NONNULL(1,3,4) from virTypedParamsFilter util: Remove NONNULL(2) for virNetDevBandwidthPlug util: Remove NONNULL(1) for virNetDevOpenvswitchGetVhostuserIfname util: Remove NONNULL's for virNetDevVPortProfile[Associate|Disassociate] util: Remove NONNULL(1) for virNetDevMacVLanDeleteWithVPortProfile cpu: Remove NONNULL(1) for cpuBaseline src/conf/netdev_bandwidth_conf.c | 2 +- src/conf/netdev_bandwidth_conf.h | 5 ++--- src/conf/numa_conf.h | 4 ++-- src/cpu/cpu.h | 3 +-- src/network/bridge_driver.c | 2 +- src/qemu/qemu_driver.c | 3 +-- src/qemu/qemu_monitor.c | 3 +-- src/qemu/qemu_monitor.h | 8 ++++---- src/util/virbitmap.c | 28 +++++++++++++--------------- src/util/virbitmap.h | 6 ++---- src/util/virhostdev.c | 9 ++++++--- src/util/virhostdev.h | 10 +++++----- src/util/virnetdev.h | 2 +- src/util/virnetdevbandwidth.h | 3 +-- src/util/virnetdevmacvlan.h | 2 +- src/util/virnetdevopenvswitch.h | 4 ++-- src/util/virnetdevvportprofile.h | 10 ++++------ src/util/virtypedparam.h | 3 +-- tests/testutils.c | 2 +- tests/virbitmaptest.c | 2 +- 20 files changed, 51 insertions(+), 60 deletions(-) -- 2.9.3
The qemuDomainSnapshotPrepare should always set a > 0 format value anyway, so remove the check. Found by Coverity. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_driver.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 0e06508..6762952 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -14072,8 +14072,7 @@ qemuDomainSnapshotCreateSingleDiskActive(virQEMUDriverPtr driver, dd->prepared = true; /* create the actual snapshot */ - if (dd->src->format) - formatStr = virStorageFileFormatTypeToString(dd->src->format); + formatStr = virStorageFileFormatTypeToString(dd->src->format); /* The monitor is only accessed if qemu doesn't support transactions. * Otherwise the following monitor command only constructs the command. -- 2.9.3
On Wed, Mar 22, 2017 at 10:21:14 -0400, John Ferlan wrote:
The qemuDomainSnapshotPrepare should always set a > 0 format value anyway, so remove the check.
Found by Coverity.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_driver.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 0e06508..6762952 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -14072,8 +14072,7 @@ qemuDomainSnapshotCreateSingleDiskActive(virQEMUDriverPtr driver, dd->prepared = true;
/* create the actual snapshot */ - if (dd->src->format) - formatStr = virStorageFileFormatTypeToString(dd->src->format); + formatStr = virStorageFileFormatTypeToString(dd->src->format);
dd->src->format will be always non-zero so formatStr is guaranteed not to return NULL. The value is assigned in qemuDomainSnapshotPrepare The condition is probably useless here.
The 'ifindex' argument is not a pointer, so no need for NONNULL in prototype Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/util/virnetdev.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/util/virnetdev.h b/src/util/virnetdev.h index 01e9c5b..9c9daf1 100644 --- a/src/util/virnetdev.h +++ b/src/util/virnetdev.h @@ -158,7 +158,7 @@ int virNetDevSetName(const char *ifname, const char *newifname) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_RETURN_CHECK; char *virNetDevGetName(int ifindex) - ATTRIBUTE_NONNULL(1) ATTRIBUTE_RETURN_CHECK; + ATTRIBUTE_RETURN_CHECK; int virNetDevGetIndex(const char *ifname, int *ifindex) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_RETURN_CHECK; -- 2.9.3
Since the code checks and handles a NULL 'node' before proceeding there's no need for the prototype with the NONNULL(2). Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/netdev_bandwidth_conf.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/conf/netdev_bandwidth_conf.h b/src/conf/netdev_bandwidth_conf.h index cdeac09..c687427 100644 --- a/src/conf/netdev_bandwidth_conf.h +++ b/src/conf/netdev_bandwidth_conf.h @@ -32,7 +32,7 @@ int virNetDevBandwidthParse(virNetDevBandwidthPtr *bandwidth, xmlNodePtr node, int net_type) - ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_RETURN_CHECK; + ATTRIBUTE_NONNULL(1) ATTRIBUTE_RETURN_CHECK; int virNetDevBandwidthFormat(virNetDevBandwidthPtr def, virBufferPtr buf) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2); -- 2.9.3
Since the code checks and handles NULL parameters, remove the NONNULL from the prototype. Also fix the comment in the source to reference the right name. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/netdev_bandwidth_conf.c | 2 +- src/conf/netdev_bandwidth_conf.h | 3 +-- 2 files changed, 2 insertions(+), 3 deletions(-) diff --git a/src/conf/netdev_bandwidth_conf.c b/src/conf/netdev_bandwidth_conf.c index 8824332..50138fd 100644 --- a/src/conf/netdev_bandwidth_conf.c +++ b/src/conf/netdev_bandwidth_conf.c @@ -241,7 +241,7 @@ virNetDevBandwidthRateFormat(virNetDevBandwidthRatePtr def, } /** - * virNetDevBandwidthDefFormat: + * virNetDevBandwidthFormat: * @def: Data source * @buf: Buffer to print to * diff --git a/src/conf/netdev_bandwidth_conf.h b/src/conf/netdev_bandwidth_conf.h index c687427..30f9889 100644 --- a/src/conf/netdev_bandwidth_conf.h +++ b/src/conf/netdev_bandwidth_conf.h @@ -34,8 +34,7 @@ int virNetDevBandwidthParse(virNetDevBandwidthPtr *bandwidth, int net_type) ATTRIBUTE_NONNULL(1) ATTRIBUTE_RETURN_CHECK; int virNetDevBandwidthFormat(virNetDevBandwidthPtr def, - virBufferPtr buf) - ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2); + virBufferPtr buf); void virDomainClearNetBandwidth(virDomainObjPtr vm) ATTRIBUTE_NONNULL(1); -- 2.9.3
Since the code checks and handles a NULL 'numa' parameter, remove the NONNULL from the prototype. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/numa_conf.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/conf/numa_conf.h b/src/conf/numa_conf.h index 05529ba..b6a5354 100644 --- a/src/conf/numa_conf.h +++ b/src/conf/numa_conf.h @@ -85,8 +85,8 @@ int virDomainNumatuneMaybeGetNodeset(virDomainNumaPtr numatune, virBitmapPtr *retNodeset, int cellid); -size_t virDomainNumaGetNodeCount(virDomainNumaPtr numa) - ATTRIBUTE_NONNULL(1); +size_t virDomainNumaGetNodeCount(virDomainNumaPtr numa); + virBitmapPtr virDomainNumaGetNodeCpumask(virDomainNumaPtr numa, size_t node) ATTRIBUTE_NONNULL(1); -- 2.9.3
On Wed, Mar 22, 2017 at 10:21:18 -0400, John Ferlan wrote:
Since the code checks and handles a NULL 'numa' parameter, remove the NONNULL from the prototype.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/numa_conf.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
ACK
The prototype requires a NONNULL argument and the only caller passes in a non-null parameter. Besides the "else if" condition would deref it anyway. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/network/bridge_driver.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index a753cd7..d975092 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -5468,7 +5468,7 @@ networkBandwidthUpdate(virDomainNetDefPtr iface, /* Okay, there are three possible scenarios: */ if (ifaceBand && ifaceBand->in && ifaceBand->in->floor && - newBandwidth && newBandwidth->in && newBandwidth->in->floor) { + newBandwidth->in && newBandwidth->in->floor) { /* Either we just need to update @floor .. */ if (virNetDevBandwidthUpdateRate(network->def->bridge, -- 2.9.3
The prototype requires not passing a NULL in the parameter and the callers all would fail far before this code would fail if 'vm' was NULL, so just remove the check. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_monitor.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c index 79da472..8218029 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -910,8 +910,7 @@ qemuMonitorOpen(virDomainObjPtr vm, case VIR_DOMAIN_CHR_TYPE_UNIX: hasSendFD = true; if ((fd = qemuMonitorOpenUnix(config->data.nix.path, - vm ? vm->pid : 0, - timeout)) < 0) + vm->pid, timeout)) < 0) return NULL; break; -- 2.9.3
The 'mon' argument validity is checked in the QEMU_CHECK_MONITOR for the following functions, so they don't need the NONNULL on their prototype: qemuMonitorUpdateVideoMemorySize qemuMonitorUpdateVideoVram64Size qemuMonitorGetAllBlockStatsInfo qemuMonitorBlockStatsUpdateCapacity Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_monitor.h | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h index cf79424..888a03d 100644 --- a/src/qemu/qemu_monitor.h +++ b/src/qemu/qemu_monitor.h @@ -281,11 +281,11 @@ void qemuMonitorSetOptions(qemuMonitorPtr mon, virJSONValuePtr options) int qemuMonitorUpdateVideoMemorySize(qemuMonitorPtr mon, virDomainVideoDefPtr video, const char *videoName) - ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3); + ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3); int qemuMonitorUpdateVideoVram64Size(qemuMonitorPtr mon, virDomainVideoDefPtr video, const char *videoName) - ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3); + ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3); int qemuMonitorHMPCommandWithFd(qemuMonitorPtr mon, const char *cmd, int scm_fd, @@ -497,12 +497,12 @@ struct _qemuBlockStats { int qemuMonitorGetAllBlockStatsInfo(qemuMonitorPtr mon, virHashTablePtr *ret_stats, bool backingChain) - ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2); + ATTRIBUTE_NONNULL(2); int qemuMonitorBlockStatsUpdateCapacity(qemuMonitorPtr mon, virHashTablePtr stats, bool backingChain) - ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2); + ATTRIBUTE_NONNULL(2); int qemuMonitorBlockResize(qemuMonitorPtr mon, const char *dev_name, -- 2.9.3
Rather than returning an int and a *bitmap pointer, just return and check a NULL bitmap pointer Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/util/virbitmap.c | 26 ++++++++++++-------------- src/util/virbitmap.h | 7 +++---- tests/testutils.c | 2 +- tests/virbitmaptest.c | 2 +- 4 files changed, 17 insertions(+), 20 deletions(-) diff --git a/src/util/virbitmap.c b/src/util/virbitmap.c index 1b47d74..d263d73 100644 --- a/src/util/virbitmap.c +++ b/src/util/virbitmap.c @@ -545,7 +545,6 @@ virBitmapParse(const char *str, /** * virBitmapParseUnlimited: * @str: points to a string representing a human-readable bitmap - * @bitmap: a bitmap created from @str * * This function is the counterpart of virBitmapFormat. This function creates * a bitmap, in which bits are set according to the content of @str. @@ -556,20 +555,20 @@ virBitmapParse(const char *str, * to set, and ^N, which means to unset the bit, and N-M for ranges of bits * to set. * - * Returns 0 on success, or -1 in case of error. + * Returns @bitmap on success, or NULL in case of error */ -int -virBitmapParseUnlimited(const char *str, - virBitmapPtr *bitmap) +virBitmapPtr +virBitmapParseUnlimited(const char *str) { + virBitmapPtr bitmap; bool neg = false; const char *cur = str; char *tmp; size_t i; int start, last; - if (!(*bitmap = virBitmapNewEmpty())) - return -1; + if (!(bitmap = virBitmapNewEmpty())) + return NULL; if (!str) goto error; @@ -605,10 +604,10 @@ virBitmapParseUnlimited(const char *str, if (*cur == ',' || *cur == 0) { if (neg) { - if (virBitmapClearBitExpand(*bitmap, start) < 0) + if (virBitmapClearBitExpand(bitmap, start) < 0) goto error; } else { - if (virBitmapSetBitExpand(*bitmap, start) < 0) + if (virBitmapSetBitExpand(bitmap, start) < 0) goto error; } } else if (*cur == '-') { @@ -626,7 +625,7 @@ virBitmapParseUnlimited(const char *str, cur = tmp; for (i = start; i <= last; i++) { - if (virBitmapSetBitExpand(*bitmap, i) < 0) + if (virBitmapSetBitExpand(bitmap, i) < 0) goto error; } @@ -644,14 +643,13 @@ virBitmapParseUnlimited(const char *str, } } - return 0; + return bitmap; error: virReportError(VIR_ERR_INVALID_ARG, _("Failed to parse bitmap '%s'"), str); - virBitmapFree(*bitmap); - *bitmap = NULL; - return -1; + virBitmapFree(bitmap); + return NULL; } /** diff --git a/src/util/virbitmap.h b/src/util/virbitmap.h index 58e0ee6..3ba40ae 100644 --- a/src/util/virbitmap.h +++ b/src/util/virbitmap.h @@ -94,10 +94,9 @@ virBitmapParseSeparator(const char *str, char terminator, virBitmapPtr *bitmap, size_t bitmapSize); -int -virBitmapParseUnlimited(const char *str, - virBitmapPtr *bitmap) - ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2); +virBitmapPtr +virBitmapParseUnlimited(const char *str) + ATTRIBUTE_NONNULL(1); virBitmapPtr virBitmapNewCopy(virBitmapPtr src) ATTRIBUTE_NONNULL(1); diff --git a/tests/testutils.c b/tests/testutils.c index a596a83..13eff9e 100644 --- a/tests/testutils.c +++ b/tests/testutils.c @@ -926,7 +926,7 @@ int virTestMain(int argc, } if ((testRange = getenv("VIR_TEST_RANGE")) != NULL) { - if (virBitmapParseUnlimited(testRange, &testBitmap) < 0) { + if (!(testBitmap = virBitmapParseUnlimited(testRange))) { fprintf(stderr, "Cannot parse range %s\n", testRange); return EXIT_FAILURE; } diff --git a/tests/virbitmaptest.c b/tests/virbitmaptest.c index 3ee07ff..e007efc 100644 --- a/tests/virbitmaptest.c +++ b/tests/virbitmaptest.c @@ -650,7 +650,7 @@ test12(const void *opaque ATTRIBUTE_UNUSED) TEST_MAP(151, "128"); virBitmapFree(map); - if (virBitmapParseUnlimited("34,1023", &map) < 0) + if (!(map = virBitmapParseUnlimited("34,1023"))) goto cleanup; TEST_MAP(1024, "34,1023"); -- 2.9.3
The code checks and handles a NULL 'str', so just remove the NONNULL. Update the error message to add the NULLSTR() around 'str' also. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/util/virbitmap.c | 2 +- src/util/virbitmap.h | 3 +-- 2 files changed, 2 insertions(+), 3 deletions(-) diff --git a/src/util/virbitmap.c b/src/util/virbitmap.c index d263d73..eac63d9 100644 --- a/src/util/virbitmap.c +++ b/src/util/virbitmap.c @@ -647,7 +647,7 @@ virBitmapParseUnlimited(const char *str) error: virReportError(VIR_ERR_INVALID_ARG, - _("Failed to parse bitmap '%s'"), str); + _("Failed to parse bitmap '%s'"), NULLSTR(str)); virBitmapFree(bitmap); return NULL; } diff --git a/src/util/virbitmap.h b/src/util/virbitmap.h index 3ba40ae..36282af 100644 --- a/src/util/virbitmap.h +++ b/src/util/virbitmap.h @@ -95,8 +95,7 @@ virBitmapParseSeparator(const char *str, virBitmapPtr *bitmap, size_t bitmapSize); virBitmapPtr -virBitmapParseUnlimited(const char *str) - ATTRIBUTE_NONNULL(1); +virBitmapParseUnlimited(const char *str); virBitmapPtr virBitmapNewCopy(virBitmapPtr src) ATTRIBUTE_NONNULL(1); -- 2.9.3
The called function uses a STRNEQ_NULLABLE anyway for both 'drv_name' and 'dom_name', so no need. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/util/virhostdev.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/util/virhostdev.h b/src/util/virhostdev.h index 1202136..4ce5c4d 100644 --- a/src/util/virhostdev.h +++ b/src/util/virhostdev.h @@ -103,7 +103,7 @@ virHostdevReAttachPCIDevices(virHostdevManagerPtr hostdev_mgr, virDomainHostdevDefPtr *hostdevs, int nhostdevs, const char *oldStateDir) - ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3); + ATTRIBUTE_NONNULL(1); void virHostdevReAttachUSBDevices(virHostdevManagerPtr hostdev_mgr, const char *drv_name, -- 2.9.3
The comparison code used STREQ_NULLABLE anyway for both 'drv_name' and 'dom_name', so no need. Add a NULLSTR on the 'dom_name' too. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/util/virhostdev.c | 2 +- src/util/virhostdev.h | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/util/virhostdev.c b/src/util/virhostdev.c index 86ca8e0..c424a97 100644 --- a/src/util/virhostdev.c +++ b/src/util/virhostdev.c @@ -1624,7 +1624,7 @@ virHostdevReAttachUSBDevices(virHostdevManagerPtr mgr, if (!(usb = virUSBDeviceNew(usbsrc->bus, usbsrc->device, NULL))) { VIR_WARN("Unable to reattach USB device %03d.%03d on domain %s", - usbsrc->bus, usbsrc->device, dom_name); + usbsrc->bus, usbsrc->device, NULLSTR(dom_name)); continue; } diff --git a/src/util/virhostdev.h b/src/util/virhostdev.h index 4ce5c4d..76039bb 100644 --- a/src/util/virhostdev.h +++ b/src/util/virhostdev.h @@ -110,7 +110,7 @@ virHostdevReAttachUSBDevices(virHostdevManagerPtr hostdev_mgr, const char *dom_name, virDomainHostdevDefPtr *hostdevs, int nhostdevs) - ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3); + ATTRIBUTE_NONNULL(1); void virHostdevReAttachSCSIDevices(virHostdevManagerPtr hostdev_mgr, const char *drv_name, -- 2.9.3
The comparison code used STREQ_NULLABLE anyway for both 'drv_name' and 'dom_name', so no need. Add a NULLSTR on the 'dom_name' too. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/util/virhostdev.c | 2 +- src/util/virhostdev.h | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/util/virhostdev.c b/src/util/virhostdev.c index c424a97..fc0ebcb 100644 --- a/src/util/virhostdev.c +++ b/src/util/virhostdev.c @@ -1756,7 +1756,7 @@ virHostdevReAttachSCSIVHostDevices(virHostdevManagerPtr mgr, if (!(host = virSCSIVHostDeviceNew(hostsrc->wwpn))) { VIR_WARN("Unable to reattach SCSI_host device %s on domain %s", - hostsrc->wwpn, dom_name); + hostsrc->wwpn, NULLSTR(dom_name)); virObjectUnlock(mgr->activeSCSIVHostHostdevs); return; } diff --git a/src/util/virhostdev.h b/src/util/virhostdev.h index 76039bb..43ba705 100644 --- a/src/util/virhostdev.h +++ b/src/util/virhostdev.h @@ -124,7 +124,7 @@ virHostdevReAttachSCSIVHostDevices(virHostdevManagerPtr hostdev_mgr, const char *dom_name, virDomainHostdevDefPtr *hostdevs, int nhostdevs) - ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3); + ATTRIBUTE_NONNULL(1); int virHostdevUpdateActivePCIDevices(virHostdevManagerPtr mgr, virDomainHostdevDefPtr *hostdevs, -- 2.9.3
Since the code checks 'mgr == NULL' anyway, no need for the prototype to have the NONNULL arg check. Also add an error message to indicate what the failure is so that there isn't a failed for some reason error. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/util/virhostdev.c | 5 ++++- src/util/virhostdev.h | 2 +- 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/src/util/virhostdev.c b/src/util/virhostdev.c index fc0ebcb..a75d108 100644 --- a/src/util/virhostdev.c +++ b/src/util/virhostdev.c @@ -1873,8 +1873,11 @@ virHostdevPrepareDomainDevices(virHostdevManagerPtr mgr, if (!def->nhostdevs) return 0; - if (mgr == NULL) + if (!mgr) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("no host device manager defined")); return -1; + } if (flags & VIR_HOSTDEV_SP_PCI) { if (virHostdevPreparePCIDevices(mgr, driver, diff --git a/src/util/virhostdev.h b/src/util/virhostdev.h index 43ba705..7ee0b43 100644 --- a/src/util/virhostdev.h +++ b/src/util/virhostdev.h @@ -157,7 +157,7 @@ virHostdevPrepareDomainDevices(virHostdevManagerPtr mgr, const char *driver, virDomainDefPtr def, unsigned int flags) - ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3); + ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3); void virHostdevReAttachDomainDevices(virHostdevManagerPtr mgr, const char *driver, -- 2.9.3
Since the function handles a NULL 'mgr' condition, no need for the NONNULL Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/util/virhostdev.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/util/virhostdev.h b/src/util/virhostdev.h index 7ee0b43..45d1c2e 100644 --- a/src/util/virhostdev.h +++ b/src/util/virhostdev.h @@ -164,7 +164,7 @@ virHostdevReAttachDomainDevices(virHostdevManagerPtr mgr, virDomainDefPtr def, unsigned int flags, const char *oldStateDir) - ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3); + ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3); bool virHostdevIsSCSIDevice(virDomainHostdevDefPtr hostdev) ATTRIBUTE_NONNULL(1); -- 2.9.3
The code checks and handles a NULL 'migrate', so no need for NONNULL Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/util/virnetdevopenvswitch.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/util/virnetdevopenvswitch.h b/src/util/virnetdevopenvswitch.h index 4f62be1..021eba9 100644 --- a/src/util/virnetdevopenvswitch.h +++ b/src/util/virnetdevopenvswitch.h @@ -51,7 +51,7 @@ int virNetDevOpenvswitchGetMigrateData(char **migrate, const char *ifname) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_RETURN_CHECK; int virNetDevOpenvswitchSetMigrateData(char *migrate, const char *ifname) - ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_RETURN_CHECK; + ATTRIBUTE_NONNULL(2) ATTRIBUTE_RETURN_CHECK; int virNetDevOpenvswitchInterfaceStats(const char *ifname, virDomainInterfaceStatsPtr stats) -- 2.9.3
The API checks each parameter for NULL anyway and would error, so need to add NONNULL on prototype. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/util/virtypedparam.h | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/util/virtypedparam.h b/src/util/virtypedparam.h index 7ab143f..59d5fbc 100644 --- a/src/util/virtypedparam.h +++ b/src/util/virtypedparam.h @@ -79,8 +79,7 @@ virTypedParamsFilter(virTypedParameterPtr params, int nparams, const char *name, virTypedParameterPtr **ret) - ATTRIBUTE_RETURN_CHECK ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(3) - ATTRIBUTE_NONNULL(4); + ATTRIBUTE_RETURN_CHECK; int virTypedParameterAssign(virTypedParameterPtr param, const char *name, -- 2.9.3
Since the code checks and handles a NULL 'net_bandwidth' parameter, so no need for NONNNULL. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/util/virnetdevbandwidth.h | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/util/virnetdevbandwidth.h b/src/util/virnetdevbandwidth.h index bceac2e..64f3537 100644 --- a/src/util/virnetdevbandwidth.h +++ b/src/util/virnetdevbandwidth.h @@ -59,8 +59,7 @@ int virNetDevBandwidthPlug(const char *brname, const virMacAddr *ifmac_ptr, virNetDevBandwidthPtr bandwidth, unsigned int id) - ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) - ATTRIBUTE_NONNULL(3) ATTRIBUTE_NONNULL(4) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(3) ATTRIBUTE_NONNULL(4) ATTRIBUTE_RETURN_CHECK; int virNetDevBandwidthUnplug(const char *brname, -- 2.9.3
Since the code checks and handles a NULL 'path', no need for the NONNULL Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/util/virnetdevopenvswitch.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/util/virnetdevopenvswitch.h b/src/util/virnetdevopenvswitch.h index 021eba9..7380a2d 100644 --- a/src/util/virnetdevopenvswitch.h +++ b/src/util/virnetdevopenvswitch.h @@ -59,6 +59,6 @@ int virNetDevOpenvswitchInterfaceStats(const char *ifname, int virNetDevOpenvswitchGetVhostuserIfname(const char *path, char **ifname) - ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_RETURN_CHECK; + ATTRIBUTE_NONNULL(2) ATTRIBUTE_RETURN_CHECK; #endif /* __VIR_NETDEV_OPENVSWITCH_H__ */ -- 2.9.3
The source code will check for NULL arguments for 'macvtap_macaddr' and 'vmuuid', so no need for the NONNULL in the prototypes. Following the stack for both arguments to virNetDevVPortProfileOpSetLink also shows called functions would handle a NULL value. Additionally, modified the prototype to use the same 'macvtap_macaddr' name as the source code for consistency. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/util/virnetdevvportprofile.h | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/src/util/virnetdevvportprofile.h b/src/util/virnetdevvportprofile.h index dc3e643..b706c41 100644 --- a/src/util/virnetdevvportprofile.h +++ b/src/util/virnetdevvportprofile.h @@ -95,23 +95,21 @@ int virNetDevVPortProfileMerge3(virNetDevVPortProfilePtr *result, int virNetDevVPortProfileAssociate(const char *ifname, const virNetDevVPortProfile *virtPort, - const virMacAddr *macaddr, + const virMacAddr *macvtap_macaddr, const char *linkdev, int vf, const unsigned char *vmuuid, virNetDevVPortProfileOp vmOp, bool setlink_only) - ATTRIBUTE_NONNULL(3) ATTRIBUTE_NONNULL(4) ATTRIBUTE_NONNULL(6) - ATTRIBUTE_RETURN_CHECK; + ATTRIBUTE_NONNULL(4) ATTRIBUTE_RETURN_CHECK; int virNetDevVPortProfileDisassociate(const char *ifname, const virNetDevVPortProfile *virtPort, - const virMacAddr *macaddr, + const virMacAddr *macvtap_macaddr, const char *linkdev, int vf, virNetDevVPortProfileOp vmOp) - ATTRIBUTE_NONNULL(3) ATTRIBUTE_NONNULL(4) - ATTRIBUTE_RETURN_CHECK; + ATTRIBUTE_NONNULL(4) ATTRIBUTE_RETURN_CHECK; #endif /* __VIR_NETDEV_VPORT_PROFILE_H__ */ -- 2.9.3
Since the source code checks 'ifname' for NULL before using, the prototype doesn't need the NONNULL Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/util/virnetdevmacvlan.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/util/virnetdevmacvlan.h b/src/util/virnetdevmacvlan.h index 9a85a65..c40f23e 100644 --- a/src/util/virnetdevmacvlan.h +++ b/src/util/virnetdevmacvlan.h @@ -88,7 +88,7 @@ int virNetDevMacVLanDeleteWithVPortProfile(const char *ifname, int mode, virNetDevVPortProfilePtr virtPortProfile, char *stateDir) - ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3) + ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3) ATTRIBUTE_NONNULL(6) ATTRIBUTE_RETURN_CHECK; int virNetDevMacVLanRestartWithVPortProfile(const char *cr_ifname, -- 2.9.3
Since the code checks and handles a NULL 'cpus' anyway, so no need for the NONNULL. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/cpu/cpu.h | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/cpu/cpu.h b/src/cpu/cpu.h index 7d6d3e9..d23409a 100644 --- a/src/cpu/cpu.h +++ b/src/cpu/cpu.h @@ -195,8 +195,7 @@ cpuBaseline (virCPUDefPtr *cpus, unsigned int ncpus, const char **models, unsigned int nmodels, - unsigned int flags) - ATTRIBUTE_NONNULL(1); + unsigned int flags); int virCPUUpdate(virArch arch, -- 2.9.3
On Wed, Mar 22, 2017 at 10:21:13AM -0400, John Ferlan wrote:
Hopefully the quantity doesn't scare anyone off... These are mostly innocuous, but separated each out to appease the masses.
The first patch fixes a recently uncovered Coverity warning regarding FORWARD_NULL of the 'formatStr'... According to an IRC response from pkrempa the format should be set up properly in qemuDomainSnapshotPrepare
Patches 2-22 resolve various different build issues in the Coverity environment that sets "lv_cv_static_analysis=yes" during the build. This is 'reproducible' if one sets that as part of their autogen.sh processing - something that we should probably do in the CI build environment so we can get all those ATTRIBUTE_NONNULL checks to be run on a daily basis.
I was using that and I had some issues that were about more than just a build issue. Trying it now, it cleanly says it just needs some fixes for the NUNNULL parameters. That's nice. With optimizations this series might even fix things, let's see =) ACK to this series.
participants (3)
-
John Ferlan -
Martin Kletzander -
Peter Krempa