[libvirt] [PATCH 0/4] Resolve a few Coverity found issues
Was actually hoping the nodedev series would get reviewed, but since it didn't the first patch fixes a problem that Coverity noted... The other 3 patches are also things Coverity has noted and I've been holding onto in my wait for enough Coverity issues to surface before posting queue of things to do. The really strange thing about patch 2 is Coverity complained on the second one, but not the first mainly because of the net->ifname check a few lines earlier. By moving things - the complaint went away, but the code looked better with a helper so I kept it that way. Patch 3 is based on newer code that caught the attention, while patch 4 is a much older change which just got buried amongst 30 others that I keep in a false positive branch John Ferlan (4): nodedev: Add check for NULL obj before call Unlock hotplug: Create helper to remove vport util: Adjust the #ifdef logic in virNetDevBridgeCreate test: Fix possible ap leak for VIR_TEST_PRELOAD src/node_device/node_device_driver.c | 3 ++- src/qemu/qemu_hotplug.c | 41 +++++++++++++++++------------------- src/util/virnetdevbridge.c | 3 ++- tests/qemucapsprobe.c | 2 +- tests/testutils.c | 4 ++-- tests/testutils.h | 6 +++++- 6 files changed, 31 insertions(+), 28 deletions(-) -- 2.9.4
Commit id '95ea171b' was a bit too aggressive in removing the if (obj) check since cleanup is reachable after Unlock and obj = NULL. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/node_device/node_device_driver.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/node_device/node_device_driver.c b/src/node_device/node_device_driver.c index bc5c0e5..5bf202e 100644 --- a/src/node_device/node_device_driver.c +++ b/src/node_device/node_device_driver.c @@ -650,7 +650,8 @@ nodeDeviceDestroy(virNodeDevicePtr dev) cleanup: nodeDeviceUnlock(); - virNodeDeviceObjUnlock(obj); + if (obj) + virNodeDeviceObjUnlock(obj); VIR_FREE(wwnn); VIR_FREE(wwpn); return ret; -- 2.9.4
On Wed, Jun 28, 2017 at 07:37:35 -0400, John Ferlan wrote:
Commit id '95ea171b' was a bit too aggressive in removing the if (obj) check since cleanup is reachable after Unlock and obj = NULL.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/node_device/node_device_driver.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
ACK
Combine and "clean up" a bit two places that are removing the vport Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_hotplug.c | 41 +++++++++++++++++++---------------------- 1 file changed, 19 insertions(+), 22 deletions(-) diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index a486fb4..1af85be 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -937,6 +937,23 @@ qemuDomainAttachDeviceDiskLive(virConnectPtr conn, } +static void +qemuDomainNetDeviceVportRemove(virDomainNetDefPtr net) +{ + virNetDevVPortProfilePtr vport = virDomainNetGetActualVirtPortProfile(net); + + if (!vport) + return; + + if (vport->virtPortType == VIR_NETDEV_VPORT_PROFILE_MIDONET) { + ignore_value(virNetDevMidonetUnbindPort(vport)); + } else if (vport->virtPortType == VIR_NETDEV_VPORT_PROFILE_OPENVSWITCH) { + const char *brname = virDomainNetGetActualBridgeName(net); + ignore_value(virNetDevOpenvswitchRemovePort(brname, net->ifname)); + } +} + + int qemuDomainAttachNetDevice(virQEMUDriverPtr driver, virDomainObjPtr vm, @@ -954,7 +971,6 @@ qemuDomainAttachNetDevice(virQEMUDriverPtr driver, size_t queueSize = 0; char *nicstr = NULL; char *netstr = NULL; - virNetDevVPortProfilePtr vport = NULL; int ret = -1; int vlan; bool releaseaddr = false; @@ -1302,16 +1318,7 @@ qemuDomainAttachNetDevice(virQEMUDriverPtr driver, cfg->stateDir)); } - vport = virDomainNetGetActualVirtPortProfile(net); - if (vport) { - if (vport->virtPortType == VIR_NETDEV_VPORT_PROFILE_MIDONET) { - ignore_value(virNetDevMidonetUnbindPort(vport)); - } else if (vport->virtPortType == VIR_NETDEV_VPORT_PROFILE_OPENVSWITCH) { - ignore_value(virNetDevOpenvswitchRemovePort( - virDomainNetGetActualBridgeName(net), - net->ifname)); - } - } + qemuDomainNetDeviceVportRemove(net); } virDomainNetRemoveHostdev(vm->def, net); @@ -3949,7 +3956,6 @@ qemuDomainRemoveNetDevice(virQEMUDriverPtr driver, { virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); qemuDomainObjPrivatePtr priv = vm->privateData; - virNetDevVPortProfilePtr vport; virObjectEventPtr event; char *hostnet_name = NULL; char *charDevAlias = NULL; @@ -4038,16 +4044,7 @@ qemuDomainRemoveNetDevice(virQEMUDriverPtr driver, cfg->stateDir)); } - vport = virDomainNetGetActualVirtPortProfile(net); - if (vport) { - if (vport->virtPortType == VIR_NETDEV_VPORT_PROFILE_MIDONET) { - ignore_value(virNetDevMidonetUnbindPort(vport)); - } else if (vport->virtPortType == VIR_NETDEV_VPORT_PROFILE_OPENVSWITCH) { - ignore_value(virNetDevOpenvswitchRemovePort( - virDomainNetGetActualBridgeName(net), - net->ifname)); - } - } + qemuDomainNetDeviceVportRemove(net); networkReleaseActualDevice(vm->def, net); virDomainNetDefFree(net); -- 2.9.4
On Wed, Jun 28, 2017 at 07:37:36 -0400, John Ferlan wrote:
Combine and "clean up" a bit two places that are removing the vport
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_hotplug.c | 41 +++++++++++++++++++---------------------- 1 file changed, 19 insertions(+), 22 deletions(-)
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index a486fb4..1af85be 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -937,6 +937,23 @@ qemuDomainAttachDeviceDiskLive(virConnectPtr conn, }
+static void +qemuDomainNetDeviceVportRemove(virDomainNetDefPtr net) +{ + virNetDevVPortProfilePtr vport = virDomainNetGetActualVirtPortProfile(net); + + if (!vport) + return; + + if (vport->virtPortType == VIR_NETDEV_VPORT_PROFILE_MIDONET) { + ignore_value(virNetDevMidonetUnbindPort(vport)); + } else if (vport->virtPortType == VIR_NETDEV_VPORT_PROFILE_OPENVSWITCH) { + const char *brname = virDomainNetGetActualBridgeName(net);
Declare this variable at the top level along with vport.
+ ignore_value(virNetDevOpenvswitchRemovePort(brname, net->ifname)); + } +}
ACK
Commit id 'adf846d3' added many ATTRIBUTE_FALLTHROUGH; statements; however, this particular one caught the eye of the Coverity checker which notes that the statement isn't reachable when the # if is true. While it could be considered a false positive, just add an # else in order to make it more obvious. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/util/virnetdevbridge.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/util/virnetdevbridge.c b/src/util/virnetdevbridge.c index 11b03b4..1361a51 100644 --- a/src/util/virnetdevbridge.c +++ b/src/util/virnetdevbridge.c @@ -478,11 +478,12 @@ virNetDevBridgeCreate(const char *brname) */ rc = virNetDevBridgeCreateWithIoctl(brname); goto cleanup; -# endif +# else /* intentionally fall through if virNetDevBridgeCreateWithIoctl() * isn't available. */ ATTRIBUTE_FALLTHROUGH; +# endif default: virReportSystemError(-err->error, _("error creating bridge interface %s"), -- 2.9.4
On Wed, Jun 28, 2017 at 07:37:37 -0400, John Ferlan wrote:
Commit id 'adf846d3' added many ATTRIBUTE_FALLTHROUGH; statements; however, this particular one caught the eye of the Coverity checker which notes that the statement isn't reachable when the # if is true.
While it could be considered a false positive, just add an # else in order to make it more obvious.
Signed-off-by: John Ferlan <jferlan@redhat.com> ---
I think this is a cleaner solution, since it gets rid of that weird swtich statement altogether: diff --git a/src/util/virnetdevbridge.c b/src/util/virnetdevbridge.c index 11b03b426..cfb7ebae9 100644 --- a/src/util/virnetdevbridge.c +++ b/src/util/virnetdevbridge.c @@ -468,22 +468,17 @@ virNetDevBridgeCreate(const char *brname) if (resp->nlmsg_len < NLMSG_LENGTH(sizeof(*err))) goto malformed_resp; - switch (err->error) { - case 0: - break; - case -EOPNOTSUPP: + if (err->error < 0) { # if defined(HAVE_STRUCT_IFREQ) && defined(SIOCBRADDBR) - /* fallback to ioctl if netlink doesn't support creating - * bridges - */ - rc = virNetDevBridgeCreateWithIoctl(brname); - goto cleanup; + if (err->error == -EOPNOTSUPP) { + /* fallback to ioctl if netlink doesn't support creating + * bridges + */ + rc = virNetDevBridgeCreateWithIoctl(brname); + goto cleanup; + } # endif - /* intentionally fall through if virNetDevBridgeCreateWithIoctl() - * isn't available. - */ - ATTRIBUTE_FALLTHROUGH; - default: + virReportSystemError(-err->error, _("error creating bridge interface %s"), brname); I'll send it as a formal patch if you agree.
On 06/28/2017 08:01 AM, Peter Krempa wrote:
On Wed, Jun 28, 2017 at 07:37:37 -0400, John Ferlan wrote:
Commit id 'adf846d3' added many ATTRIBUTE_FALLTHROUGH; statements; however, this particular one caught the eye of the Coverity checker which notes that the statement isn't reachable when the # if is true.
While it could be considered a false positive, just add an # else in order to make it more obvious.
Signed-off-by: John Ferlan <jferlan@redhat.com> ---
I think this is a cleaner solution, since it gets rid of that weird swtich statement altogether:
That's fine. I'll drop this one. John
diff --git a/src/util/virnetdevbridge.c b/src/util/virnetdevbridge.c index 11b03b426..cfb7ebae9 100644 --- a/src/util/virnetdevbridge.c +++ b/src/util/virnetdevbridge.c @@ -468,22 +468,17 @@ virNetDevBridgeCreate(const char *brname) if (resp->nlmsg_len < NLMSG_LENGTH(sizeof(*err))) goto malformed_resp;
- switch (err->error) { - case 0: - break; - case -EOPNOTSUPP: + if (err->error < 0) { # if defined(HAVE_STRUCT_IFREQ) && defined(SIOCBRADDBR) - /* fallback to ioctl if netlink doesn't support creating - * bridges - */ - rc = virNetDevBridgeCreateWithIoctl(brname); - goto cleanup; + if (err->error == -EOPNOTSUPP) { + /* fallback to ioctl if netlink doesn't support creating + * bridges + */ + rc = virNetDevBridgeCreateWithIoctl(brname); + goto cleanup; + } # endif - /* intentionally fall through if virNetDevBridgeCreateWithIoctl() - * isn't available. - */ - ATTRIBUTE_FALLTHROUGH; - default: + virReportSystemError(-err->error, _("error creating bridge interface %s"), brname);
I'll send it as a formal patch if you agree.
Commit id 'f0469c61e' added the ability to load multiple mock libraries by using the va_start() varable args marker; however, if something within the VIR_TEST_PRELOAD fails, then control is returned to caller without calling va_end. So add a couple more markers to indicat the need to call va_end w/ the ap before returning. Signed-off-by: John Ferlan <jferlan@redhat.com> --- tests/qemucapsprobe.c | 2 +- tests/testutils.c | 4 ++-- tests/testutils.h | 6 +++++- 3 files changed, 8 insertions(+), 4 deletions(-) diff --git a/tests/qemucapsprobe.c b/tests/qemucapsprobe.c index 662c703..bca43b4 100644 --- a/tests/qemucapsprobe.c +++ b/tests/qemucapsprobe.c @@ -48,7 +48,7 @@ main(int argc, char **argv) virThread thread; virQEMUCapsPtr caps; - VIR_TEST_PRELOAD(abs_builddir "/.libs/qemucapsprobemock.so"); + VIR_TEST_PRELOAD(abs_builddir "/.libs/qemucapsprobemock.so", false, NULL); if (argc != 2) { fprintf(stderr, "%s QEMU_binary\n", argv[0]); diff --git a/tests/testutils.c b/tests/testutils.c index f455969..16fd3c6 100644 --- a/tests/testutils.c +++ b/tests/testutils.c @@ -904,11 +904,11 @@ int virTestMain(int argc, virLogOutputPtr *outputs = NULL; if (getenv("VIR_TEST_FILE_ACCESS")) - VIR_TEST_PRELOAD(TEST_MOCK); + VIR_TEST_PRELOAD(TEST_MOCK, false, NULL); va_start(ap, func); while ((lib = va_arg(ap, const char *))) - VIR_TEST_PRELOAD(lib); + VIR_TEST_PRELOAD(lib, true, ap); va_end(ap); progname = last_component(argv[0]); diff --git a/tests/testutils.h b/tests/testutils.h index c7f02e4..e4c602b 100644 --- a/tests/testutils.h +++ b/tests/testutils.h @@ -115,12 +115,14 @@ int virTestMain(int argc, return virTestMain(argc, argv, func, NULL); \ } -# define VIR_TEST_PRELOAD(lib) \ +# define VIR_TEST_PRELOAD(lib, haveap, _ap) \ do { \ const char *preload = getenv("LD_PRELOAD"); \ if (preload == NULL || strstr(preload, lib) == NULL) { \ char *newenv; \ if (!virFileIsExecutable(lib)) { \ + if (haveap) \ + va_end(_ap); \ perror(lib); \ return EXIT_FAILURE; \ } \ @@ -128,6 +130,8 @@ int virTestMain(int argc, newenv = (char *) lib; \ } else if (virAsprintf(&newenv, "%s:%s", lib, preload) < 0) { \ perror("virAsprintf"); \ + if (haveap) \ + va_end(_ap); \ return EXIT_FAILURE; \ } \ setenv("LD_PRELOAD", newenv, 1); \ -- 2.9.4
On Wed, Jun 28, 2017 at 07:37:38 -0400, John Ferlan wrote:
Commit id 'f0469c61e' added the ability to load multiple mock libraries by using the va_start() varable args marker; however, if something within the VIR_TEST_PRELOAD fails, then control is returned to caller without calling va_end. So add a couple more markers to indicat the need to call va_end w/ the ap before returning.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- tests/qemucapsprobe.c | 2 +- tests/testutils.c | 4 ++-- tests/testutils.h | 6 +++++- 3 files changed, 8 insertions(+), 4 deletions(-)
[...]
diff --git a/tests/testutils.h b/tests/testutils.h index c7f02e4..e4c602b 100644 --- a/tests/testutils.h +++ b/tests/testutils.h @@ -115,12 +115,14 @@ int virTestMain(int argc, return virTestMain(argc, argv, func, NULL); \ }
-# define VIR_TEST_PRELOAD(lib) \ +# define VIR_TEST_PRELOAD(lib, haveap, _ap) \ do { \ const char *preload = getenv("LD_PRELOAD"); \ if (preload == NULL || strstr(preload, lib) == NULL) { \ char *newenv; \ if (!virFileIsExecutable(lib)) { \ + if (haveap) \ + va_end(_ap); \ perror(lib); \
NACK, this is gross. Please make this macro jump to a label passed as argument so that the caller can do cleanup.
On 06/28/2017 07:45 AM, Peter Krempa wrote:
On Wed, Jun 28, 2017 at 07:37:38 -0400, John Ferlan wrote:
Commit id 'f0469c61e' added the ability to load multiple mock libraries by using the va_start() varable args marker; however, if something within the VIR_TEST_PRELOAD fails, then control is returned to caller without calling va_end. So add a couple more markers to indicat the need to call va_end w/ the ap before returning.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- tests/qemucapsprobe.c | 2 +- tests/testutils.c | 4 ++-- tests/testutils.h | 6 +++++- 3 files changed, 8 insertions(+), 4 deletions(-)
[...]
diff --git a/tests/testutils.h b/tests/testutils.h index c7f02e4..e4c602b 100644 --- a/tests/testutils.h +++ b/tests/testutils.h @@ -115,12 +115,14 @@ int virTestMain(int argc, return virTestMain(argc, argv, func, NULL); \ }
-# define VIR_TEST_PRELOAD(lib) \ +# define VIR_TEST_PRELOAD(lib, haveap, _ap) \ do { \ const char *preload = getenv("LD_PRELOAD"); \ if (preload == NULL || strstr(preload, lib) == NULL) { \ char *newenv; \ if (!virFileIsExecutable(lib)) { \ + if (haveap) \ + va_end(_ap); \ perror(lib); \
NACK, this is gross. Please make this macro jump to a label passed as argument so that the caller can do cleanup.
yuck - this was the cheaper/simpler approach. Not a fan of labels in the macro... It would mean that virTestMain gets two labels (e.g. "error:" and "va_error:") with a lot of code in between that returns EXIT_FAILURE that could conceivably also "goto error;", but won't... I dunno, I just find that worse. Similar for main in qemucapsprobe.c which would get an error: label to return EXIT_FAILURE even though there's 5 other such returns prior to that which don't goto error; I'll just drop this and keep my local copy as is. If someone wants to be more elegant, then have at it ;-) Thanks for the quick reviews on these. John
participants (2)
-
John Ferlan -
Peter Krempa