Re: [libvirt] [PATCH 2/3] network: Use local variables in networkUpdatePortBandwidth

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(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) { > > > 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)'.