[libvirt] [PATCH 2/2] util: virNetDevBandwidthPlug: Drop ATTRIBUTE_UNUSED(4)

[Erik Skultety]

On Mon, Nov 25, 2019 at 09:30:26AM +0100, Jiri Denemark wrote:
> On Fri, Nov 22, 2019 at 17:09:00 +0100, Erik Skultety wrote:
> > Since we know for sure that the @bandwidth parameter is properly handled
> > in networkCheckBandwidth (@ifaceBand), it cannot ever be NULL, but
> > coverity doesn't see this fact. In order to prevent coverity from
> > reporting a false positive here, drop ATTRIBUTE_UNUSED for this specific
>
> s/ATTRIBUTE_UNUSED/ATTRIBUTE_NONNULL/ both in the subject and the commit
> message, however...

Oops, don't know how that happened... :)

>
> > argument - virNetDevBandwidthPlug is also called from a single place only
> >
> > Signed-off-by: Erik Skultety <eskultet at redhat.com>
> > ---
> >  src/util/virnetdevbandwidth.h | 3 +--
> >  1 file changed, 1 insertion(+), 2 deletions(-)
> >
> > diff --git a/src/util/virnetdevbandwidth.h b/src/util/virnetdevbandwidth.h
> > index 19323c5ed2..59d7513286 100644
> > --- a/src/util/virnetdevbandwidth.h
> > +++ b/src/util/virnetdevbandwidth.h
> > @@ -55,8 +55,7 @@ int virNetDevBandwidthPlug(const char *brname,
> >                             const virMacAddr *ifmac_ptr,
> >                             virNetDevBandwidthPtr bandwidth,
> >                             unsigned int id)
> > -    ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(3) ATTRIBUTE_NONNULL(4)
> > -    G_GNUC_WARN_UNUSED_RESULT;
> > +    ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(3) G_GNUC_WARN_UNUSED_RESULT;
> >
> >  int virNetDevBandwidthUnplug(const char *brname,
> >                               unsigned int id)
>
> I don't think this is a good solution. From the point of view of this
> function ATTRIBUTE_NONNULL is correct for all three parameters and

Well, first of all ATTRIBUTE_NONNULL has a very limited use case and most of
the time only coverity complains about something which relates to that
attribute. My reasoning was that since in this case we can guarantee that the
argument will never be NULL, we can drop it. However...

> removing it for one of them does not make sense.
>
> Since we just want to silence coverity's false positive, we could use
> sa_assert in the right place to teach coverity virNetDevBandwidthPlug is
> not ever called with bandwidth == NULL:

...I actually like ^your suggestion here and if it turns out it works, I'll
happily go with that one instead. Putting John on CC. I'll push 1/2 in the
meantime.

John, could you please try the patch below and see whether coverity stops
complaining about the issue that your original patch tried to address?

Erik

>
>     diff --git i/src/network/bridge_driver.c w/src/network/bridge_driver.c
>     index 9b12b98d88..52c8ba4c73 100644
>     --- i/src/network/bridge_driver.c
>     +++ w/src/network/bridge_driver.c
>     @@ -5263,6 +5263,8 @@ networkPlugBandwidth(virNetworkObjPtr obj,
>              /* no QoS needs to be set; claim success */
>              return 0;
>
>     +    sa_assert(ifaceBand);
>     +
>          virMacAddrFormat(mac, ifmac);
>
>          if (networkPlugBandwidthImpl(obj, mac, ifaceBand, class_id, new_rate) < 0)
>
> Jirka