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 <eskul...@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