There is no guarantee that an enum start it mapped onto a value of zero. However, we are guaranteed that enum items are consecutive integers. Moreover, it's a pity to define an enum to avoid using magical constants but then using them anyway. Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; --- src/util/virnetdev.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c index 1e20789..13d0f23 100644 --- a/src/util/virnetdev.c +++ b/src/util/virnetdev.c @@ -2708,7 +2708,7 @@ static int virNetDevParseMcast(char *buf, virNetDevMcastEntryPtr mcast) char *saveptr; char *endptr; - for (ifindex = 0, next = buf; ifindex < VIR_MCAST_TYPE_LAST; ifindex++, + for (ifindex = VIR_MCAST_TYPE_INDEX_TOKEN, next = buf; ifindex < VIR_MCAST_TYPE_LAST; ifindex++, next = NULL) { token = strtok_r(next, VIR_MCAST_TOKEN_DELIMS, &saveptr); -- 2.4.6

This is no functional change. It's just that later in the series we will need to pass class_id as an integer. Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; --- src/network/bridge_driver.c | 4 ++-- src/util/virnetdevbandwidth.c | 10 +++++++--- src/util/virnetdevbandwidth.h | 2 +- 3 files changed, 10 insertions(+), 6 deletions(-) diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index 3d6721b..17fc430 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -4840,7 +4840,7 @@ networkPlugBandwidth(virNetworkObjPtr net, } /* update rate for non guaranteed NICs */ new_rate -= net->floor_sum; - if (virNetDevBandwidthUpdateRate(net->def->bridge, "1:2", + if (virNetDevBandwidthUpdateRate(net->def->bridge, 2, net->def->bandwidth, new_rate) < 0) VIR_WARN("Unable to update rate for 1:2 class on %s bridge", net->def->bridge); @@ -4891,7 +4891,7 @@ networkUnplugBandwidth(virNetworkObjPtr net, } /* update rate for non guaranteed NICs */ new_rate -= net->floor_sum; - if (virNetDevBandwidthUpdateRate(net->def->bridge, "1:2", + if (virNetDevBandwidthUpdateRate(net->def->bridge, 2, net->def->bandwidth, new_rate) < 0) VIR_WARN("Unable to update rate for 1:2 class on %s bridge", net->def->bridge); diff --git a/src/util/virnetdevbandwidth.c b/src/util/virnetdevbandwidth.c index 6ae0877..91201ae 100644 --- a/src/util/virnetdevbandwidth.c +++ b/src/util/virnetdevbandwidth.c @@ -638,7 +638,8 @@ virNetDevBandwidthUnplug(const char *brname, /** * virNetDevBandwidthUpdateRate: * @ifname: interface name - * @classid: ID of class to update + * @id: unique identifier (MUST be greater than 2) + * @bandwidth: used to derive 'ceil' of class with @id * @new_rate: new rate * * This function updates the 'rate' attribute of HTB class. @@ -650,16 +651,18 @@ virNetDevBandwidthUnplug(const char *brname, */ int virNetDevBandwidthUpdateRate(const char *ifname, - const char *class_id, + unsigned int id, virNetDevBandwidthPtr bandwidth, unsigned long long new_rate) { int ret = -1; virCommandPtr cmd = NULL; + char *class_id = NULL; char *rate = NULL; char *ceil = NULL; - if (virAsprintf(&rate, "%llukbps", new_rate) < 0 || + if (virAsprintf(&class_id, "1:%x", id) < 0 || + virAsprintf(&rate, "%llukbps", new_rate) < 0 || virAsprintf(&ceil, "%llukbps", bandwidth->in->peak ? bandwidth->in->peak : bandwidth->in->average) < 0) @@ -677,6 +680,7 @@ virNetDevBandwidthUpdateRate(const char *ifname, cleanup: virCommandFree(cmd); + VIR_FREE(class_id); VIR_FREE(rate); VIR_FREE(ceil); return ret; diff --git a/src/util/virnetdevbandwidth.h b/src/util/virnetdevbandwidth.h index 9b1d2a6..f42094c 100644 --- a/src/util/virnetdevbandwidth.h +++ b/src/util/virnetdevbandwidth.h @@ -68,7 +68,7 @@ int virNetDevBandwidthUnplug(const char *brname, ATTRIBUTE_NONNULL(1) ATTRIBUTE_RETURN_CHECK; int virNetDevBandwidthUpdateRate(const char *ifname, - const char *class_id, + unsigned int id, virNetDevBandwidthPtr bandwidth, unsigned long long new_rate) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) -- 2.4.6

