[libvirt] [PATCH 5/8] virsh: block job: Support --bytes and scaled integers when setting speed

[John Ferlan]

On 04/30/2015 10:45 AM, Peter Krempa wrote:
> Allow specifying sizes in bytes or as scaled integers for user
> convenience.
> ---
>  tools/virsh-domain.c | 31 ++++++++++++++++++++++++-------
>  tools/virsh.pod      | 10 +++++++---
>  2 files changed, 31 insertions(+), 10 deletions(-)
> 
> diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
> index 0d0e39e..fef3918 100644
> --- a/tools/virsh-domain.c
> +++ b/tools/virsh-domain.c
> @@ -2541,16 +2541,35 @@ static bool
>  vshBlockJobSetSpeed(vshControl *ctl,
>                      const vshCmd *cmd,
>                      virDomainPtr dom,
> -                    const char *path)
> +                    const char *path,
> +                    bool bytes)
>  {
>      unsigned long bandwidth;
> +    unsigned long long bw;
> +    unsigned int flags = 0;
> +
> +    if (bytes)
> +        flags |= VIR_DOMAIN_BLOCK_JOB_SPEED_BANDWIDTH_BYTES;
> 
>      if (vshCommandOptULWrap(cmd, "bandwidth", &bandwidth) < 0) {
> -        vshError(ctl, "%s", _("bandwidth must be a number"));
> -        return false;
> +        /* try to parse the number as scaled size */
> +        if (vshCommandOptScaledInt(cmd, "bandwidth", &bw, 1, ULLONG_MAX) < 0) {
> +            vshError(ctl, "%s", _("bandwidth must be a number"));
> +            return false;
> +        }
> +
> +        if (!bytes)
> +            bw /= 1024 * 1024;

Or like other code with does the shifting by 20 bits...

> +
> +        if (bw > ULONG_MAX) {

Coverity complains CONSTANT_EXPRESSION_RESULT

(1) Event result_independent_of_operands: 	"bw > 18446744073709551615ULL
/* 9223372036854775807L * 2UL + 1UL */" is always false regardless of
the values of its operands. This occurs as the logical operand of if.

However, why even make the check here? Since virsh.pod astutely points
out "The hypervisor can choose whether to reject the value or convert it
to the maximum value allowed."  I see in qemuDomainBlockJobSetSpeed that
bandwidth maximum is checked.

> +            vshError(ctl, _("bandwidth must be less than %lu"), ULONG_MAX);
> +            return false;
> +        }
> +
> +        bandwidth = bw;
>      }
> 
> -    if (virDomainBlockJobSetSpeed(dom, path, bandwidth, 0) < 0)
> +    if (virDomainBlockJobSetSpeed(dom, path, bandwidth, flags) < 0)
>          return false;
> 
>      return true;
> @@ -2584,8 +2603,6 @@ cmdBlockJob(vshControl *ctl, const vshCmd *cmd)
>      VSH_EXCLUSIVE_OPTIONS_VAR(bytes, abort);
>      VSH_EXCLUSIVE_OPTIONS_VAR(bytes, pivot);
>      VSH_EXCLUSIVE_OPTIONS_VAR(bytes, async);
> -    /* XXX also support --bytes with bandwidth mode */
> -    VSH_EXCLUSIVE_OPTIONS_VAR(bytes, bandwidth);
> 
>      if (abort || pivot || async)
>          return blockJobImpl(ctl, cmd, VSH_CMD_BLOCK_JOB_ABORT, NULL);
> @@ -2598,7 +2615,7 @@ cmdBlockJob(vshControl *ctl, const vshCmd *cmd)
>          goto cleanup;
> 
>      if (bandwidth)
> -        ret = vshBlockJobSetSpeed(ctl, cmd, dom, path);
> +        ret = vshBlockJobSetSpeed(ctl, cmd, dom, path, bytes);
>      else
>          ret = vshBlockJobInfo(ctl, dom, path, raw, bytes);
> 
> diff --git a/tools/virsh.pod b/tools/virsh.pod
> index f576fb0..85743c6 100644
> --- a/tools/virsh.pod
> +++ b/tools/virsh.pod
> @@ -1151,10 +1151,14 @@ not supply bytes/s resolution; when omitting the flag, raw output is
>  listed in MiB/s and human-readable output automatically selects the
>  best resolution supported by the server.
> 
> -I<--bandwidth> can be used to set bandwidth limit for the active job.
> -Specifying a negative value is interpreted as an unsigned long long
> +I<--bandwidth> can be used to set bandwidth limit for the active job in MiB/s.
> +If I<--bytes> is specified then the bandwidth value is interpreted in bytes/s.
> +Specifying a negative value is interpreted as an unsigned long
>  value or essentially unlimited. The hypervisor can choose whether to
> -reject the value or convert it to the maximum value allowed.
> +reject the value or convert it to the maximum value allowed. Optionally a
> +scaled positive number may be used as bandwidth (see B<NOTES> above). Using
> +I<--bytes> with a scaled value allows to use finer granularity. Note that the
> +I<--bytes> may be unsupported by the hypervisor.

You'll have adjustments here too based on 4/8 changes...  Also depending
on your response to 3/8 - you may need to change the discussion to only
support scaled values for SetSpeed

ACK with the ULONG_MAX check removed and I trust there will be an
adjustment to the text here based on how bandwidth can be used...

John

> 
>  =item B<blockresize> I<domain> I<path> I<size>
>