Re: [libvirt] [PATCH 8/9] virsh: Implement VIR_DOMAIN_BANDWIDTH_IN_FLOOR

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(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 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(&params, &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...