[libvirt] [PATCH 0/2] Forbid starting VMs with bandwidth on unsupported vNIC type
The patch 1/2 is trivial. The patch 2/2 is controversial and I expect heavy discussion on it :-) Thing is, there might be somebody misusing our XML to store the QoS setting, and do the actual setting themselves in startup hook. Now with this patch, we will forbid such usage scenario. Michal Privoznik (2): conf: Increase virNetDevBandwidthParse intelligence qemu, lxc: Fail explicitly if setting QoS on unsupported vNIC types src/conf/domain_conf.c | 10 ++++++---- src/conf/netdev_bandwidth_conf.c | 38 +++++++++++++++++++++++--------------- src/conf/netdev_bandwidth_conf.h | 7 ++++--- src/conf/network_conf.c | 7 +++---- src/lxc/lxc_driver.c | 20 +++++++++++++++----- src/lxc/lxc_process.c | 20 +++++++++++++++----- src/qemu/qemu_command.c | 21 +++++++++++++++------ src/qemu/qemu_hotplug.c | 20 +++++++++++++++----- tests/virnetdevbandwidthtest.c | 12 ++++++------ 9 files changed, 102 insertions(+), 53 deletions(-) -- 2.0.5
There's this function virNetDevBandwidthParse which parses the bandwidth XML snippet. But it's not clever much. For the following XML it allocates the virNetDevBandwidth structure even though it's completely empty: <bandwidth> </bandwidth> Later in the code there are some places where we check if bandwidth was set or not. And since we obtained pointer from the parsing function we think that it is when in fact it isn't. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/domain_conf.c | 10 ++++++---- src/conf/netdev_bandwidth_conf.c | 38 +++++++++++++++++++++++--------------- src/conf/netdev_bandwidth_conf.h | 7 ++++--- src/conf/network_conf.c | 7 +++---- tests/virnetdevbandwidthtest.c | 12 ++++++------ 5 files changed, 42 insertions(+), 32 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index d1a483a..8f333ef 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -7354,8 +7354,9 @@ virDomainActualNetDefParseXML(xmlNodePtr node, bandwidth_node = virXPathNode("./bandwidth", ctxt); if (bandwidth_node && - !(actual->bandwidth = virNetDevBandwidthParse(bandwidth_node, - actual->type))) + virNetDevBandwidthParse(&actual->bandwidth, + bandwidth_node, + actual->type) < 0) goto error; vlanNode = virXPathNode("./vlan", ctxt); @@ -7612,8 +7613,9 @@ virDomainNetDefParseXML(virDomainXMLOptionPtr xmlopt, goto error; } } else if (xmlStrEqual(cur->name, BAD_CAST "bandwidth")) { - if (!(def->bandwidth = virNetDevBandwidthParse(cur, - def->type))) + if (virNetDevBandwidthParse(&def->bandwidth, + cur, + def->type) < 0) goto error; } else if (xmlStrEqual(cur->name, BAD_CAST "vlan")) { if (virNetDevVlanParse(cur, ctxt, &def->vlan) < 0) diff --git a/src/conf/netdev_bandwidth_conf.c b/src/conf/netdev_bandwidth_conf.c index 63a39e3..c3e0f9f 100644 --- a/src/conf/netdev_bandwidth_conf.c +++ b/src/conf/netdev_bandwidth_conf.c @@ -102,6 +102,7 @@ virNetDevBandwidthParseRate(xmlNodePtr node, virNetDevBandwidthRatePtr rate) /** * virNetDevBandwidthParse: + * @bandwidth: parsed bandwidth * @node: XML node * @net_type: one of virDomainNetType * @@ -111,21 +112,23 @@ virNetDevBandwidthParseRate(xmlNodePtr node, virNetDevBandwidthRatePtr rate) * * Returns !NULL on success, NULL on error. */ -virNetDevBandwidthPtr -virNetDevBandwidthParse(xmlNodePtr node, +int +virNetDevBandwidthParse(virNetDevBandwidthPtr *bandwidth, + xmlNodePtr node, int net_type) { + int ret = -1; virNetDevBandwidthPtr def = NULL; xmlNodePtr cur; xmlNodePtr in = NULL, out = NULL; if (VIR_ALLOC(def) < 0) - return NULL; + return ret; if (!node || !xmlStrEqual(node->name, BAD_CAST "bandwidth")) { virReportError(VIR_ERR_INVALID_ARG, "%s", _("invalid argument supplied")); - goto error; + goto cleanup; } cur = node->children; @@ -137,7 +140,7 @@ virNetDevBandwidthParse(xmlNodePtr node, virReportError(VIR_ERR_XML_DETAIL, "%s", _("Only one child <inbound> " "element allowed")); - goto error; + goto cleanup; } in = cur; } else if (xmlStrEqual(cur->name, BAD_CAST "outbound")) { @@ -145,7 +148,7 @@ virNetDevBandwidthParse(xmlNodePtr node, virReportError(VIR_ERR_XML_DETAIL, "%s", _("Only one child <outbound> " "element allowed")); - goto error; + goto cleanup; } out = cur; } @@ -156,11 +159,11 @@ virNetDevBandwidthParse(xmlNodePtr node, if (in) { if (VIR_ALLOC(def->in) < 0) - goto error; + goto cleanup; if (virNetDevBandwidthParseRate(in, def->in) < 0) { /* helper reported error for us */ - goto error; + goto cleanup; } if (def->in->floor && net_type != VIR_DOMAIN_NET_TYPE_NETWORK) { @@ -174,32 +177,37 @@ virNetDevBandwidthParse(xmlNodePtr node, _("floor attribute is supported only for " "interfaces of type network")); } - goto error; + goto cleanup; } } if (out) { if (VIR_ALLOC(def->out) < 0) - goto error; + goto cleanup; if (virNetDevBandwidthParseRate(out, def->out) < 0) { /* helper reported error for us */ - goto error; + goto cleanup; } if (def->out->floor) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("'floor' attribute allowed " "only in <inbound> element")); - goto error; + goto cleanup; } } - return def; + if (!def->in && !def->out) + VIR_FREE(def); - error: + *bandwidth = def; + def = NULL; + ret = 0; + + cleanup: virNetDevBandwidthFree(def); - return NULL; + return ret; } static int diff --git a/src/conf/netdev_bandwidth_conf.h b/src/conf/netdev_bandwidth_conf.h index 60dacc6..6cbf4ae 100644 --- a/src/conf/netdev_bandwidth_conf.h +++ b/src/conf/netdev_bandwidth_conf.h @@ -29,9 +29,10 @@ # include "virxml.h" # include "domain_conf.h" -virNetDevBandwidthPtr virNetDevBandwidthParse(xmlNodePtr node, - int net_type) - ATTRIBUTE_NONNULL(1) ATTRIBUTE_RETURN_CHECK; +int virNetDevBandwidthParse(virNetDevBandwidthPtr *bandwidth, + xmlNodePtr node, + int net_type) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_RETURN_CHECK; int virNetDevBandwidthFormat(virNetDevBandwidthPtr def, virBufferPtr buf) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2); diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c index ddb5c07..26fe18d 100644 --- a/src/conf/network_conf.c +++ b/src/conf/network_conf.c @@ -1649,9 +1649,8 @@ virNetworkPortGroupParseXML(virPortGroupDefPtr def, bandwidth_node = virXPathNode("./bandwidth", ctxt); if (bandwidth_node && - !(def->bandwidth = virNetDevBandwidthParse(bandwidth_node, -1))) { + virNetDevBandwidthParse(&def->bandwidth, bandwidth_node, -1) < 0) goto cleanup; - } vlanNode = virXPathNode("./vlan", ctxt); if (vlanNode && virNetDevVlanParse(vlanNode, ctxt, &def->vlan) < 0) @@ -2088,8 +2087,8 @@ virNetworkDefParseXML(xmlXPathContextPtr ctxt) /* Parse network domain information */ def->domain = virXPathString("string(./domain[1]/@name)", ctxt); - if ((bandwidthNode = virXPathNode("./bandwidth", ctxt)) != NULL && - (def->bandwidth = virNetDevBandwidthParse(bandwidthNode, -1)) == NULL) + if ((bandwidthNode = virXPathNode("./bandwidth", ctxt)) && + virNetDevBandwidthParse(&def->bandwidth, bandwidthNode, -1) < 0) goto error; vlanNode = virXPathNode("./vlan", ctxt); diff --git a/tests/virnetdevbandwidthtest.c b/tests/virnetdevbandwidthtest.c index cd24442..3b46455 100644 --- a/tests/virnetdevbandwidthtest.c +++ b/tests/virnetdevbandwidthtest.c @@ -43,6 +43,7 @@ struct testSetStruct { #define PARSE(xml, var) \ do { \ + int rc; \ xmlDocPtr doc; \ xmlXPathContextPtr ctxt = NULL; \ \ @@ -54,11 +55,12 @@ struct testSetStruct { &ctxt))) \ goto cleanup; \ \ - (var) = virNetDevBandwidthParse(ctxt->node, \ - VIR_DOMAIN_NET_TYPE_NETWORK); \ + rc = virNetDevBandwidthParse(&(var), \ + ctxt->node, \ + VIR_DOMAIN_NET_TYPE_NETWORK); \ xmlFreeDoc(doc); \ xmlXPathFreeContext(ctxt); \ - if (!(var)) \ + if (rc < 0) \ goto cleanup; \ } while (0) @@ -127,9 +129,7 @@ mymain(void) DO_TEST_SET(NULL, NULL); - DO_TEST_SET(("<bandwidth/>"), - (TC " qdisc del dev eth0 root\n" - TC " qdisc del dev eth0 ingress\n")); + DO_TEST_SET("<bandwidth/>", NULL); DO_TEST_SET(("<bandwidth>" " <inbound average='1024'/>" -- 2.0.5
On 01/07/2015 12:31 PM, Michal Privoznik wrote:
There's this function virNetDevBandwidthParse which parses the bandwidth XML snippet. But it's not clever much. For the following XML it allocates the virNetDevBandwidth structure even though it's completely empty:
<bandwidth> </bandwidth>
Later in the code there are some places where we check if bandwidth was set or not. And since we obtained pointer from the parsing function we think that it is when in fact it isn't.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/domain_conf.c | 10 ++++++---- src/conf/netdev_bandwidth_conf.c | 38 +++++++++++++++++++++++--------------- src/conf/netdev_bandwidth_conf.h | 7 ++++--- src/conf/network_conf.c | 7 +++---- tests/virnetdevbandwidthtest.c | 12 ++++++------ 5 files changed, 42 insertions(+), 32 deletions(-)
ACK John
https://bugzilla.redhat.com/show_bug.cgi?id=1165993 So, there are still plenty of vNIC types that we don't know how to set bandwidth on. Let's fail explicitly in case user has requested it instead of pretending everything Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/lxc/lxc_driver.c | 20 +++++++++++++++----- src/lxc/lxc_process.c | 20 +++++++++++++++----- src/qemu/qemu_command.c | 21 +++++++++++++++------ src/qemu/qemu_hotplug.c | 20 +++++++++++++++----- 4 files changed, 60 insertions(+), 21 deletions(-) diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c index 51c664f..10a4a7e 100644 --- a/src/lxc/lxc_driver.c +++ b/src/lxc/lxc_driver.c @@ -4139,6 +4139,7 @@ lxcDomainAttachDeviceNetLive(virConnectPtr conn, virLXCDomainObjPrivatePtr priv = vm->privateData; int ret = -1; int actualType; + virNetDevBandwidthPtr actualBandwidth; char *veth = NULL; if (!priv->initpid) { @@ -4186,11 +4187,20 @@ lxcDomainAttachDeviceNetLive(virConnectPtr conn, _("Network device type is not supported")); goto cleanup; } - /* set network bandwidth */ - if (virNetDevSupportBandwidth(actualType) && - virNetDevBandwidthSet(net->ifname, - virDomainNetGetActualBandwidth(net), false) < 0) - goto cleanup; + /* Set bandwidth or fail if requested and not supported. */ + actualBandwidth = virDomainNetGetActualBandwidth(net); + if (actualBandwidth) { + if (!virNetDevSupportBandwidth(actualType)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("setting bandwidth on interfaces of " + "type '%s' is not implemented yet"), + virDomainNetTypeToString(actualType)); + goto cleanup; + } + + if (virNetDevBandwidthSet(net->ifname, actualBandwidth, false) < 0) + goto cleanup; + } if (virNetDevSetNamespace(veth, priv->initpid) < 0) { virDomainAuditNet(vm, NULL, net, "attach", false); diff --git a/src/lxc/lxc_process.c b/src/lxc/lxc_process.c index d7eb8bc..9fa900e 100644 --- a/src/lxc/lxc_process.c +++ b/src/lxc/lxc_process.c @@ -371,6 +371,7 @@ static int virLXCProcessSetupInterfaces(virConnectPtr conn, for (i = 0; i < def->nnets; i++) { char *veth = NULL; + virNetDevBandwidthPtr actualBandwidth; /* If appropriate, grab a physical device from the configured * network's pool of devices, or resolve bridge device name * to the one defined in the network definition. @@ -424,11 +425,20 @@ static int virLXCProcessSetupInterfaces(virConnectPtr conn, } - /* set network bandwidth */ - if (virNetDevSupportBandwidth(type) && - virNetDevBandwidthSet(net->ifname, - virDomainNetGetActualBandwidth(net), false) < 0) - goto cleanup; + /* Set bandwidth or fail if requested and not supported. */ + actualBandwidth = virDomainNetGetActualBandwidth(net); + if (actualBandwidth) { + if (!virNetDevSupportBandwidth(type)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("setting bandwidth on interfaces of " + "type '%s' is not implemented yet"), + virDomainNetTypeToString(type)); + goto cleanup; + } + + if (virNetDevBandwidthSet(net->ifname, actualBandwidth, false) < 0) + goto cleanup; + } (*veths)[(*nveths)-1] = veth; diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index d5679de..1729866 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -7358,6 +7358,7 @@ qemuBuildInterfaceCommandLine(virCommandPtr cmd, char **tapfdName = NULL; char **vhostfdName = NULL; int actualType = virDomainNetGetActualType(net); + virNetDevBandwidthPtr actualBandwidth; size_t i; if (actualType == VIR_DOMAIN_NET_TYPE_VHOSTUSER) @@ -7408,12 +7409,20 @@ qemuBuildInterfaceCommandLine(virCommandPtr cmd, goto cleanup; } - /* Set Bandwidth */ - if (virNetDevSupportBandwidth(actualType) && - virNetDevBandwidthSet(net->ifname, - virDomainNetGetActualBandwidth(net), - false) < 0) - goto cleanup; + /* Set bandwidth or fail if requested and not supported. */ + actualBandwidth = virDomainNetGetActualBandwidth(net); + if (actualBandwidth) { + if (!virNetDevSupportBandwidth(actualType)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("setting bandwidth on interfaces of " + "type '%s' is not implemented yet"), + virDomainNetTypeToString(actualType)); + goto cleanup; + } + + if (virNetDevBandwidthSet(net->ifname, actualBandwidth, false) < 0) + goto cleanup; + } if ((actualType == VIR_DOMAIN_NET_TYPE_NETWORK || actualType == VIR_DOMAIN_NET_TYPE_BRIDGE || diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 1714341..5ea374b 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -838,6 +838,7 @@ int qemuDomainAttachNetDevice(virConnectPtr conn, bool releaseaddr = false; bool iface_connected = false; int actualType; + virNetDevBandwidthPtr actualBandwidth; virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); size_t i; @@ -926,11 +927,20 @@ int qemuDomainAttachNetDevice(virConnectPtr conn, if (qemuInterfaceStartDevice(net) < 0) goto cleanup; - /* Set Bandwidth */ - if (virNetDevSupportBandwidth(actualType) && - virNetDevBandwidthSet(net->ifname, - virDomainNetGetActualBandwidth(net), false) < 0) - goto cleanup; + /* Set bandwidth or fail if requested and not supported. */ + actualBandwidth = virDomainNetGetActualBandwidth(net); + if (actualBandwidth) { + if (!virNetDevSupportBandwidth(actualType)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("setting bandwidth on interfaces of " + "type '%s' is not implemented yet"), + virDomainNetTypeToString(actualType)); + goto cleanup; + } + + if (virNetDevBandwidthSet(net->ifname, actualBandwidth, false) < 0) + goto cleanup; + } for (i = 0; i < tapfdSize; i++) { if (virSecurityManagerSetTapFDLabel(driver->securityManager, -- 2.0.5
On 01/07/2015 12:31 PM, Michal Privoznik wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=1165993
So, there are still plenty of vNIC types that we don't know how to set bandwidth on. Let's fail explicitly in case user has requested it instead of pretending everything
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/lxc/lxc_driver.c | 20 +++++++++++++++----- src/lxc/lxc_process.c | 20 +++++++++++++++----- src/qemu/qemu_command.c | 21 +++++++++++++++------ src/qemu/qemu_hotplug.c | 20 +++++++++++++++----- 4 files changed, 60 insertions(+), 21 deletions(-)
Guess your .0 expectations regarding heavy discussion haven't come to fruition... With these changes we'll no longer be lying about setting QoS at least at the peril of course of upsetting those that have set them inappropriately on an unsupported vNIC. These changes though would mean a domain that was at one time successfully running will no longer start without changing (removing) the QoS settings on the unsupported vNIC - I could see that upsetting the customer base more than say perhaps somehow messaging (VIR_INFO, VIR_WARNING?) that the vNIC doesn't support QoS settings and are being ignored. Is there a way to message that out to something more than the log file(s) while still continuing? In reality the settings were being ignored anyway previously so a message is at least an upgrade, although perhaps not quite an optimal solution for something which may not "see" the message if it isn't displayed back to the stdout/err. Another option... message via VIR_WARNING/INFO *and* then just clear them and save the domain status/xml so that they are not seen again... Then of course soldier on through startup. Is that a viable option based on history of dealing with such things? I'm OK with the changes as is; however, I think getting another opinion or two would be useful... John
diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c index 51c664f..10a4a7e 100644 --- a/src/lxc/lxc_driver.c +++ b/src/lxc/lxc_driver.c @@ -4139,6 +4139,7 @@ lxcDomainAttachDeviceNetLive(virConnectPtr conn, virLXCDomainObjPrivatePtr priv = vm->privateData; int ret = -1; int actualType; + virNetDevBandwidthPtr actualBandwidth; char *veth = NULL;
if (!priv->initpid) { @@ -4186,11 +4187,20 @@ lxcDomainAttachDeviceNetLive(virConnectPtr conn, _("Network device type is not supported")); goto cleanup; } - /* set network bandwidth */ - if (virNetDevSupportBandwidth(actualType) && - virNetDevBandwidthSet(net->ifname, - virDomainNetGetActualBandwidth(net), false) < 0) - goto cleanup; + /* Set bandwidth or fail if requested and not supported. */ + actualBandwidth = virDomainNetGetActualBandwidth(net); + if (actualBandwidth) { + if (!virNetDevSupportBandwidth(actualType)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("setting bandwidth on interfaces of " + "type '%s' is not implemented yet"), + virDomainNetTypeToString(actualType)); + goto cleanup; + } + + if (virNetDevBandwidthSet(net->ifname, actualBandwidth, false) < 0) + goto cleanup; + }
if (virNetDevSetNamespace(veth, priv->initpid) < 0) { virDomainAuditNet(vm, NULL, net, "attach", false); diff --git a/src/lxc/lxc_process.c b/src/lxc/lxc_process.c index d7eb8bc..9fa900e 100644 --- a/src/lxc/lxc_process.c +++ b/src/lxc/lxc_process.c @@ -371,6 +371,7 @@ static int virLXCProcessSetupInterfaces(virConnectPtr conn,
for (i = 0; i < def->nnets; i++) { char *veth = NULL; + virNetDevBandwidthPtr actualBandwidth; /* If appropriate, grab a physical device from the configured * network's pool of devices, or resolve bridge device name * to the one defined in the network definition. @@ -424,11 +425,20 @@ static int virLXCProcessSetupInterfaces(virConnectPtr conn,
}
- /* set network bandwidth */ - if (virNetDevSupportBandwidth(type) && - virNetDevBandwidthSet(net->ifname, - virDomainNetGetActualBandwidth(net), false) < 0) - goto cleanup; + /* Set bandwidth or fail if requested and not supported. */ + actualBandwidth = virDomainNetGetActualBandwidth(net); + if (actualBandwidth) { + if (!virNetDevSupportBandwidth(type)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("setting bandwidth on interfaces of " + "type '%s' is not implemented yet"), + virDomainNetTypeToString(type)); + goto cleanup; + } + + if (virNetDevBandwidthSet(net->ifname, actualBandwidth, false) < 0) + goto cleanup; + }
(*veths)[(*nveths)-1] = veth;
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index d5679de..1729866 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -7358,6 +7358,7 @@ qemuBuildInterfaceCommandLine(virCommandPtr cmd, char **tapfdName = NULL; char **vhostfdName = NULL; int actualType = virDomainNetGetActualType(net); + virNetDevBandwidthPtr actualBandwidth; size_t i;
if (actualType == VIR_DOMAIN_NET_TYPE_VHOSTUSER) @@ -7408,12 +7409,20 @@ qemuBuildInterfaceCommandLine(virCommandPtr cmd, goto cleanup; }
- /* Set Bandwidth */ - if (virNetDevSupportBandwidth(actualType) && - virNetDevBandwidthSet(net->ifname, - virDomainNetGetActualBandwidth(net), - false) < 0) - goto cleanup; + /* Set bandwidth or fail if requested and not supported. */ + actualBandwidth = virDomainNetGetActualBandwidth(net); + if (actualBandwidth) { + if (!virNetDevSupportBandwidth(actualType)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("setting bandwidth on interfaces of " + "type '%s' is not implemented yet"), + virDomainNetTypeToString(actualType)); + goto cleanup; + } + + if (virNetDevBandwidthSet(net->ifname, actualBandwidth, false) < 0) + goto cleanup; + }
if ((actualType == VIR_DOMAIN_NET_TYPE_NETWORK || actualType == VIR_DOMAIN_NET_TYPE_BRIDGE || diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 1714341..5ea374b 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -838,6 +838,7 @@ int qemuDomainAttachNetDevice(virConnectPtr conn, bool releaseaddr = false; bool iface_connected = false; int actualType; + virNetDevBandwidthPtr actualBandwidth; virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); size_t i;
@@ -926,11 +927,20 @@ int qemuDomainAttachNetDevice(virConnectPtr conn, if (qemuInterfaceStartDevice(net) < 0) goto cleanup;
- /* Set Bandwidth */ - if (virNetDevSupportBandwidth(actualType) && - virNetDevBandwidthSet(net->ifname, - virDomainNetGetActualBandwidth(net), false) < 0) - goto cleanup; + /* Set bandwidth or fail if requested and not supported. */ + actualBandwidth = virDomainNetGetActualBandwidth(net); + if (actualBandwidth) { + if (!virNetDevSupportBandwidth(actualType)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("setting bandwidth on interfaces of " + "type '%s' is not implemented yet"), + virDomainNetTypeToString(actualType)); + goto cleanup; + } + + if (virNetDevBandwidthSet(net->ifname, actualBandwidth, false) < 0) + goto cleanup; + }
for (i = 0; i < tapfdSize; i++) { if (virSecurityManagerSetTapFDLabel(driver->securityManager,
On 01/13/2015 10:58 AM, John Ferlan wrote:
On 01/07/2015 12:31 PM, Michal Privoznik wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=1165993
So, there are still plenty of vNIC types that we don't know how to set bandwidth on. Let's fail explicitly in case user has requested it instead of pretending everything
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/lxc/lxc_driver.c | 20 +++++++++++++++----- src/lxc/lxc_process.c | 20 +++++++++++++++----- src/qemu/qemu_command.c | 21 +++++++++++++++------ src/qemu/qemu_hotplug.c | 20 +++++++++++++++----- 4 files changed, 60 insertions(+), 21 deletions(-)
Guess your .0 expectations regarding heavy discussion haven't come to fruition...
Partly because it got lost in the post vacation spew :-) I do understand Michal's concern. In particular, the network hook script support was put in specifically to allow management apps to support bandwidth management on interface types that libvirt couldn't/didn't directly support. If I recall correctly, the idea is that, although libvirt does nothing with it, the bandwidth info will be in the XML that's passed to the hook script when the interface is brought up, and the hook script can do whatever it likes to setup the bandwidth. Or am I remembering incorrectly. If that functionality is important, then this patch can't go in. If not, then it's a reasonable thing to do.
With these changes we'll no longer be lying about setting QoS at least at the peril of course of upsetting those that have set them inappropriately on an unsupported vNIC.
These changes though would mean a domain that was at one time successfully running will no longer start without changing (removing) the QoS settings on the unsupported vNIC - I could see that upsetting the customer base more than say perhaps somehow messaging (VIR_INFO, VIR_WARNING?) that the vNIC doesn't support QoS settings and are being ignored. Is there a way to message that out to something more than the log file(s) while still continuing?
This may be a reasonable middle ground. On the other hand, there are *many* other instances of unsupported config where we log an error when we could have instead logged a warning and assumed that the user may have a hook script that adds in the support; it would be nice to either be consistent about this one way or the other, or at least have a good story about why we do it differently in this case.
In reality the settings were being ignored anyway previously so a message is at least an upgrade, although perhaps not quite an optimal solution for something which may not "see" the message if it isn't displayed back to the stdout/err.
Another option... message via VIR_WARNING/INFO *and* then just clear them and save the domain status/xml so that they are not seen again... Then of course soldier on through startup. Is that a viable option based on history of dealing with such things?
I don't like that option - rather than immediately telling you that you're doing something wrong and forcing you to fix it, it would semi-silently remove your hard work.
I'm OK with the changes as is; however, I think getting another opinion or two would be useful...
John
diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c index 51c664f..10a4a7e 100644 --- a/src/lxc/lxc_driver.c +++ b/src/lxc/lxc_driver.c @@ -4139,6 +4139,7 @@ lxcDomainAttachDeviceNetLive(virConnectPtr conn, virLXCDomainObjPrivatePtr priv = vm->privateData; int ret = -1; int actualType; + virNetDevBandwidthPtr actualBandwidth; char *veth = NULL;
if (!priv->initpid) { @@ -4186,11 +4187,20 @@ lxcDomainAttachDeviceNetLive(virConnectPtr conn, _("Network device type is not supported")); goto cleanup; } - /* set network bandwidth */ - if (virNetDevSupportBandwidth(actualType) && - virNetDevBandwidthSet(net->ifname, - virDomainNetGetActualBandwidth(net), false) < 0) - goto cleanup; + /* Set bandwidth or fail if requested and not supported. */ + actualBandwidth = virDomainNetGetActualBandwidth(net); + if (actualBandwidth) { + if (!virNetDevSupportBandwidth(actualType)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("setting bandwidth on interfaces of " + "type '%s' is not implemented yet"), + virDomainNetTypeToString(actualType)); + goto cleanup; + } + + if (virNetDevBandwidthSet(net->ifname, actualBandwidth, false) < 0) + goto cleanup; + }
if (virNetDevSetNamespace(veth, priv->initpid) < 0) { virDomainAuditNet(vm, NULL, net, "attach", false); diff --git a/src/lxc/lxc_process.c b/src/lxc/lxc_process.c index d7eb8bc..9fa900e 100644 --- a/src/lxc/lxc_process.c +++ b/src/lxc/lxc_process.c @@ -371,6 +371,7 @@ static int virLXCProcessSetupInterfaces(virConnectPtr conn,
for (i = 0; i < def->nnets; i++) { char *veth = NULL; + virNetDevBandwidthPtr actualBandwidth; /* If appropriate, grab a physical device from the configured * network's pool of devices, or resolve bridge device name * to the one defined in the network definition. @@ -424,11 +425,20 @@ static int virLXCProcessSetupInterfaces(virConnectPtr conn,
}
- /* set network bandwidth */ - if (virNetDevSupportBandwidth(type) && - virNetDevBandwidthSet(net->ifname, - virDomainNetGetActualBandwidth(net), false) < 0) - goto cleanup; + /* Set bandwidth or fail if requested and not supported. */ + actualBandwidth = virDomainNetGetActualBandwidth(net); + if (actualBandwidth) { + if (!virNetDevSupportBandwidth(type)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("setting bandwidth on interfaces of " + "type '%s' is not implemented yet"), + virDomainNetTypeToString(type)); + goto cleanup; + } + + if (virNetDevBandwidthSet(net->ifname, actualBandwidth, false) < 0) + goto cleanup; + }
(*veths)[(*nveths)-1] = veth;
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index d5679de..1729866 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -7358,6 +7358,7 @@ qemuBuildInterfaceCommandLine(virCommandPtr cmd, char **tapfdName = NULL; char **vhostfdName = NULL; int actualType = virDomainNetGetActualType(net); + virNetDevBandwidthPtr actualBandwidth; size_t i;
if (actualType == VIR_DOMAIN_NET_TYPE_VHOSTUSER) @@ -7408,12 +7409,20 @@ qemuBuildInterfaceCommandLine(virCommandPtr cmd, goto cleanup; }
- /* Set Bandwidth */ - if (virNetDevSupportBandwidth(actualType) && - virNetDevBandwidthSet(net->ifname, - virDomainNetGetActualBandwidth(net), - false) < 0) - goto cleanup; + /* Set bandwidth or fail if requested and not supported. */ + actualBandwidth = virDomainNetGetActualBandwidth(net); + if (actualBandwidth) { + if (!virNetDevSupportBandwidth(actualType)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("setting bandwidth on interfaces of " + "type '%s' is not implemented yet"), + virDomainNetTypeToString(actualType)); + goto cleanup; + } + + if (virNetDevBandwidthSet(net->ifname, actualBandwidth, false) < 0) + goto cleanup; + }
if ((actualType == VIR_DOMAIN_NET_TYPE_NETWORK || actualType == VIR_DOMAIN_NET_TYPE_BRIDGE || diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 1714341..5ea374b 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -838,6 +838,7 @@ int qemuDomainAttachNetDevice(virConnectPtr conn, bool releaseaddr = false; bool iface_connected = false; int actualType; + virNetDevBandwidthPtr actualBandwidth; virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); size_t i;
@@ -926,11 +927,20 @@ int qemuDomainAttachNetDevice(virConnectPtr conn, if (qemuInterfaceStartDevice(net) < 0) goto cleanup;
- /* Set Bandwidth */ - if (virNetDevSupportBandwidth(actualType) && - virNetDevBandwidthSet(net->ifname, - virDomainNetGetActualBandwidth(net), false) < 0) - goto cleanup; + /* Set bandwidth or fail if requested and not supported. */ + actualBandwidth = virDomainNetGetActualBandwidth(net); + if (actualBandwidth) { + if (!virNetDevSupportBandwidth(actualType)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("setting bandwidth on interfaces of " + "type '%s' is not implemented yet"), + virDomainNetTypeToString(actualType)); + goto cleanup; + } + + if (virNetDevBandwidthSet(net->ifname, actualBandwidth, false) < 0) + goto cleanup; + }
for (i = 0; i < tapfdSize; i++) { if (virSecurityManagerSetTapFDLabel(driver->securityManager,
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On 01/13/2015 09:26 AM, Laine Stump wrote:
I do understand Michal's concern. In particular, the network hook script support was put in specifically to allow management apps to support bandwidth management on interface types that libvirt couldn't/didn't directly support. If I recall correctly, the idea is that, although libvirt does nothing with it, the bandwidth info will be in the XML that's passed to the hook script when the interface is brought up, and the hook script can do whatever it likes to setup the bandwidth.
Or am I remembering incorrectly.
If that functionality is important, then this patch can't go in. If not, then it's a reasonable thing to do.
In the past, we've had other places where users were actively relying on unsupported XML being preserved; when we tightened the schema, a complaint was raised that we broke their setups, but we countered that their setup was relying on undefined behavior and that we explicitly have the namespace-aware annotations that they can use instead to guarantee that XML will be preserved and untouched by libvirt. I'm okay with stating that any user relying on this has been relying on unsupported behavior, but agree that it is nicer to warn the user rather than just silently break things. Maybe Dan has better advice? -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 13.01.2015 17:57, Eric Blake wrote:
On 01/13/2015 09:26 AM, Laine Stump wrote:
I do understand Michal's concern. In particular, the network hook script support was put in specifically to allow management apps to support bandwidth management on interface types that libvirt couldn't/didn't directly support. If I recall correctly, the idea is that, although libvirt does nothing with it, the bandwidth info will be in the XML that's passed to the hook script when the interface is brought up, and the hook script can do whatever it likes to setup the bandwidth.
Or am I remembering incorrectly.
If that functionality is important, then this patch can't go in. If not, then it's a reasonable thing to do.
In the past, we've had other places where users were actively relying on unsupported XML being preserved; when we tightened the schema, a complaint was raised that we broke their setups, but we countered that their setup was relying on undefined behavior and that we explicitly have the namespace-aware annotations that they can use instead to guarantee that XML will be preserved and untouched by libvirt. I'm okay with stating that any user relying on this has been relying on unsupported behavior, but agree that it is nicer to warn the user rather than just silently break things. Maybe Dan has better advice?
Fair enough, let me respin with VIR_WARN (or is VIR_INFO more appropriate?). I've pushed the 1/2 btw since it's a code cleanup practically unrelated to 2/2. Michal
participants (4)
-
Eric Blake -
John Ferlan -
Laine Stump -
Michal Privoznik