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
