Re: [libvirt] [PATCH 2/2] util: virNetDevBandwidthPlug: Drop ATTRIBUTE_UNUSED(4)
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(a)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