[libvirt] [PATCH 0/3] A few more Coverity patches
Finally got Coverity to build with the recent build env changes. With doing that Coverity re-examines everything and bridge_driver generated a couple of longer term things. The virFileRewrite is from more recent changes. John Ferlan (3): util: Remove unnecessary check in virFileRewrite network: Use local variables in networkUpdatePortBandwidth network: Check for QOS before blindly using it src/network/bridge_driver.c | 31 ++++++++++++++++++++++--------- src/util/virfile.c | 3 +-- 2 files changed, 23 insertions(+), 11 deletions(-) -- 2.20.1
Since g_strdup_printf will abort, we know @newfile won't be NULL. Found by Coverity Signed-off-by: John Ferlan <jferlan@redhat.com> --- 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
On 11/14/19 6:58 PM, John Ferlan wrote:
Since g_strdup_printf will abort, we know @newfile won't be NULL.
Found by Coverity
Signed-off-by: John Ferlan <jferlan@redhat.com> ---
Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>
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@redhat.com> --- 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/14/19 6:58 PM, John Ferlan wrote:
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@redhat.com> --- 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) {
Nit: both old_floor and new_floor are unsigned, thus comparing them to > 0 or doing (old_floor && new_floor) like it was being done before is the same thing. Same deal with the 'if (new_floor > 0)' down below. I don't mind the extra clarity though. Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>
/* 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,
On 11/18/19 2:27 PM, Daniel Henrique Barboza wrote:
On 11/14/19 6:58 PM, John Ferlan wrote:
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@redhat.com> --- 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) {
Nit: both old_floor and new_floor are unsigned, thus comparing them to > 0 or doing (old_floor && new_floor) like it was being done before is the same thing. Same deal with the 'if (new_floor > 0)' down below.
I don't mind the extra clarity though.
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
On Mon, Nov 18, 2019 at 14:47:03 +0100, Michal Privoznik wrote:
On 11/18/19 2:27 PM, Daniel Henrique Barboza wrote:
On 11/14/19 6:58 PM, John Ferlan wrote:
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@redhat.com> --- 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) {
Nit: both old_floor and new_floor are unsigned, thus comparing them to > 0 or doing (old_floor && new_floor) like it was being done before is the same thing. Same deal with the 'if (new_floor > 0)' down below.
I don't mind the extra clarity though.
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)'.
We also encourage it officially in our coding style: https://libvirt.org/hacking.html#conditions
On 11/18/19 10:53 AM, Peter Krempa wrote:
On Mon, Nov 18, 2019 at 14:47:03 +0100, Michal Privoznik wrote:
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)'.
I'll start paying more attention to it in the code I'll be writing too.
We also encourage it officially in our coding style:
I wasn't aware that (hasFoos == true) was a valid/encouraged format. I took flak in QEMU doing these kind of things and got used to do the short format whenever possible. Interesting. DHB
On Mon, Nov 18, 2019 at 11:28:08 -0300, Daniel Henrique Barboza wrote:
On 11/18/19 10:53 AM, Peter Krempa wrote:
On Mon, Nov 18, 2019 at 14:47:03 +0100, Michal Privoznik wrote:
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)'.
I'll start paying more attention to it in the code I'll be writing too.
We also encourage it officially in our coding style:
I wasn't aware that (hasFoos == true) was a valid/encouraged format. I took flak in QEMU doing these kind of things and got used to do the short format whenever possible. Interesting.
Note that if (hasFoos) is also valid without any preference towards one or the other so the short form is good and in most cases not confusing. What's specifically discouraged is testing integers without a comparison operator as in such case it obscures that there might be a value of significance other than the usual boolean cases NULL, !NULL in case of pointers of false, true in case of booleans.
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@redhat.com> --- 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
On 11/14/19 6:58 PM, John Ferlan wrote:
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@redhat.com> --- src/network/bridge_driver.c | 14 ++++++++++++++ 1 file changed, 14 insertions(+)
Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>
participants (4)
-
Daniel Henrique Barboza -
John Ferlan -
Michal Privoznik -
Peter Krempa