[libvirt] [PATCH 0/2] Fix a VM startup failure: QOS must be defined for network 'default'
Caused by patching a coverity false positive in commit f4db846c32c0a1e99a0f62b340273e48f8a98ed3. Erik Skultety (2): Revert "network: Check for QOS before blindly using it" util: virNetDevBandwidthPlug: Drop ATTRIBUTE_UNUSED(4) src/network/bridge_driver.c | 14 -------------- src/util/virnetdevbandwidth.h | 3 +-- 2 files changed, 1 insertion(+), 16 deletions(-) -- 2.23.0
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@redhat.com> --- 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
On 11/22/19 1:08 PM, Erik Skultety wrote:
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@redhat.com> ---
Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>
Em sex., 22 de nov. de 2019 às 14:09, Erik Skultety <eskultet@redhat.com> escreveu:
This reverts commit f4db846c32c0a1e99a0f62b340273e48f8a98ed3.
This patch results in the following error when trying to start essentially any VM with default network:
This kind of issue should be caught by test driver, shouldn't it? -- Julio Faracco
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@redhat.com> --- 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@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On Sun, Nov 24, 2019 at 06:12:58PM -0200, Julio Faracco wrote:
Em sex., 22 de nov. de 2019 às 14:09, Erik Skultety <eskultet@redhat.com> escreveu:
This reverts commit f4db846c32c0a1e99a0f62b340273e48f8a98ed3.
This patch results in the following error when trying to start essentially any VM with default network:
This kind of issue should be caught by test driver, shouldn't it?
Test driver is supposed to be completely isolated from the host. In this case, the error comes from the bridge_driver which does interact with the underlying host so no, test driver is of no help here. Erik
On 11/24/19 9:12 PM, Julio Faracco wrote:
Em sex., 22 de nov. de 2019 às 14:09, Erik Skultety <eskultet@redhat.com> escreveu:
This reverts commit f4db846c32c0a1e99a0f62b340273e48f8a98ed3.
This patch results in the following error when trying to start essentially any VM with default network:
This kind of issue should be caught by test driver, shouldn't it?
Not really. The test driver doesn't call APIs of other drivers. It's only providing sensible outputs for APIs inputs without doing any actual work. An integration test would have caught this though. Michal
On Fri, Nov 22, 2019 at 17:08:59 +0100, Erik Skultety wrote:
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@redhat.com> --- src/network/bridge_driver.c | 14 -------------- 1 file changed, 14 deletions(-)
Reviewed-by: Jiri Denemark <jdenemar@redhat.com>
On 11/22/19 5:08 PM, Erik Skultety wrote:
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@redhat.com> --- src/network/bridge_driver.c | 14 -------------- 1 file changed, 14 deletions(-)
Reviewed-by: Michal Privoznik <mprivozn@redhat.com> Michal
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@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) -- 2.23.0
On 11/22/19 1:09 PM, 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 argument - virNetDevBandwidthPlug is also called from a single place only
Signed-off-by: Erik Skultety <eskultet@redhat.com> ---
Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>
On 11/22/19 5:09 PM, 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 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)
Le sigh. Reviewed-by: Michal Privoznik <mprivozn@redhat.com> Michal
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...
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 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: 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
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...
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
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
participants (6)
-
Daniel Henrique Barboza -
Erik Skultety -
Jiri Denemark -
John Ferlan -
Julio Faracco -
Michal Privoznik