This reverts commit f4db846c32c0a1e99a0f62b340273e48f8a98ed3. This patch results in the following error when trying to start essentially any VM with default network:

unsupported configuration: QOS must be defined for network 'default' Coverity didn't see that the bandwidth == NULL it complained about in virNetDevBandwidthPlug was already checked properly in networkCheckBandwidth, thus causing networkPlugBandwidth to return 0 and finish before a call to virNetDevBandwidthPlug would have been even made. Signed-off-by: Erik Skultety <eskultet(a)redhat.com&gt; --- src/network/bridge_driver.c | 14 -------------- 1 file changed, 14 deletions(-) diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index 9c49c70564..68bb916501 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -4567,13 +4567,6 @@ networkAllocatePort(virNetworkObjPtr obj, return -1; } - if (!port->bandwidth) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("QOS must be defined for network '%s'"), - netdef->name); - return -1; - } - if (networkPlugBandwidth(obj, &port->mac, port->bandwidth, &port->class_id) < 0) return -1; break; @@ -4640,13 +4633,6 @@ networkAllocatePort(virNetworkObjPtr obj, } } - if (!port->bandwidth) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("QOS must be defined for network '%s'"), - netdef->name); - return -1; - } - if (networkPlugBandwidth(obj, &port->mac, port->bandwidth, &port->class_id) < 0) return -1; break; -- 2.23.0 -- libvir-list mailing list libvir-list(a)redhat.com https://www.redhat.com/mailman/listinfo/libvir-list

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

argument - virNetDevBandwidthPlug is also called from a single place only Signed-off-by: Erik Skultety <eskultet(a)redhat.com&gt; --- 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)

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&gt; >> --- >> 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...

> 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?

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