On 08/03/2015 02:39 AM, Michal Privoznik wrote: something that dawned on me reading later patches... ^^^ Comment says MUST be > 2, but there's no check/error... Also I note "2" is passed often... John

When a domain vNIC's bandwidth is to be changed (at runtime) it is possible that guaranteed minimal bandwidth (@floor) will change too. Well, so far it is, because we still don't have an implementation that allows setting it dynamically, so it's effectively erased on: #virsh domiftune $dom vnet0 --inbound 0 However, that's slightly unfortunate. We do some checks on domain startup to see if @floor can be guaranteed. We ought do the same if QoS is changed at runtime. Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; --- src/network/bridge_driver.c | 72 +++++++++++++++++++++++++++++++++++++++++++-- src/network/bridge_driver.h | 12 ++++++++ 2 files changed, 82 insertions(+), 2 deletions(-) diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index 17fc430..fa60ba4 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -4686,9 +4686,18 @@ networkGetNetworkAddress(const char *netname, char **netaddr) * networkCheckBandwidth: * @net: network QoS * @ifaceBand: interface QoS (may be NULL if no QoS) + * @oldBandwidth: new interface QoS (may be NULL if no QoS) * @ifaceMac: interface MAC (used in error messages for identification) * @new_rate: new rate for non guaranteed class * + * Function checks if @ifaceBand can be satisfied on @net. However, sometimes it + * may happen that the interface that @ifaceBand corresponds to is already + * plugged into the @net and the bandwidth is to be updated. In that case we + * need to check if new bandwidth can be satisfied. If that's the case + * @ifaceBand should point to new bandwidth settings and @oldBandwidth to + * current ones. If you want to suppress this functionality just pass + * @oldBandwidth == NULL. + * * Returns: -1 if plugging would overcommit network QoS * 0 if plugging is safe (@new_rate updated) * 1 if no QoS is set (@new_rate untouched) @@ -4696,6 +4705,7 @@ networkGetNetworkAddress(const char *netname, char **netaddr) static int networkCheckBandwidth(virNetworkObjPtr net, virNetDevBandwidthPtr ifaceBand, + virNetDevBandwidthPtr oldBandwidth, virMacAddr ifaceMac, unsigned long long *new_rate) { @@ -4723,6 +4733,8 @@ networkCheckBandwidth(virNetworkObjPtr net, } tmp_new_rate = netBand->in->average; + if (oldBandwidth && oldBandwidth->in) + tmp_floor_sum -= oldBandwidth->in->floor; tmp_floor_sum += ifaceBand->in->floor; /* check against peak */ @@ -4749,7 +4761,8 @@ networkCheckBandwidth(virNetworkObjPtr net, goto cleanup; } - *new_rate = tmp_new_rate; + if (new_rate) + *new_rate = tmp_new_rate; ret = 0; cleanup: @@ -4791,7 +4804,7 @@ networkPlugBandwidth(virNetworkObjPtr net, char ifmac[VIR_MAC_STRING_BUFLEN]; virNetDevBandwidthPtr ifaceBand = virDomainNetGetActualBandwidth(iface); - if ((plug_ret = networkCheckBandwidth(net, ifaceBand, + if ((plug_ret = networkCheckBandwidth(net, ifaceBand, NULL, iface->mac, &new_rate)) < 0) { /* helper reported error */ goto cleanup; @@ -4917,3 +4930,58 @@ networkNetworkObjTaint(virNetworkObjPtr net, virNetworkTaintTypeToString(taint)); } } + + +static bool +networkBandwidthGenericChecks(virDomainNetDefPtr iface, + virNetDevBandwidthPtr newBandwidth) +{ + virNetDevBandwidthPtr ifaceBand = virDomainNetGetActualBandwidth(iface); + unsigned long long old_floor, new_floor; + + if (virDomainNetGetActualType(iface) != VIR_DOMAIN_NET_TYPE_NETWORK) { + /* This is not an interface that's plugged into a network. + * We don't care. Thus from our POV bandwidth change is allowed. */ + return false; + } + + old_floor = new_floor = 0; + + if (ifaceBand && ifaceBand->in) + old_floor = ifaceBand->in->floor; + if (newBandwidth && newBandwidth->in) + new_floor = newBandwidth->in->floor; + + return new_floor != old_floor; +} + + +bool +networkBandwidthChangeAllowed(virDomainNetDefPtr iface, + virNetDevBandwidthPtr newBandwidth) +{ + virNetworkDriverStatePtr driver = networkGetDriver(); + virNetworkObjPtr network = NULL; + virNetDevBandwidthPtr ifaceBand = virDomainNetGetActualBandwidth(iface); + bool ret = false; + + if (!networkBandwidthGenericChecks(iface, newBandwidth)) + return true; + + network = virNetworkObjFindByName(driver->networks, iface->data.network.name); + if (!network) { + virReportError(VIR_ERR_NO_NETWORK, + _("no network with matching name '%s'"), + iface->data.network.name); + return false; + } + + if (networkCheckBandwidth(network, newBandwidth, ifaceBand, iface->mac, NULL) < 0) + goto cleanup; + + ret = true; + + cleanup: + virNetworkObjEndAPI(&network); + return ret; +} diff --git a/src/network/bridge_driver.h b/src/network/bridge_driver.h index 513ccf7..cce9237 100644 --- a/src/network/bridge_driver.h +++ b/src/network/bridge_driver.h @@ -52,6 +52,11 @@ int networkDnsmasqConfContents(virNetworkObjPtr network, char **configstr, dnsmasqContext *dctx, dnsmasqCapsPtr caps); + +bool networkBandwidthChangeAllowed(virDomainNetDefPtr iface, + virNetDevBandwidthPtr newBandwidth) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2); + # else /* Define no-op replacements that don't drag in any link dependencies. */ # define networkAllocateActualDevice(dom, iface) 0 @@ -73,6 +78,13 @@ networkReleaseActualDevice(virDomainDefPtr dom ATTRIBUTE_UNUSED, return 0; } +static inline bool +networkBandwidthChangeAllowed(virDomainNetDefPtr iface ATTRIBUTE_UNUSED, + virNetDevBandwidthPtr newBandwidth ATTRIBUTE_UNUSED) +{ + return true; +} + # endif typedef char *(*networkDnsmasqLeaseFileNameFunc)(const char *netname); -- 2.4.6

