Since g_strdup_printf will abort, we know @newfile won't be NULL. Found by Coverity Signed-off-by: John Ferlan <jferlan(a)redhat.com&gt; --- src/util/virfile.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/util/virfile.c b/src/util/virfile.c index cb6cfc0fe7..fca7ff9d35 100644 --- a/src/util/virfile.c +++ b/src/util/virfile.c @@ -543,8 +543,7 @@ virFileRewrite(const char *path, cleanup: VIR_FORCE_CLOSE(fd); - if (newfile) - unlink(newfile); + unlink(newfile); return ret; } -- 2.20.1

We go through the trouble of checking {old|new}Bandwidth[->in] and storing the result in local @old_floor and @new_floor, but then we don't use them. Instead we make derefs to the longer name. This caused Coverity to note dereferencing newBandwidth->in without first checking @newBandwidth like was done for new_floor could cause a NULL dereference. Signed-off-by: John Ferlan <jferlan(a)redhat.com&gt; --- src/network/bridge_driver.c | 17 ++++++++--------- 1 file changed, 8 insertions(+), 9 deletions(-) diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index 7ee5d7ee53..68bb916501 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -5380,19 +5380,18 @@ networkUpdatePortBandwidth(virNetworkObjPtr obj, /* Okay, there are three possible scenarios: */ - if (oldBandwidth && oldBandwidth->in && oldBandwidth->in->floor && - newBandwidth->in && newBandwidth->in->floor) { + if (old_floor > 0 && new_floor > 0) { /* Either we just need to update @floor .. */ if (virNetDevBandwidthUpdateRate(def->bridge, *class_id, def->bandwidth, - newBandwidth->in->floor) < 0) + new_floor) < 0) return -1; tmp_floor_sum = virNetworkObjGetFloorSum(obj); - tmp_floor_sum -= oldBandwidth->in->floor; - tmp_floor_sum += newBandwidth->in->floor; + tmp_floor_sum -= old_floor; + tmp_floor_sum += new_floor; virNetworkObjSetFloorSum(obj, tmp_floor_sum); new_rate -= tmp_floor_sum; @@ -5401,17 +5400,17 @@ networkUpdatePortBandwidth(virNetworkObjPtr obj, virNetworkObjSaveStatus(driver->stateDir, obj, network_driver->xmlopt) < 0) { /* Ouch, rollback */ - tmp_floor_sum -= newBandwidth->in->floor; - tmp_floor_sum += oldBandwidth->in->floor; + tmp_floor_sum -= new_floor; + tmp_floor_sum += old_floor; virNetworkObjSetFloorSum(obj, tmp_floor_sum); ignore_value(virNetDevBandwidthUpdateRate(def->bridge, *class_id, def->bandwidth, - oldBandwidth->in->floor)); + old_floor)); return -1; } - } else if (newBandwidth->in && newBandwidth->in->floor) { + } else if (new_floor > 0) { /* .. or we need to plug in new .. */ if (networkPlugBandwidthImpl(obj, mac, newBandwidth, -- 2.20.1

On 11/18/19 2:27 PM, Daniel Henrique Barboza wrote: I believe it was John who persuaded us to use explicit integer comparison for integer variables. The idea is that it's clear from the check itself if we are comparing integers or pointers. And I agree with him - in my new code I always use 'if (x > 0)' or 'if (x != 0)' instead of 'if (x)' or 'if (!x)'. Michal

If networkAllocatePort calls networkPlugBandwidth eventually the port->bandwidth would be passed to virNetDevBandwidthPlug which requires that the parameter is non-NULL. Coverity additionally notes that since (!port->bandwidth) is checked earlier in the networkAllocatePort method that the subsequent call to blindly use if for a function that requires it needs to check. Signed-off-by: John Ferlan <jferlan(a)redhat.com&gt; --- src/network/bridge_driver.c | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index 68bb916501..9c49c70564 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -4567,6 +4567,13 @@ networkAllocatePort(virNetworkObjPtr obj, return -1; } + if (!port->bandwidth) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("QOS must be defined for network '%s'"), + netdef->name); + return -1; + } + if (networkPlugBandwidth(obj, &port->mac, port->bandwidth, &port->class_id) < 0) return -1; break; @@ -4633,6 +4640,13 @@ networkAllocatePort(virNetworkObjPtr obj, } } + if (!port->bandwidth) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("QOS must be defined for network '%s'"), + netdef->name); + return -1; + } + if (networkPlugBandwidth(obj, &port->mac, port->bandwidth, &port->class_id) < 0) return -1; break; -- 2.20.1