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

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(a)redhat.com&gt; --- 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,

+ 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)