So, if a domain vNIC's bandwidth has been successfully set, it's possible that because @floor is set on network's bridge, this part may need updating too. And that's exactly what this function does. While the previous commit introduced a function to check if @floor can be satisfied, this does all the hard work. In general, there may be three, well four possibilities: 1) No change in @floor value (either it remain unset, or its value hasn't changed) 2) The @floor value has changed from a non-zero to a non-zero value 3) New @floor is to be set 4) Old @floor must be cleared out The difference between 2), 3) and 4) is, that while in 2) the QoS tree on the network's bridge already has a special class for the vNIC, in 3) the class must be created from scratch. In 4) it must be removed. Fortunately, we have helpers for all three interesting cases. Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; --- src/network/bridge_driver.c | 160 ++++++++++++++++++++++++++++++++++++-------- src/network/bridge_driver.h | 10 +++ 2 files changed, 142 insertions(+), 28 deletions(-) diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index fa60ba4..5fad015 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -4792,38 +4792,17 @@ networkNextClassID(virNetworkObjPtr net) return ret; } + static int -networkPlugBandwidth(virNetworkObjPtr net, - virDomainNetDefPtr iface) +networkPlugBandwidthImpl(virNetworkObjPtr net, + virDomainNetDefPtr iface, + virNetDevBandwidthPtr ifaceBand, + unsigned long long new_rate) { virNetworkDriverStatePtr driver = networkGetDriver(); - int ret = -1; - int plug_ret; - unsigned long long new_rate = 0; ssize_t class_id = 0; - char ifmac[VIR_MAC_STRING_BUFLEN]; - virNetDevBandwidthPtr ifaceBand = virDomainNetGetActualBandwidth(iface); - - if ((plug_ret = networkCheckBandwidth(net, ifaceBand, NULL, - iface->mac, &new_rate)) < 0) { - /* helper reported error */ - goto cleanup; - } - - if (plug_ret > 0) { - /* no QoS needs to be set; claim success */ - ret = 0; - goto cleanup; - } - - virMacAddrFormat(&iface->mac, ifmac); - if (iface->type != VIR_DOMAIN_NET_TYPE_NETWORK || - !iface->data.network.actual) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("Cannot set bandwidth on interface '%s' of type %d"), - ifmac, iface->type); - goto cleanup; - } + int plug_ret; + int ret = -1; /* generate new class_id */ if ((class_id = networkNextClassID(net)) < 0) { @@ -4859,6 +4838,46 @@ networkPlugBandwidth(virNetworkObjPtr net, net->def->bridge); ret = 0; + cleanup: + return ret; +} + + +static int +networkPlugBandwidth(virNetworkObjPtr net, + virDomainNetDefPtr iface) +{ + int ret = -1; + int plug_ret; + unsigned long long new_rate = 0; + char ifmac[VIR_MAC_STRING_BUFLEN]; + virNetDevBandwidthPtr ifaceBand = virDomainNetGetActualBandwidth(iface); + + if ((plug_ret = networkCheckBandwidth(net, ifaceBand, NULL, + iface->mac, &new_rate)) < 0) { + /* helper reported error */ + goto cleanup; + } + + if (plug_ret > 0) { + /* no QoS needs to be set; claim success */ + ret = 0; + goto cleanup; + } + + virMacAddrFormat(&iface->mac, ifmac); + if (iface->type != VIR_DOMAIN_NET_TYPE_NETWORK || + !iface->data.network.actual) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Cannot set bandwidth on interface '%s' of type %d"), + ifmac, iface->type); + goto cleanup; + } + + if (networkPlugBandwidthImpl(net, iface, ifaceBand, new_rate) < 0) + goto cleanup; + + ret = 0; cleanup: return ret; @@ -4939,6 +4958,9 @@ networkBandwidthGenericChecks(virDomainNetDefPtr iface, virNetDevBandwidthPtr ifaceBand = virDomainNetGetActualBandwidth(iface); unsigned long long old_floor, new_floor; + if (!iface && !newBandwidth) + return false; + if (virDomainNetGetActualType(iface) != VIR_DOMAIN_NET_TYPE_NETWORK) { /* This is not an interface that's plugged into a network. * We don't care. Thus from our POV bandwidth change is allowed. */ @@ -4985,3 +5007,85 @@ networkBandwidthChangeAllowed(virDomainNetDefPtr iface, virNetworkObjEndAPI(&network); return ret; } + + +int +networkBandwidthUpdate(virDomainNetDefPtr iface, + virNetDevBandwidthPtr newBandwidth) +{ + virNetworkDriverStatePtr driver = networkGetDriver(); + virNetworkObjPtr network = NULL; + virNetDevBandwidthPtr ifaceBand = virDomainNetGetActualBandwidth(iface); + unsigned long long new_rate = 0; + int plug_ret; + int ret = -1; + + if (!networkBandwidthGenericChecks(iface, newBandwidth)) + return 0; + + network = virNetworkObjFindByName(driver->networks, iface->data.network.name); + if (!network) { + virReportError(VIR_ERR_NO_NETWORK, + _("no network with matching name '%s'"), + iface->data.network.name); + return ret; + } + + if ((plug_ret = networkCheckBandwidth(network, ifaceBand, NULL, + iface->mac, &new_rate)) < 0) { + /* helper reported error */ + goto cleanup; + } + + if (plug_ret > 0) { + /* no QoS needs to be set; claim success */ + ret = 0; + goto cleanup; + } + + /* Okay, there are three possible scenarios: */ + + if (ifaceBand->in && ifaceBand->in->floor && + newBandwidth->in && newBandwidth->in->floor) { + /* Either we just need to update @floor .. */ + + if (virNetDevBandwidthUpdateRate(network->def->bridge, + iface->data.network.actual->class_id, + network->def->bandwidth, + newBandwidth->in->floor) < 0) + goto cleanup; + + network->floor_sum -= ifaceBand->in->floor; + network->floor_sum += newBandwidth->in->floor; + new_rate -= network->floor_sum; + + if (virNetDevBandwidthUpdateRate(network->def->bridge, 2, + network->def->bandwidth, new_rate) < 0 || + virNetworkSaveStatus(driver->stateDir, network) < 0) { + /* Ouch, rollback */ + network->floor_sum -= newBandwidth->in->floor; + network->floor_sum += ifaceBand->in->floor; + + ignore_value(virNetDevBandwidthUpdateRate(network->def->bridge, + iface->data.network.actual->class_id, + network->def->bandwidth, + ifaceBand->in->floor)); + goto cleanup; + } + } else if (newBandwidth->in && newBandwidth->in->floor) { + /* .. or we need to plug in new .. */ + + if (networkPlugBandwidthImpl(network, iface, newBandwidth, new_rate) < 0) + goto cleanup; + } else { + /* .. or unplug old. */ + + if (networkUnplugBandwidth(network, iface) < 0) + goto cleanup; + } + + ret = 0; + cleanup: + virNetworkObjEndAPI(&network); + return ret; +} diff --git a/src/network/bridge_driver.h b/src/network/bridge_driver.h index cce9237..3a9f22d 100644 --- a/src/network/bridge_driver.h +++ b/src/network/bridge_driver.h @@ -57,6 +57,9 @@ bool networkBandwidthChangeAllowed(virDomainNetDefPtr iface, virNetDevBandwidthPtr newBandwidth) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2); +int networkBandwidthUpdate(virDomainNetDefPtr iface, + virNetDevBandwidthPtr newBandwidth); + # else /* Define no-op replacements that don't drag in any link dependencies. */ # define networkAllocateActualDevice(dom, iface) 0 @@ -85,6 +88,13 @@ networkBandwidthChangeAllowed(virDomainNetDefPtr iface ATTRIBUTE_UNUSED, return true; } +static inline int +networkBandwidthUpdate(virDomainNetDefPtr iface ATTRIBUTE_UNUSED, + virNetDevBandwidthPtr newBandwidth ATTRIBUTE_UNUSED) +{ + return 0; +} + # endif typedef char *(*networkDnsmasqLeaseFileNameFunc)(const char *netname); -- 2.4.6

As sketched in previous commits, imagine the following scenario: virsh # domiftune gentoo vnet0 inbound.average: 100 inbound.peak : 0 inbound.burst : 0 outbound.average: 100 outbound.peak : 0 outbound.burst : 0 virsh # domiftune gentoo vnet0 --inbound 0 virsh # shutdown gentoo Domain gentoo is being shutdown virsh # list --all error: Failed to list domains error: Cannot recv data: Connection reset by peer Program received signal SIGSEGV, Segmentation fault. 0x00007fffe80ea221 in networkUnplugBandwidth (net=0x7fff9400c1a0, iface=0x7fff940ea3e0) at network/bridge_driver.c:4881 4881 net->floor_sum -= ifaceBand->in->floor; This is rather unfortunate. We should not SIGSEGV here. The problem is, that while in the second step the inbound QoS was cleared out, the network part of it was not updated (moreover, we don't report that vnet0 had inbound.floor set). Internal structure therefore still had some fragments left (e.g. class_id). So when qemuProcessStop() started to clean up the environment it got to networkUnplugBandwidth(). Here, class_id is set therefore function assumes that there is an inbound QoS. This actually is a fair assumption to make, there's no need for a special QoS box in network's QoS when there's no QoS to set. Anyway, the problem is not the networkUnplugBandwidth() rather than qemuDomainSetInterfaceParameters() which completely forgot about QoS being disperse (some parts are set directly on interface itself, some on bridge the interface is plugged into). Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; --- src/qemu/qemu_driver.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index b9278f8..171b58f 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -100,6 +100,7 @@ #include "vircgroup.h" #include "virnuma.h" #include "dirname.h" +#include "network/bridge_driver.h" #define VIR_FROM_THIS VIR_FROM_QEMU @@ -11221,7 +11222,11 @@ qemuDomainSetInterfaceParameters(virDomainPtr dom, sizeof(*newBandwidth->out)); } - if (virNetDevBandwidthSet(net->ifname, newBandwidth, false) < 0) { + if (!networkBandwidthChangeAllowed(net, newBandwidth)) + goto endjob; + + if (virNetDevBandwidthSet(net->ifname, newBandwidth, false) < 0 || + networkBandwidthUpdate(net, newBandwidth) < 0) { ignore_value(virNetDevBandwidthSet(net->ifname, net->bandwidth, false)); -- 2.4.6

On 11.08.2015 03:06, John Ferlan wrote: Yes and no. You are right that we should check if new bandwidth can be fulfilled prior to setting it. And now, with this patch pushed we are doing that. I admit that it's hard to see since the checks are on a different places - for instance, in networkAllocateActualDevice() which calls networkPlugBandwidth() eventually and that one finally calls networkCheckBandwidth(). Maybe one day I'll pull up my sleeves and refactor the code (to actually follow the advice I am going to give in my talk on KVM Forum). Michal

On 08/03/2015 02:39 AM, Michal Privoznik wrote: My compiler complained about uninitialized value here especially when used with strtok_r (setting = NULL pacifies compiler). ACK with the adjustment. John

This macro represents the single missing field we don't expose yet within QoS: inbound.floor. Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; --- include/libvirt/libvirt-domain.h | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/include/libvirt/libvirt-domain.h b/include/libvirt/libvirt-domain.h index e8202cf..e8ea7b4 100644 --- a/include/libvirt/libvirt-domain.h +++ b/include/libvirt/libvirt-domain.h @@ -1335,6 +1335,13 @@ int virDomainInterfaceStats (virDomainPtr dom, # define VIR_DOMAIN_BANDWIDTH_IN_BURST "inbound.burst" /** + * VIR_DOMAIN_BANDWIDTH_IN_FLOOR: + * + * Macro represents the inbound floor of NIC bandwidth, as a uint. + */ +# define VIR_DOMAIN_BANDWIDTH_IN_FLOOR "inbound.floor" + +/** * VIR_DOMAIN_BANDWIDTH_OUT_AVERAGE: * * Macro represents the outbound average of NIC bandwidth, as a uint. -- 2.4.6

We have a function parseRateStr() that parses --inbound and --outbound arguments to both attach-interface and domiftune. Now that we have all virTypedParams macros needed for QoS, lets parse even floor attribute. The extended format for the arguments looks like this then: --inbound average[,peak[,burst[,floor]]] Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; --- tools/virsh-domain.c | 23 ++++++++++++++++++----- tools/virsh.pod | 12 ++++++------ 2 files changed, 24 insertions(+), 11 deletions(-) diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index bb40ddd..5b7e623 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -873,7 +873,7 @@ static int parseRateStr(vshControl *ctl, char *next; char *saveptr; enum { - AVERAGE, PEAK, BURST + AVERAGE, PEAK, BURST, FLOOR } state; int ret = -1; @@ -882,7 +882,7 @@ static int parseRateStr(vshControl *ctl, next = vshStrdup(ctl, rateStr); - for (state = AVERAGE; state <= BURST; state++) { + for (state = AVERAGE; state <= FLOOR; state++) { unsigned long long *tmp; const char *field_name; @@ -905,6 +905,11 @@ static int parseRateStr(vshControl *ctl, tmp = &rate->burst; field_name = "burst"; break; + + case FLOOR: + tmp = &rate->floor; + field_name = "floor"; + break; } if (virStrToLong_ullp(token, NULL, 10, tmp) < 0) { @@ -976,7 +981,7 @@ cmdAttachInterface(vshControl *ctl, const vshCmd *cmd) memset(&inbound, 0, sizeof(inbound)); if (parseRateStr(ctl, inboundStr, &inbound) < 0) goto cleanup; - if (inbound.average == 0) { + if (!inbound.average && !inbound.floor) { vshError(ctl, _("inbound average is mandatory")); goto cleanup; } @@ -3308,8 +3313,10 @@ cmdDomIftune(vshControl *ctl, const vshCmd *cmd) UINT_MAX); goto cleanup; } - if (inbound.average == 0 && (inbound.burst || inbound.peak)) { - vshError(ctl, _("inbound average is mandatory")); + + if ((!inbound.average && (inbound.burst || inbound.peak)) && + !inbound.floor) { + vshError(ctl, _("either inbound average or floor is mandatory")); goto cleanup; } @@ -3329,6 +3336,12 @@ cmdDomIftune(vshControl *ctl, const vshCmd *cmd) VIR_DOMAIN_BANDWIDTH_IN_BURST, inbound.burst) < 0) goto save_error; + + if (inbound.floor && + virTypedParamsAddUInt(&params, &nparams, &maxparams, + VIR_DOMAIN_BANDWIDTH_IN_FLOOR, + inbound.floor) < 0) + goto save_error; } if (outboundStr) { diff --git a/tools/virsh.pod b/tools/virsh.pod index 5ee9a96..a6148d3 100644 --- a/tools/virsh.pod +++ b/tools/virsh.pod @@ -770,7 +770,7 @@ I<interface-device> can be the interface's target name or the MAC address. =item B<domiftune> I<domain> I<interface-device> [[I<--config>] [I<--live>] | [I<--current>]] -[I<--inbound average,peak,burst>] +[I<--inbound average,peak,burst,floor>] [I<--outbound average,peak,burst>] Set or query the domain's network interface's bandwidth parameters. @@ -779,9 +779,9 @@ or the MAC address. If no I<--inbound> or I<--outbound> is specified, this command will query and show the bandwidth settings. Otherwise, it will set the -inbound or outbound bandwidth. I<average,peak,burst> is the same as -in command I<attach-interface>. Values for I<average> and I<peak> are -expressed in kilobytes per second, while I<burst> is expressed in kilobytes +inbound or outbound bandwidth. I<average,peak,burst,floor> is the same as +in command I<attach-interface>. Values for I<average>, I<peak> and I<floor> +are expressed in kilobytes per second, while I<burst> is expressed in kilobytes in a single burst at -I<peak> speed as described in the Network XML documentation at L<http://libvirt.org/formatnetwork.html#elementQoS>;. @@ -2499,7 +2499,7 @@ Likewise, I<--shareable> is an alias for I<--mode shareable>. =item B<attach-interface> I<domain> I<type> I<source> [[[I<--live>] [I<--config>] | [I<--current>]] | [I<--persistent>]] [I<--target target>] [I<--mac mac>] [I<--script script>] [I<--model model>] -[I<--inbound average,peak,burst>] [I<--outbound average,peak,burst>] +[I<--inbound average,peak,burst,floor>] [I<--outbound average,peak,burst>] Attach a new network interface to the domain. I<type> can be I<network> to indicate connection via a libvirt virtual network, or @@ -2520,7 +2520,7 @@ instead of the default script not in addition to it; --script is valid only for interfaces of type I<bridge> and only for Xen domains. I<model> specifies the network device model to be presented to the domain. I<inbound> and I<outbound> control the bandwidth of the -interface. I<peak> and I<burst> are optional, so "average,peak", +interface. I<peak>, I<burst> and I<floor> are optional, so "average,peak", "average,,burst" and "average" are also legal. Values for I<average> and I<peak> are expressed in kilobytes per second, while I<burst> is expressed in kilobytes in a single burst at -I<peak> speed as -- 2.4.6

On 11.08.2015 03:25, John Ferlan wrote: Yeah, I should have updated the error message too. At least one of average and floor is required. Your example is perfectly valid and in fact I've just just that during my testing when writing the patches. We can, but so far the outbound.floor is just ignored. diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index a1913a9..a957836 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -982,7 +982,7 @@ cmdAttachInterface(vshControl *ctl, const vshCmd *cmd) if (parseRateStr(ctl, inboundStr, &inbound) < 0) goto cleanup; if (!inbound.average && !inbound.floor) { - vshError(ctl, _("inbound average is mandatory")); + vshError(ctl, _("either inbound average or floor is mandatory")); goto cleanup; } } @@ -994,6 +994,10 @@ cmdAttachInterface(vshControl *ctl, const vshCmd *cmd) vshError(ctl, _("outbound average is mandatory")); goto cleanup; } + if (outbound.floor) { + vshError(ctl, _("outbound floor is unsupported yet")); + goto cleanup; + } } /* Make XML of interface */ @@ -3358,6 +3362,11 @@ cmdDomIftune(vshControl *ctl, const vshCmd *cmd) goto cleanup; } + if (outbound.floor) { + vshError(ctl, _("outbound floor is unsupported yet")); + goto cleanup; + } + if (virTypedParamsAddUInt(&params, &nparams, &maxparams, VIR_DOMAIN_BANDWIDTH_OUT_AVERAGE, outbound.average) < 0) diff --git a/tools/virsh.pod b/tools/virsh.pod index a6148d3..07e6ba7 100644 --- a/tools/virsh.pod +++ b/tools/virsh.pod @@ -782,7 +782,7 @@ query and show the bandwidth settings. Otherwise, it will set the inbound or outbound bandwidth. I<average,peak,burst,floor> is the same as in command I<attach-interface>. Values for I<average>, I<peak> and I<floor> are expressed in kilobytes per second, while I<burst> is expressed in kilobytes -in a single burst at -I<peak> speed as described in the Network XML +in a single burst at I<peak> speed as described in the Network XML documentation at L<http://libvirt.org/formatnetwork.html#elementQoS>;. To clear inbound or outbound settings, use I<--inbound> or I<--outbound> @@ -2520,11 +2520,13 @@ instead of the default script not in addition to it; --script is valid only for interfaces of type I<bridge> and only for Xen domains. I<model> specifies the network device model to be presented to the domain. I<inbound> and I<outbound> control the bandwidth of the -interface. I<peak>, I<burst> and I<floor> are optional, so "average,peak", -"average,,burst" and "average" are also legal. Values for I<average> -and I<peak> are expressed in kilobytes per second, while I<burst> is -expressed in kilobytes in a single burst at -I<peak> speed as -described in the Network XML documentation at +interface. At least one from the I<average>, I<floor> pair must be +specified. The other two I<peak> and I<burst> are optional, so +"average,peak", "average,,burst", "average,,,floor", "average" and +",,,floor" are also legal. Values for I<average>, I<floor> and I<peak> +are expressed in kilobytes per second, while I<burst> is expressed in +kilobytes in a single burst at I<peak> speed as described in the +Network XML documentation at L<http://libvirt.org/formatnetwork.html#elementQoS>;. If I<--live> is specified, affect a running domain. Michal

Well, there are just two places that needs adjustment: qemuDomainGetInterfaceParameters - to report the @floor qemuDomainSetInterfaceParameters - now that the function has been fixed, we can allow updating @floor too. Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; --- src/qemu/qemu_driver.c | 23 ++++++++++++++++++----- 1 file changed, 18 insertions(+), 5 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 171b58f..7123083 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -138,7 +138,7 @@ VIR_LOG_INIT("qemu.qemu_driver"); #define QEMU_NB_BLKIO_PARAM 6 -#define QEMU_NB_BANDWIDTH_PARAM 6 +#define QEMU_NB_BANDWIDTH_PARAM 7 static void processWatchdogEvent(virQEMUDriverPtr driver, virDomainObjPtr vm, @@ -11126,6 +11126,8 @@ qemuDomainSetInterfaceParameters(virDomainPtr dom, VIR_TYPED_PARAM_UINT, VIR_DOMAIN_BANDWIDTH_IN_BURST, VIR_TYPED_PARAM_UINT, + VIR_DOMAIN_BANDWIDTH_IN_FLOOR, + VIR_TYPED_PARAM_UINT, VIR_DOMAIN_BANDWIDTH_OUT_AVERAGE, VIR_TYPED_PARAM_UINT, VIR_DOMAIN_BANDWIDTH_OUT_PEAK, @@ -11178,6 +11180,9 @@ qemuDomainSetInterfaceParameters(virDomainPtr dom, bandwidth->in->peak = params[i].value.ui; } else if (STREQ(param->field, VIR_DOMAIN_BANDWIDTH_IN_BURST)) { bandwidth->in->burst = params[i].value.ui; + } else if (STREQ(param->field, VIR_DOMAIN_BANDWIDTH_IN_FLOOR)) { + bandwidth->in->floor = params[i].value.ui; + inboundSpecified = true; } else if (STREQ(param->field, VIR_DOMAIN_BANDWIDTH_OUT_AVERAGE)) { bandwidth->out->average = params[i].value.ui; outboundSpecified = true; @@ -11191,7 +11196,7 @@ qemuDomainSetInterfaceParameters(virDomainPtr dom, /* average is mandatory, peak and burst are optional. So if no * average is given, we free inbound/outbound here which causes * inbound/outbound to not be set. */ - if (!bandwidth->in->average) + if (!bandwidth->in->average && !bandwidth->in->floor) VIR_FREE(bandwidth->in); if (!bandwidth->out->average) VIR_FREE(bandwidth->out); @@ -11355,7 +11360,15 @@ qemuDomainGetInterfaceParameters(virDomainPtr dom, if (net->bandwidth && net->bandwidth->in) params[i].value.ui = net->bandwidth->in->burst; break; - case 3: /* outbound.average */ + case 3: /* inbound.floor */ + if (virTypedParameterAssign(&params[i], + VIR_DOMAIN_BANDWIDTH_IN_FLOOR, + VIR_TYPED_PARAM_UINT, 0) < 0) + goto cleanup; + if (net->bandwidth && net->bandwidth->in) + params[i].value.ui = net->bandwidth->in->floor; + break; + case 4: /* outbound.average */ if (virTypedParameterAssign(&params[i], VIR_DOMAIN_BANDWIDTH_OUT_AVERAGE, VIR_TYPED_PARAM_UINT, 0) < 0) @@ -11363,7 +11376,7 @@ qemuDomainGetInterfaceParameters(virDomainPtr dom, if (net->bandwidth && net->bandwidth->out) params[i].value.ui = net->bandwidth->out->average; break; - case 4: /* outbound.peak */ + case 5: /* outbound.peak */ if (virTypedParameterAssign(&params[i], VIR_DOMAIN_BANDWIDTH_OUT_PEAK, VIR_TYPED_PARAM_UINT, 0) < 0) @@ -11371,7 +11384,7 @@ qemuDomainGetInterfaceParameters(virDomainPtr dom, if (net->bandwidth && net->bandwidth->out) params[i].value.ui = net->bandwidth->out->peak; break; - case 5: /* outbound.burst */ + case 6: /* outbound.burst */ if (virTypedParameterAssign(&params[i], VIR_DOMAIN_BANDWIDTH_OUT_BURST, VIR_TYPED_PARAM_UINT, 0) < 0) -- 2.4.6

On 11.08.2015 03:31, John Ferlan wrote: Either average or floor is required. This is what I am squashing in: diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index d92c6e0..2e44500 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -11197,9 +11197,9 @@ qemuDomainSetInterfaceParameters(virDomainPtr dom, } } - /* average is mandatory, peak and burst are optional. So if no - * average is given, we free inbound/outbound here which causes - * inbound/outbound to not be set. */ + /* average or floor are mandatory, peak and burst are optional. + * So if no average or floor is given, we free inbound/outbound + * here which causes inbound/outbound to not be set. */ if (!bandwidth->in->average && !bandwidth->in->floor) VIR_FREE(bandwidth->in); if (!bandwidth->out->average) Michal