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@xxxxxxxxxx>
> ---
> 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
>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list