[libvirt] [PATCH v4 6/7] blockjob: automatically use bytes for bandwidth in virsh

Trickier than I had hoped; but now that we can parse numbers with arbitrary scale, it's time to use those numbers to call into the new API if the old API would have rounded the input, while still gracefully falling back to the old API if the new one fails. * tools/virsh-domain.c (blockJobBandwidth): Add parameter, adjust return value. (blockJobImpl, cmdBlockCopy): Adjust callers. Signed-off-by: Eric Blake <eblake(a)redhat.com&gt; --- tools/virsh-domain.c | 154 +++++++++++++++++++++++++++++++++++++-------------- 1 file changed, 113 insertions(+), 41 deletions(-) diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index 594647a..14d6921 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -1471,18 +1471,29 @@ typedef enum { } 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. */ +/* Parse a block job bandwidth parameter. Caller must provide exactly + * one of @bandwidth or @bandwidthBytes. Returns -1 on error with + * message printed, 0 if no bandwidth needed, 1 if *bandwidth is set + * to non-zero MiB/s, and 2 if *bandwidth or *bandwidthBytes is + * non-zero bytes/s. */ static int blockJobBandwidth(vshControl *ctl, const vshCmd *cmd, - unsigned long *bandwidth) + unsigned long *bandwidth, unsigned long long *bandwidthBytes) { const char *value = NULL; int ret = vshCommandOptString(cmd, "bandwidth", &value); unsigned long long speed; + unsigned long long limit = ULLONG_MAX; + + if (bandwidth) { + *bandwidth = 0; + bandwidthBytes = &speed; + if (sizeof(*bandwidth) < sizeof(speed)) + limit = ULONG_MAX << 20ULL; + } else { + *bandwidthBytes = 0; + } - *bandwidth = 0; if (ret <= 0) return ret; @@ -1504,19 +1515,29 @@ blockJobBandwidth(vshControl *ctl, const vshCmd *cmd, /* 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) { + if (vshCommandOptScaledInt(cmd, "bandwidth", bandwidthBytes, 1024 * 1024, + limit) < 0) { vshError(ctl, _("could not parse bandwidth '%s'"), value); return -1; } + if (!bandwidth) /* bandwidthBytes is already correct */ + return 2; - /* Now scale it to MiB/s for the caller. What a pain. */ - speed = VIR_DIV_UP(speed, 1024 * 1024); + /* If the number fits in unsigned long, is not a multiple of + * MiB/s, and we haven't proven the new API flag will fail, then + * let the caller try it directly. Otherwise, scale it to MiB/s + * for the caller. What a pain. */ + if ((unsigned long) speed == speed && speed & (1024 * 1024 - 1) && + !ctl->blockJobNoBytes) { + ret = 2; + } else { + speed = VIR_DIV_UP(speed, 1024 * 1024); + ret = 1; + } /* We already checked that narrowing the type will fit. */ *bandwidth = speed; - return !!speed; + return ret; } @@ -1527,10 +1548,12 @@ blockJobImpl(vshControl *ctl, const vshCmd *cmd, virDomainPtr dom = NULL; const char *path; unsigned long bandwidth = 0; + int bandwidthScale; bool ret = false; const char *base = NULL; const char *top = NULL; unsigned int flags = 0; + int rc; if (!(dom = vshCommandOptDomain(ctl, cmd, NULL))) goto cleanup; @@ -1538,7 +1561,7 @@ blockJobImpl(vshControl *ctl, const vshCmd *cmd, if (vshCommandOptStringReq(ctl, cmd, "path", &path) < 0) goto cleanup; - if (blockJobBandwidth(ctl, cmd, &bandwidth) < 0) + if ((bandwidthScale = blockJobBandwidth(ctl, cmd, &bandwidth, NULL)) < 0) goto cleanup; switch (mode) { @@ -1551,7 +1574,19 @@ blockJobImpl(vshControl *ctl, const vshCmd *cmd, goto cleanup; break; case VSH_CMD_BLOCK_JOB_SPEED: - if (virDomainBlockJobSetSpeed(dom, path, bandwidth, 0) < 0) + if (bandwidthScale > 1) + flags |= VIR_DOMAIN_BLOCK_JOB_SPEED_BANDWIDTH_BYTES; + rc = virDomainBlockJobSetSpeed(dom, path, bandwidth, flags); + if (rc < 0 && bandwidthScale > 1 && + last_error->code == VIR_ERR_INVALID_ARG) { + /* try again with MiB/s */ + ctl->blockJobNoBytes = true; + flags &= ~VIR_DOMAIN_BLOCK_JOB_SPEED_BANDWIDTH_BYTES; + bandwidth = VIR_DIV_UP(bandwidth, 1024 * 1024); + vshResetLibvirtError(); + rc = virDomainBlockJobSetSpeed(dom, path, bandwidth, flags); + } + if (rc < 0) goto cleanup; break; case VSH_CMD_BLOCK_JOB_PULL: @@ -1559,15 +1594,28 @@ blockJobImpl(vshControl *ctl, const vshCmd *cmd, goto cleanup; if (vshCommandOptBool(cmd, "keep-relative")) flags |= VIR_DOMAIN_BLOCK_REBASE_RELATIVE; + /* This flag intentionally has the same value for both APIs */ + if (bandwidthScale > 1) + flags |= VIR_DOMAIN_BLOCK_PULL_BANDWIDTH_BYTES; + if (base || flags) + rc = virDomainBlockRebase(dom, path, base, bandwidth, flags); + else + rc = virDomainBlockPull(dom, path, bandwidth, flags); - if (base || flags) { - if (virDomainBlockRebase(dom, path, base, bandwidth, flags) < 0) - goto cleanup; - } else { - if (virDomainBlockPull(dom, path, bandwidth, 0) < 0) - goto cleanup; + if (rc < 0 && bandwidthScale > 1 && + last_error->code == VIR_ERR_INVALID_ARG) { + /* try again with MiB/s */ + ctl->blockJobNoBytes = true; + flags &= ~VIR_DOMAIN_BLOCK_PULL_BANDWIDTH_BYTES; + bandwidth = VIR_DIV_UP(bandwidth, 1024 * 1024); + vshResetLibvirtError(); + if (base || flags) + rc = virDomainBlockRebase(dom, path, base, bandwidth, flags); + else + rc = virDomainBlockPull(dom, path, bandwidth, flags); } - + if (rc < 0) + goto cleanup; break; case VSH_CMD_BLOCK_JOB_COMMIT: if (vshCommandOptStringReq(ctl, cmd, "base", &base) < 0 || @@ -1583,7 +1631,19 @@ blockJobImpl(vshControl *ctl, const vshCmd *cmd, flags |= VIR_DOMAIN_BLOCK_COMMIT_ACTIVE; if (vshCommandOptBool(cmd, "keep-relative")) flags |= VIR_DOMAIN_BLOCK_COMMIT_RELATIVE; - if (virDomainBlockCommit(dom, path, base, top, bandwidth, flags) < 0) + if (bandwidthScale > 1) + flags |= VIR_DOMAIN_BLOCK_COMMIT_BANDWIDTH_BYTES; + rc = virDomainBlockCommit(dom, path, base, top, bandwidth, flags); + if (rc < 0 && bandwidthScale > 1 && + last_error->code == VIR_ERR_INVALID_ARG) { + /* try again with MiB/s */ + ctl->blockJobNoBytes = true; + flags &= ~VIR_DOMAIN_BLOCK_PULL_BANDWIDTH_BYTES; + bandwidth = VIR_DIV_UP(bandwidth, 1024 * 1024); + vshResetLibvirtError(); + rc = virDomainBlockCommit(dom, path, base, top, bandwidth, flags); + } + if (rc < 0) goto cleanup; break; } @@ -1944,7 +2004,6 @@ cmdBlockCopy(vshControl *ctl, const vshCmd *cmd) virDomainPtr dom = NULL; const char *dest = NULL; const char *format = NULL; - unsigned long bandwidth = 0; unsigned int granularity = 0; unsigned long long buf_size = 0; unsigned int flags = 0; @@ -2010,8 +2069,6 @@ cmdBlockCopy(vshControl *ctl, const vshCmd *cmd) if (!(dom = vshCommandOptDomain(ctl, cmd, NULL))) goto cleanup; - if (blockJobBandwidth(ctl, cmd, &bandwidth) < 0) - goto cleanup; if (vshCommandOptUInt(cmd, "granularity", &granularity) < 0) { vshError(ctl, "%s", _("granularity must be a number")); goto cleanup; @@ -2040,22 +2097,18 @@ cmdBlockCopy(vshControl *ctl, const vshCmd *cmd) if (granularity || buf_size || (format && STRNEQ(format, "raw")) || xml) { /* New API */ - if (bandwidth || granularity || buf_size) { + unsigned long long bandwidthBytes = 0; + if (blockJobBandwidth(ctl, cmd, NULL, &bandwidthBytes) < 0) + goto cleanup; + + if (bandwidthBytes || granularity || buf_size) { params = vshCalloc(ctl, 3, sizeof(*params)); - if (bandwidth) { - /* bandwidth is ulong MiB/s, but the typed parameter is - * ullong bytes/s; make sure we don't overflow */ - if (bandwidth + 0ULL > ULLONG_MAX >> 20) { - virReportError(VIR_ERR_OVERFLOW, - _("bandwidth must be less than %llu"), - ULLONG_MAX >> 20); - } - if (virTypedParameterAssign(&params[nparams++], - VIR_DOMAIN_BLOCK_COPY_BANDWIDTH, - VIR_TYPED_PARAM_ULLONG, - bandwidth << 20ULL) < 0) - goto cleanup; - } + if (bandwidthBytes && + virTypedParameterAssign(&params[nparams++], + VIR_DOMAIN_BLOCK_COPY_BANDWIDTH, + VIR_TYPED_PARAM_ULLONG, + bandwidthBytes) < 0) + goto cleanup; if (granularity && virTypedParameterAssign(&params[nparams++], VIR_DOMAIN_BLOCK_COPY_GRANULARITY, @@ -2088,14 +2141,33 @@ cmdBlockCopy(vshControl *ctl, const vshCmd *cmd) if (virDomainBlockCopy(dom, path, xmlstr, params, nparams, flags) < 0) goto cleanup; } else { - /* Old API */ + /* Old API, parse scaled bandwidth */ + int rc; + unsigned long bandwidth = 0; + int bandwidthScale; + + if ((bandwidthScale = blockJobBandwidth(ctl, cmd, &bandwidth, + NULL)) < 0) + goto cleanup; flags |= VIR_DOMAIN_BLOCK_REBASE_COPY; if (vshCommandOptBool(cmd, "blockdev")) flags |= VIR_DOMAIN_BLOCK_REBASE_COPY_DEV; if (STREQ_NULLABLE(format, "raw")) flags |= VIR_DOMAIN_BLOCK_REBASE_COPY_RAW; + if (bandwidthScale > 1) + flags |= VIR_DOMAIN_BLOCK_REBASE_BANDWIDTH_BYTES; - if (virDomainBlockRebase(dom, path, dest, bandwidth, flags) < 0) + rc = virDomainBlockRebase(dom, path, dest, bandwidth, flags); + if (rc < 0 && bandwidthScale > 1 && + last_error->code == VIR_ERR_INVALID_ARG) { + /* try again with MiB/s */ + ctl->blockJobNoBytes = true; + flags &= ~VIR_DOMAIN_BLOCK_REBASE_BANDWIDTH_BYTES; + bandwidth = VIR_DIV_UP(bandwidth, 1024 * 1024); + vshResetLibvirtError(); + rc = virDomainBlockRebase(dom, path, dest, bandwidth, flags); + } + if (rc < 0) goto cleanup; } -- 1.9.3