On 08/03/2015 02:39 AM, Michal Privoznik wrote:

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@redhat.com> --- 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)

You'll need to remove the ATTRIBUTE_NONNULL(2) to make Coverity builds happy... You can move the ATTRIBUTE_RETURN_CHECK to the same line if you want also. ACK with that adjustment, John

On 11.08.2015 02:49, John Ferlan wrote:

On 08/03/2015 02:39 AM, Michal Privoznik wrote:

...

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@redhat.com> --- src/network/bridge_driver.c | 4 ++-- src/util/virnetdevbandwidth.c | 10 +++++++--- src/util/virnetdevbandwidth.h | 2 +- 3 files changed, 10 insertions(+), 6 deletions(-)

something that dawned on me reading later patches...

...

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)

^^^ Comment says MUST be > 2, but there's no check/error... Also I note "2" is passed often...

Yeah, this is basically my personal note. In fact, it's not valid at all and I will remove it. The intent was that in the QoS tree that is created on the bridge, class/leaf #2 has a specific purpose => all nonguaranteed traffic goes through it. Therefore it should be updated with higher caution than anything else. But this comment is just misleading. Michal

On 08/03/2015 02:39 AM, Michal Privoznik wrote:

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@redhat.com> --- 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)

new_rate is unchanged if passed as NULL too Part of me says - who cares - just set it and let the caller decide - both paths will "always have" that extra "if" check now... and it only ever matters if plugging is safe. I'm OK keeping as is - so I'll ACK and let you decide. At the very least point out that new_rate can be NULL in the comments. I'm guessing that if the caller that has it NULL had a value, then there'd be a warning about an unused variable... John

@@ -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);

On 08/03/2015 02:39 AM, Michal Privoznik wrote:

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@redhat.com> --- 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);

^^ 'iface' is used here... but !iface checked below...

unsigned long long old_floor, new_floor;

+ if (!iface && !newBandwidth) + return false; +

Adding this !iface check causes Coverity issues for the following usage of 'iface' (especially if 'newBandwidth' is non NULL)... Checking the two (eventual) callers - networkBandwidthChangeAllowed already has ATTRIBUTE_NONNULL(1) (2) checks and it seems logically at least that networkBandwidthUpdate should have it. Thus I don't think it's necessary (although I may have read things wrong too ;-))

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);

^^ 'iface' is used unconditionally here So arg1 can have the ATTRIBUTE_NONNULL 'newBandwidth' is used below, so it too can have ATTRIBUTE_NONNULL Which means check in GenericChecks is unnecessary - although to be safe we could add the ATTRIBUTE_NONNULL to the static decl too.

+ 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); +

I would think based on code ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2); would be need to be added. Additionally, networkBandwidthChangeAllowed has them and this is called after that... Beyond that I think things look OK ACK with some adjustments John

# 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);

On 08/03/2015 02:39 AM, Michal Privoznik wrote:

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@redhat.com> --- 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) {

Is this the only caller of virNetDevBandwidthSet that needs the "ChangeAllowed" check first? I think the answer is no based on your explanation in the commit message, but figured I'd ask... Tough to keep track of the multiple callers and what they're checking before calling the Set and/or if they need to Update as well (it's been a long day ;-), but I figured I'd at least look... ACK for the change in any case. John

ignore_value(virNetDevBandwidthSet(net->ifname, net->bandwidth, false));

On 11.08.2015 03:09, John Ferlan wrote:

On 08/03/2015 02:39 AM, Michal Privoznik wrote:

...

The function is used to parse a tuple delimited by commas into virNetDevBandwidth structure. So far only three out of fore fields are supported: average, peak and burst. The single missing field is floor. Well, the parsing works, but I think we can do better. Especially when we will need to parse floor too in very close future.

Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- tools/virsh-domain.c | 80 ++++++++++++++++++++++++++++++---------------------- 1 file changed, 47 insertions(+), 33 deletions(-)

diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index a61656f..bb40ddd 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -865,36 +865,58 @@ static const vshCmdOptDef opts_attach_interface[] = { /* parse inbound and outbound which are in the format of * 'average,peak,burst', in which peak and burst are optional, * thus 'average,,burst' and 'average,peak' are also legal. */ -static int parseRateStr(const char *rateStr, virNetDevBandwidthRatePtr rate) +static int parseRateStr(vshControl *ctl, + const char *rateStr, + virNetDevBandwidthRatePtr rate) { - const char *average = NULL; - char *peak = NULL, *burst = NULL; + char *token; + char *next; + char *saveptr;

My compiler complained about uninitialized value here especially when used with strtok_r (setting = NULL pacifies compiler).

Interesting since I want strtok_r to initialize the variable to a value. Whatever, initializing to NULL - I can live with that.

ACK with the adjustment.

John

Michal

On 08/03/2015 02:39 AM, Michal Privoznik wrote:

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@redhat.com> --- 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

Some comments above here need adjustment to describe the new field and possible options... Does it matter if someone provides "floor" on outbound (it's a testing question ;-))

@@ -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"));

Why checking !inbound.floor? What if someone does ",,,<floor value>"

goto cleanup; }

Also should we check below here for outboundStr having floor? (improperly)

@@ -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(¶ms, &nparams, &maxparams, + VIR_DOMAIN_BANDWIDTH_IN_FLOOR, + inbound.floor) < 0) + goto save_error; }

if (outboundStr) {

... should we check here if someone provides outbound.floor value and fail?

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

"average,,,floor", I'm OK with just seeing a diff for a final ACK... John

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

On 08/03/2015 02:39 AM, Michal Privoznik wrote:

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@redhat.com> --- 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;

If 'average' is required, then can one really provide just floor?

} 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. */

Adjust comment to account for floor...

- if (!bandwidth->in->average) + if (!bandwidth->in->average && !bandwidth->in->floor)

Again, I thought average was required ... sorry it's just late. Maybe the updated comment will answer my query... ACK in principle John

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(¶ms[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(¶ms[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(¶ms[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(¶ms[i], VIR_DOMAIN_BANDWIDTH_OUT_BURST, VIR_TYPED_PARAM_UINT, 0) < 0)