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@xxxxxxxxxx>
> > > ---
> > > 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
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
