6:37 a.m.
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
6:37 a.m.
New subject: [libvirt] [PATCH 1/4] nodedev: Add check for NULL obj before call Unlock
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(a)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
6:48 a.m.
New subject: [libvirt] [PATCH 1/4] nodedev: Add check for NULL obj before call Unlock
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(a)redhat.com>
---
src/node_device/node_device_driver.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
ACK
6:37 a.m.
New subject: [libvirt] [PATCH 2/4] hotplug: Create helper to remove vport
Combine and "clean up" a bit two places that are removing the vport
Signed-off-by: John Ferlan <jferlan(a)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
7:05 a.m.
New subject: [libvirt] [PATCH 2/4] hotplug: Create helper to remove vport
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(a)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
6:37 a.m.
New subject: [libvirt] [PATCH 3/4] util: Adjust the #ifdef logic in virNetDevBridgeCreate
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(a)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
7:01 a.m.
New subject: [libvirt] [PATCH 3/4] util: Adjust the #ifdef logic in virNetDevBridgeCreate
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(a)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.
7:08 a.m.
New subject: [libvirt] [PATCH 3/4] util: Adjust the #ifdef logic in virNetDevBridgeCreate
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(a)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.
6:37 a.m.
New subject: [libvirt] [PATCH 4/4] test: Fix possible ap leak for VIR_TEST_PRELOAD
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(a)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
6:45 a.m.
New subject: [libvirt] [PATCH 4/4] test: Fix possible ap leak for VIR_TEST_PRELOAD
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(a)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.
7:41 a.m.
New subject: [libvirt] [PATCH 4/4] test: Fix possible ap leak for VIR_TEST_PRELOAD
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(a)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
2707
days inactive
2707
days old
10 comments
2 participants
participants (2)
-
John Ferlan -
Peter Krempa