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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




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




[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]

  Powered by Linux