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

[John Ferlan]

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 at 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(&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...

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
>