[libvirt] [PATCH] network: log error when <bandwidth> is requested for hostdev interfaces

This would previously be silently ignored. Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1319044 --- src/network/bridge_driver.c | 25 +++++++++++++++++++++++++ src/qemu/qemu_domain.c | 21 ++++++++++++++++----- 2 files changed, 41 insertions(+), 5 deletions(-) diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index bef8a78..0fd2095 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -3126,6 +3126,20 @@ networkValidate(virNetworkDriverStatePtr driver, def->name); return -1; } + + if (def->forward.type == VIR_NETWORK_FORWARD_HOSTDEV) { + for (i = 0; i < def->nPortGroups; i++) { + if (def->portGroups[i].bandwidth) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("unsupported <bandwidth> element " + "in <portgroup name='%s'> of " + "network '%s' with forward mode='%s'"), + def->portGroups[i].name, def->name, + virNetworkForwardTypeToString(def->forward.type)); + return -1; + } + } + } return 0; } @@ -4305,6 +4319,17 @@ networkAllocateActualDevice(virDomainDefPtr dom, goto error; } } + if (virDomainNetGetActualBandwidth(iface)) { + /* bandwidth configuration via libvirt is not supported for + * hostdev network devices + */ + if (actualType == VIR_DOMAIN_NET_TYPE_HOSTDEV) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("bandwidth settings are not supported " + "for hostdev interfaces")); + goto error; + } + } if (netdef) { netdef->connections++; diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index f7356a2..4e32251 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -2119,12 +2119,23 @@ qemuDomainDeviceDefPostParse(virDomainDeviceDefPtr dev, qemuCaps = virQEMUCapsCacheLookup(driver->qemuCapsCache, def->emulator); - if (dev->type == VIR_DOMAIN_DEVICE_NET && - dev->data.net->type != VIR_DOMAIN_NET_TYPE_HOSTDEV && - !dev->data.net->model) { - if (VIR_STRDUP(dev->data.net->model, - qemuDomainDefaultNetModel(def, qemuCaps)) < 0) + if (dev->type == VIR_DOMAIN_DEVICE_NET) { + virDomainNetDefPtr net = dev->data.net; + + if (net->type != VIR_DOMAIN_NET_TYPE_HOSTDEV && !net->model && + VIR_STRDUP(net->model, qemuDomainDefaultNetModel(def, qemuCaps)) < 0) + goto cleanup; + + if (net->type == VIR_DOMAIN_NET_TYPE_HOSTDEV && + virDomainNetGetActualBandwidth(net)) { + /* bandwidth configuration via libvirt is not supported + * for hostdev network devices + */ + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("bandwidth settings are not supported " + "for hostdev interfaces")); goto cleanup; + } } /* set default disk types and drivers */ -- 2.5.5

On Wed, May 11, 2016 at 12:18:51 -0400, Laine Stump wrote:
This would previously be silently ignored.
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1319044 --- src/network/bridge_driver.c | 25 +++++++++++++++++++++++++ src/qemu/qemu_domain.c | 21 ++++++++++++++++----- 2 files changed, 41 insertions(+), 5 deletions(-)
diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index bef8a78..0fd2095 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -3126,6 +3126,20 @@ networkValidate(virNetworkDriverStatePtr driver, def->name); return -1; } + + if (def->forward.type == VIR_NETWORK_FORWARD_HOSTDEV) { + for (i = 0; i < def->nPortGroups; i++) { + if (def->portGroups[i].bandwidth) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("unsupported <bandwidth> element " + "in <portgroup name='%s'> of " + "network '%s' with forward mode='%s'"), + def->portGroups[i].name, def->name, + virNetworkForwardTypeToString(def->forward.type)); + return -1; + } + } + }
Okay this part gets called in networkDefineXML, networkCreateXML and networkStartNetworkVirtual.
return 0; }
@@ -4305,6 +4319,17 @@ networkAllocateActualDevice(virDomainDefPtr dom, goto error; } } + if (virDomainNetGetActualBandwidth(iface)) { + /* bandwidth configuration via libvirt is not supported for + * hostdev network devices + */ + if (actualType == VIR_DOMAIN_NET_TYPE_HOSTDEV) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("bandwidth settings are not supported " + "for hostdev interfaces")); + goto error; + } + }
if (netdef) { netdef->connections++;
ACK to the code above.
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index f7356a2..4e32251 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -2119,12 +2119,23 @@ qemuDomainDeviceDefPostParse(virDomainDeviceDefPtr dev,
qemuCaps = virQEMUCapsCacheLookup(driver->qemuCapsCache, def->emulator);
- if (dev->type == VIR_DOMAIN_DEVICE_NET && - dev->data.net->type != VIR_DOMAIN_NET_TYPE_HOSTDEV && - !dev->data.net->model) { - if (VIR_STRDUP(dev->data.net->model, - qemuDomainDefaultNetModel(def, qemuCaps)) < 0) + if (dev->type == VIR_DOMAIN_DEVICE_NET) { + virDomainNetDefPtr net = dev->data.net; + + if (net->type != VIR_DOMAIN_NET_TYPE_HOSTDEV && !net->model && + VIR_STRDUP(net->model, qemuDomainDefaultNetModel(def, qemuCaps)) < 0) + goto cleanup; + + if (net->type == VIR_DOMAIN_NET_TYPE_HOSTDEV && + virDomainNetGetActualBandwidth(net)) { + /* bandwidth configuration via libvirt is not supported + * for hostdev network devices + */ + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("bandwidth settings are not supported " + "for hostdev interfaces")); goto cleanup;
NACK to this part. This makes vm configs that were previously accepted vanish. This can only be a start-time check.

On 05/12/2016 01:54 AM, Peter Krempa wrote:
On Wed, May 11, 2016 at 12:18:51 -0400, Laine Stump wrote:
This would previously be silently ignored.
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1319044 --- src/network/bridge_driver.c | 25 +++++++++++++++++++++++++ src/qemu/qemu_domain.c | 21 ++++++++++++++++----- 2 files changed, 41 insertions(+), 5 deletions(-)
diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index bef8a78..0fd2095 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -3126,6 +3126,20 @@ networkValidate(virNetworkDriverStatePtr driver, def->name); return -1; } + + if (def->forward.type == VIR_NETWORK_FORWARD_HOSTDEV) { + for (i = 0; i < def->nPortGroups; i++) { + if (def->portGroups[i].bandwidth) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("unsupported <bandwidth> element " + "in <portgroup name='%s'> of " + "network '%s' with forward mode='%s'"), + def->portGroups[i].name, def->name, + virNetworkForwardTypeToString(def->forward.type)); + return -1; + } + } + } Okay this part gets called in networkDefineXML, networkCreateXML and networkStartNetworkVirtual.
To further the logic of your NACK on the bit at the end - if we do this, the network would fail to auto-start. I guess that's better than disappearing though, so acceptable.
return 0; }
@@ -4305,6 +4319,17 @@ networkAllocateActualDevice(virDomainDefPtr dom, goto error; } } + if (virDomainNetGetActualBandwidth(iface)) { + /* bandwidth configuration via libvirt is not supported for + * hostdev network devices + */ + if (actualType == VIR_DOMAIN_NET_TYPE_HOSTDEV) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("bandwidth settings are not supported " + "for hostdev interfaces")); + goto error; + } + }
if (netdef) { netdef->connections++;
ACK to the code above.
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index f7356a2..4e32251 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -2119,12 +2119,23 @@ qemuDomainDeviceDefPostParse(virDomainDeviceDefPtr dev,
qemuCaps = virQEMUCapsCacheLookup(driver->qemuCapsCache, def->emulator);
- if (dev->type == VIR_DOMAIN_DEVICE_NET && - dev->data.net->type != VIR_DOMAIN_NET_TYPE_HOSTDEV && - !dev->data.net->model) { - if (VIR_STRDUP(dev->data.net->model, - qemuDomainDefaultNetModel(def, qemuCaps)) < 0) + if (dev->type == VIR_DOMAIN_DEVICE_NET) { + virDomainNetDefPtr net = dev->data.net; + + if (net->type != VIR_DOMAIN_NET_TYPE_HOSTDEV && !net->model && + VIR_STRDUP(net->model, qemuDomainDefaultNetModel(def, qemuCaps)) < 0) + goto cleanup; + + if (net->type == VIR_DOMAIN_NET_TYPE_HOSTDEV && + virDomainNetGetActualBandwidth(net)) { + /* bandwidth configuration via libvirt is not supported + * for hostdev network devices + */ + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("bandwidth settings are not supported " + "for hostdev interfaces")); goto cleanup; NACK to this part. This makes vm configs that were previously accepted vanish. This can only be a start-time check.
Right. Sigh. I've run into this in the past (find a nonsensical config combination that's ignored, want to make it into an error so the people aren't misled, but need to continue ignoring it at parse (i.e. and therefore define) time, so can only error out when they get around to starting it. We really need a way tomove things like this into parse-time errors. Maybe a flag propogated through the parse to ignore harmless errors which is set during libvirtd restart but cleared when defining/creating? (I don't really like that idea, but can't come up with anything better right now; the "broken domain" state doesn't really cut it, since that would leave you with a partially parsed domain that you couldn't edit via the standard APIs). Since the 2nd fragment handles validation of all interfaces (even those that aren't type='network') I'll reduce the patch to the 1st two fragments and push it. Thanks for the review!

On Thu, May 12, 2016 at 17:11:36 -0400, Laine Stump wrote:
On 05/12/2016 01:54 AM, Peter Krempa wrote:
On Wed, May 11, 2016 at 12:18:51 -0400, Laine Stump wrote:
[...]
diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index bef8a78..0fd2095 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -3126,6 +3126,20 @@ networkValidate(virNetworkDriverStatePtr driver,
[...]
Okay this part gets called in networkDefineXML, networkCreateXML and networkStartNetworkVirtual.
To further the logic of your NACK on the bit at the end - if we do this, the network would fail to auto-start. I guess that's better than disappearing though, so acceptable.
Yes a failed startup is definitely better than missing config.
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index f7356a2..4e32251 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -2119,12 +2119,23 @@ qemuDomainDeviceDefPostParse(virDomainDeviceDefPtr dev,
qemuCaps = virQEMUCapsCacheLookup(driver->qemuCapsCache, def->emulator);
- if (dev->type == VIR_DOMAIN_DEVICE_NET && - dev->data.net->type != VIR_DOMAIN_NET_TYPE_HOSTDEV && - !dev->data.net->model) { - if (VIR_STRDUP(dev->data.net->model, - qemuDomainDefaultNetModel(def, qemuCaps)) < 0) + if (dev->type == VIR_DOMAIN_DEVICE_NET) { + virDomainNetDefPtr net = dev->data.net; + + if (net->type != VIR_DOMAIN_NET_TYPE_HOSTDEV && !net->model && + VIR_STRDUP(net->model, qemuDomainDefaultNetModel(def, qemuCaps)) < 0) + goto cleanup; + + if (net->type == VIR_DOMAIN_NET_TYPE_HOSTDEV && + virDomainNetGetActualBandwidth(net)) { + /* bandwidth configuration via libvirt is not supported + * for hostdev network devices + */ + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("bandwidth settings are not supported " + "for hostdev interfaces")); goto cleanup; NACK to this part. This makes vm configs that were previously accepted vanish. This can only be a start-time check.
Right. Sigh. I've run into this in the past (find a nonsensical config combination that's ignored, want to make it into an error so the people aren't misled, but need to continue ignoring it at parse (i.e. and therefore define) time, so can only error out when they get around to starting it. We really need a way tomove things like this into parse-time errors. Maybe a flag propogated through the parse to ignore
Defintely not parse time. I'd call them "define time".
harmless errors which is set during libvirtd restart but cleared when defining/creating? (I don't really like that idea, but can't come up
No a flag propagated through the parser would lead to more spaghetti code. The parser itself is ugly enough in the current state.
with anything better right now; the "broken domain" state doesn't really cut it, since that would leave you with a partially parsed domain that you couldn't edit via the standard APIs).
As you'd need to add a flag at the places where the domain gets defined anyways so you can as easily add a validation function to those places. The driver specific validation function will then call a global validation function. As the name implies, the validation function shall not attempt to modify the definition of any way though. Advantage of this is that you can then call the same validation function when starting the VM so you don't have to duplicate the checks between the parser and VM startup code which are disjunct. Peter
participants (2)
-
Laine Stump
-
Peter Krempa