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