[libvirt] [PATCH 0/4] Resolve some Coverity issues
The issues found in the first 3 patches aren't "usually" seen, but I've had them sitting around awhile and figured they should be posted anyway since the last issue was seen from a recent change... John Ferlan (4): hotplug: Resolve Coverity FORWARD_NULL virhook: Resolve Coverity NULL_RETURNS domain_conf: Resolve Coverity CHECKED_RETURN libxl: Resolve Coverity CHECKED_RETURN src/conf/domain_conf.c | 2 +- src/libxl/libxl_migration.c | 3 +-- src/qemu/qemu_hotplug.c | 12 ++++++------ src/util/virhook.c | 3 ++- 4 files changed, 10 insertions(+), 10 deletions(-) -- 1.9.3
Coverity complained that because the cfg->macFilter call checked net->ifname != NULL before calling ebtablesRemoveForwardAllowIn, then the virNetDevOpenvswitchRemovePort call should have the same check. However, if I move the ebtables call prior to the check for TYPE_DIRECT (where there is a VIR_FREE(net->ifname)), then it seems Coverity is happy. Since firewall info is tacked on last during setup, removing it in the opposite order of initialization seems to be natural anyway Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_hotplug.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index f2740f4..ff0eaf8 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -2786,6 +2786,12 @@ qemuDomainRemoveNetDevice(virQEMUDriverPtr driver, qemuDomainReleaseDeviceAddress(vm, &net->info, NULL); virDomainConfNWFilterTeardown(net); + if (cfg->macFilter && (net->ifname != NULL)) { + ignore_value(ebtablesRemoveForwardAllowIn(driver->ebtables, + net->ifname, + &net->mac)); + } + if (virDomainNetGetActualType(net) == VIR_DOMAIN_NET_TYPE_DIRECT) { ignore_value(virNetDevMacVLanDeleteWithVPortProfile( net->ifname, &net->mac, @@ -2796,12 +2802,6 @@ qemuDomainRemoveNetDevice(virQEMUDriverPtr driver, VIR_FREE(net->ifname); } - if (cfg->macFilter && (net->ifname != NULL)) { - ignore_value(ebtablesRemoveForwardAllowIn(driver->ebtables, - net->ifname, - &net->mac)); - } - vport = virDomainNetGetActualVirtPortProfile(net); if (vport && vport->virtPortType == VIR_NETDEV_VPORT_PROFILE_OPENVSWITCH) ignore_value(virNetDevOpenvswitchRemovePort( -- 1.9.3
Coverity complains that many other callers to return err from virGetLastError() will check if err is not NULL before dereferencing it. Just do the same here for safety. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/util/virhook.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/util/virhook.c b/src/util/virhook.c index 25d0783..ee19382 100644 --- a/src/util/virhook.c +++ b/src/util/virhook.c @@ -300,7 +300,8 @@ virHookCall(int driver, if (ret < 0) { /* Convert INTERNAL_ERROR into known error. */ virErrorPtr err = virGetLastError(); - virReportError(VIR_ERR_HOOK_SCRIPT_FAILED, "%s", err->message); + virReportError(VIR_ERR_HOOK_SCRIPT_FAILED, "%s", + err ? err->message : _("unknown error")); } virCommandFree(cmd); -- 1.9.3
Commit id '0d36a5d05' modified the code slightly, but removed the return value check thus causing Coverity to complain that this call was the only one where the return value wasn't checked. Since nothing was done previously if there was a failure, just use ignore_value here to pacify Coverity Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/domain_conf.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index b0caa76..fe9b986 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -19237,7 +19237,7 @@ virDomainDefFormatInternal(virDomainDefPtr def, virDomainResourceDefFormat(buf, def->resource); if (def->sysinfo) - virSysinfoFormat(buf, def->sysinfo); + ignore_value(virSysinfoFormat(buf, def->sysinfo)); if (def->os.bootloader) { virBufferEscapeString(buf, "<bootloader>%s</bootloader>\n", -- 1.9.3
Commit id 'cb88d433' refactored the calling sequence to use a thread; however, in doing so "lost" the check for if virNetSocketAccept returns failure. Since other code makes that check, Coverity complains. Although a false positive, adding back the failure check pacifies Coverity Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/libxl/libxl_migration.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/libxl/libxl_migration.c b/src/libxl/libxl_migration.c index 0caa3d0..4db89b6 100644 --- a/src/libxl/libxl_migration.c +++ b/src/libxl/libxl_migration.c @@ -133,8 +133,7 @@ libxlMigrateReceive(virNetSocketPtr sock, size_t i; /* Accept migration connection */ - virNetSocketAccept(sock, &client_sock); - if (client_sock == NULL) { + if (virNetSocketAccept(sock, &client_sock) < 0 || !client_sock) { virReportError(VIR_ERR_OPERATION_FAILED, "%s", _("Failed to accept migration connection")); goto fail; -- 1.9.3
On 12/01/2014 09:33 AM, John Ferlan wrote:
The issues found in the first 3 patches aren't "usually" seen, but I've had them sitting around awhile and figured they should be posted anyway since the last issue was seen from a recent change...
ACK series
John Ferlan (4): hotplug: Resolve Coverity FORWARD_NULL virhook: Resolve Coverity NULL_RETURNS domain_conf: Resolve Coverity CHECKED_RETURN libxl: Resolve Coverity CHECKED_RETURN
src/conf/domain_conf.c | 2 +- src/libxl/libxl_migration.c | 3 +-- src/qemu/qemu_hotplug.c | 12 ++++++------ src/util/virhook.c | 3 ++- 4 files changed, 10 insertions(+), 10 deletions(-)
-- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
participants (2)
-
Eric Blake -
John Ferlan