[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH v4 5/7] blockjob: improve handling of bandwidth in virsh



On 09/12/14 05:55, Eric Blake wrote:
> Treating -1 as the maximum bandwidth of block jobs is not very
> practical, given the restrictions on overflow between 32-bit
> vs. 64-bit long, as well as conversions between MiB/s and bytes/s.
> We already document that 0 means unlimited, so the only reason to
> keep negative support is for back-compat.
> 
> Meanwhile, it is a good idea to allow users to input in scales
> other than MiB/s.  This patch just rounds back up to MiB/s, but by
> using a common helper, a later patch can then determine when a
> particular input scale will be better for using flags with the
> corresponding API call.
> 
> * tools/virsh-domain.c (blockJobBandwidth): New function.
> (blockJobImpl, cmdBlockCopy): Use it.
> * tools/virsh.pod (blockjob, blockcopy, blockpull, blockcommit):
> Document this.
> 
> Signed-off-by: Eric Blake <eblake redhat com>
> ---
>  tools/virsh-domain.c | 73 ++++++++++++++++++++++++++++++++++++++++++----------
>  tools/virsh.pod      | 42 ++++++++++++++++++------------
>  2 files changed, 84 insertions(+), 31 deletions(-)
> 
> diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
> index cc1e554..594647a 100644
> --- a/tools/virsh-domain.c
> +++ b/tools/virsh-domain.c

> @@ -1467,6 +1470,56 @@ typedef enum {
>      VSH_CMD_BLOCK_JOB_COMMIT,
>  } vshCmdBlockJobMode;
> 
> +
> +/* Parse a block job bandwidth parameter.  Returns -1 on error with
> + * message printed, 0 if no bandwidth needed, and 1 if *bandwidth is
> + * set to non-zero.  */
> +static int
> +blockJobBandwidth(vshControl *ctl, const vshCmd *cmd,

vshBlockJobBandwidth perhaps?

> +                  unsigned long *bandwidth)
> +{
> +    const char *value = NULL;
> +    int ret = vshCommandOptString(cmd, "bandwidth", &value);
> +    unsigned long long speed;
> +
> +    *bandwidth = 0;
> +    if (ret <= 0)
> +        return ret;
> +
> +    /* For back-compat, accept -1 as meaning unlimited, by converting
> +     * negative values to 0 (and forbid wraparound back to positive).
> +     * Alas, we have to parse the raw string ourselves, instead of
> +     * using vshCommandOptUL.  If only we had never allowed wraparound
> +     * on bandwidth.  */
> +    if (strchr(value, '-')) {
> +        unsigned long tmp;
> +
> +        if (vshCommandOptULWrap(cmd, "bandwidth", &tmp) < 0 ||
> +            (long) tmp >= 0) {
> +            vshError(ctl, _("could not parse bandwidth '%s'"), value);
> +            return -1;
> +        }
> +        return 0;
> +    }
> +
> +    /* Read a number in bytes/s, but with default unit of MiB/s if no
> +     * unit was specified. */
> +    if (vshCommandOptScaledInt(cmd, "bandwidth", &speed, 1024 * 1024,
> +                               (sizeof(*bandwidth) < sizeof(speed) ?
> +                                ULONG_MAX * 1024 * 1024 : ULLONG_MAX)) < 0) {
> +        vshError(ctl, _("could not parse bandwidth '%s'"), value);
> +        return -1;
> +    }
> +
> +    /* Now scale it to MiB/s for the caller.  What a pain.  */
> +    speed = VIR_DIV_UP(speed, 1024 * 1024);
> +
> +    /* We already checked that narrowing the type will fit.  */
> +    *bandwidth = speed;
> +    return !!speed;
> +}
> +
> +
>  static bool
>  blockJobImpl(vshControl *ctl, const vshCmd *cmd,
>               vshCmdBlockJobMode mode, virDomainPtr *pdom)

ACK,

Peter

Attachment: signature.asc
Description: OpenPGP digital signature


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]