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

Erik Skultety posted 2 patches 13 weeks ago

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

Posted by Erik Skultety 13 weeks ago
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
argument - virNetDevBandwidthPlug is also called from a single place only

Signed-off-by: Erik Skultety <eskultet@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)
-- 
2.23.0

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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

Posted by Jiri Denemark 12 weeks ago
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...

> argument - virNetDevBandwidthPlug is also called from a single place only
> 
> Signed-off-by: Erik Skultety <eskultet@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
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:

    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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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

Posted by Erik Skultety 12 weeks ago
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@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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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

Posted by John Ferlan 12 weeks ago

On 11/25/19 3:40 AM, Erik Skultety wrote:
> 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@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...
> 

Enable static analysis in your build environment, e.g.:

  ./autogen.sh --system lv_cv_static_analysis=yes

then try a build...

The *_NONNULL has been debated before - at least w/r/t passing a NULL
vs. a value being NULL. In this case I could have tried removing the
_NONNNUL, but chose not to mainly because that hasn't been accepted in
the past and the check seemed right (and of course got rid of the
Coverity error).

>> 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?
> 


Anyway, apologies for the issue and excess noise (or as a pun
bandwidth)....

Suffice to say the solution for Coverity is "complicated". It turns out
to be not at as easy as an sa_assert because of the _NONNULL(4) as other
checks fail. I'll just keep it local and out of the mainline libvirt.


John


> 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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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

Posted by Michal Privoznik 12 weeks ago
On 11/22/19 5:09 PM, 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
> argument - virNetDevBandwidthPlug is also called from a single place only
> 
> Signed-off-by: Erik Skultety <eskultet@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)
> 

Le sigh.

Reviewed-by: Michal Privoznik <mprivozn@redhat.com>

Michal

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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

Posted by Daniel Henrique Barboza 13 weeks ago

On 11/22/19 1:09 PM, 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
> argument - virNetDevBandwidthPlug is also called from a single place only
> 
> Signed-off-by: Erik Skultety <eskultet@redhat.com>
> ---

Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list