[libvirt] [PATCH v4 0/7] wrap up virDomainBlockCopy
diff to v3: patch 1 now consumes the destination in the common helper instead of copying it patch 2 rebased accordingly patch 3 didn't need much work patch 4 finally tested patches 5-7 are new Also available at git://repo.or.cz/libvirt/ericb.git branch blockcopy, http://repo.or.cz/w/libvirt/ericb.git/shortlog/refs/heads/blockcopy Eric Blake (7): blockcopy: tweak how rebase calls into copy blockcopy: add qemu implementation of new API blockcopy: add qemu implementation of new tunables blockjob: allow finer bandwidth tuning for set speed blockjob: improve handling of bandwidth in virsh blockjob: automatically use bytes for bandwidth in virsh blockjob: allow forcing of bytes scale in virsh include/libvirt/libvirt.h.in | 31 ++++-- src/libvirt.c | 83 +++++++++----- src/qemu/qemu_driver.c | 255 +++++++++++++++++++++++++++++-------------- src/qemu/qemu_migration.c | 2 +- src/qemu/qemu_monitor.c | 8 +- src/qemu/qemu_monitor.h | 2 + src/qemu/qemu_monitor_json.c | 4 + src/qemu/qemu_monitor_json.h | 2 + tests/qemumonitorjsontest.c | 2 +- tools/virsh-domain.c | 226 ++++++++++++++++++++++++++++++-------- tools/virsh.pod | 50 +++++---- 11 files changed, 481 insertions(+), 184 deletions(-) -- 1.9.3
In order to implement the new virDomainBlockCopy, the existing block copy internal implementation needs to be adjusted. The new function will parse XML into a storage source, and parse typed parameters into integers, then call into the same common backend. For now, it's easier to keep the same implementation limits that only local file destinations are suported, but now the check needs to be explicit. Similar to qemuDomainBlockJobImpl consuming 'vm', this code also consumes the caller's 'mirror' description of the destination. * src/qemu/qemu_driver.c (qemuDomainBlockCopy): Rename... (qemuDomainBlockCopyCommon): ...and adjust parameters. (qemuDomainBlockRebase): Adjust caller. Signed-off-by: Eric Blake <eblake@redhat.com> --- src/qemu/qemu_driver.c | 118 ++++++++++++++++++++++++++----------------------- 1 file changed, 63 insertions(+), 55 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 917b286..75c529b 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -14916,7 +14916,8 @@ qemuDomainBlockPivot(virConnectPtr conn, } -/* bandwidth in MiB/s per public API */ +/* bandwidth in MiB/s per public API. Caller must lock vm beforehand, + * and not access it afterwards. */ static int qemuDomainBlockJobImpl(virDomainObjPtr vm, virConnectPtr conn, @@ -15277,13 +15278,16 @@ qemuDomainBlockJobSetSpeed(virDomainPtr dom, const char *path, } -/* bandwidth in bytes/s */ +/* bandwidth in bytes/s. Caller must lock vm beforehand, and not + * access it afterwards; likewise, caller must not access mirror + * afterwards. */ static int -qemuDomainBlockCopy(virDomainObjPtr vm, - virConnectPtr conn, - const char *path, - const char *dest, const char *format, - unsigned long long bandwidth, unsigned int flags) +qemuDomainBlockCopyCommon(virDomainObjPtr vm, + virConnectPtr conn, + const char *path, + virStorageSourcePtr mirror, + unsigned long long bandwidth, + unsigned int flags) { virQEMUDriverPtr driver = conn->privateData; qemuDomainObjPrivatePtr priv; @@ -15293,13 +15297,13 @@ qemuDomainBlockCopy(virDomainObjPtr vm, int idx; struct stat st; bool need_unlink = false; - virStorageSourcePtr mirror = NULL; virQEMUDriverConfigPtr cfg = NULL; + const char *format = NULL; + int desttype = virStorageSourceGetActualType(mirror); /* Preliminaries: find the disk we are editing, sanity checks */ - virCheckFlags(VIR_DOMAIN_BLOCK_REBASE_SHALLOW | - VIR_DOMAIN_BLOCK_REBASE_REUSE_EXT | - VIR_DOMAIN_BLOCK_REBASE_COPY_DEV, -1); + virCheckFlags(VIR_DOMAIN_BLOCK_COPY_SHALLOW | + VIR_DOMAIN_BLOCK_COPY_REUSE_EXT, -1); priv = vm->privateData; cfg = virQEMUDriverGetConfig(driver); @@ -15344,8 +15348,8 @@ qemuDomainBlockCopy(virDomainObjPtr vm, if (qemuDomainDetermineDiskChain(driver, vm, disk, false) < 0) goto endjob; - if ((flags & VIR_DOMAIN_BLOCK_REBASE_SHALLOW) && - STREQ_NULLABLE(format, "raw") && + if ((flags & VIR_DOMAIN_BLOCK_COPY_SHALLOW) && + mirror->format == VIR_STORAGE_FILE_RAW && disk->src->backingStore->path) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("disk '%s' has backing file, so raw shallow copy " @@ -15355,72 +15359,65 @@ qemuDomainBlockCopy(virDomainObjPtr vm, } /* Prepare the destination file. */ - if (stat(dest, &st) < 0) { + /* XXX Allow non-file mirror destinations */ + if (!virStorageSourceIsLocalStorage(mirror)) { + virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s", + _("non-file destination not supported yet")); + } + if (stat(mirror->path, &st) < 0) { if (errno != ENOENT) { virReportSystemError(errno, _("unable to stat for disk %s: %s"), - disk->dst, dest); + disk->dst, mirror->path); goto endjob; - } else if (flags & (VIR_DOMAIN_BLOCK_REBASE_REUSE_EXT | - VIR_DOMAIN_BLOCK_REBASE_COPY_DEV)) { + } else if (flags & VIR_DOMAIN_BLOCK_COPY_REUSE_EXT || + desttype == VIR_STORAGE_TYPE_BLOCK) { virReportSystemError(errno, _("missing destination file for disk %s: %s"), - disk->dst, dest); + disk->dst, mirror->path); goto endjob; } } else if (!S_ISBLK(st.st_mode)) { - if (st.st_size && !(flags & VIR_DOMAIN_BLOCK_REBASE_REUSE_EXT)) { + if (st.st_size && !(flags & VIR_DOMAIN_BLOCK_COPY_REUSE_EXT)) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("external destination file for disk %s already " "exists and is not a block device: %s"), - disk->dst, dest); + disk->dst, mirror->path); goto endjob; } - if (flags & VIR_DOMAIN_BLOCK_REBASE_COPY_DEV) { + if (desttype == VIR_STORAGE_TYPE_BLOCK) { virReportError(VIR_ERR_INVALID_ARG, _("blockdev flag requested for disk %s, but file " - "'%s' is not a block device"), disk->dst, dest); + "'%s' is not a block device"), + disk->dst, mirror->path); goto endjob; } } - if (VIR_ALLOC(mirror) < 0) - goto endjob; - /* XXX Allow non-file mirror destinations */ - mirror->type = flags & VIR_DOMAIN_BLOCK_REBASE_COPY_DEV ? - VIR_STORAGE_TYPE_BLOCK : VIR_STORAGE_TYPE_FILE; - - if (format) { - if ((mirror->format = virStorageFileFormatTypeFromString(format)) <= 0) { - virReportError(VIR_ERR_INVALID_ARG, _("unrecognized format '%s'"), - format); - goto endjob; - } - } else { - if (!(flags & VIR_DOMAIN_BLOCK_REBASE_REUSE_EXT)) { + if (!mirror->format) { + if (!(flags & VIR_DOMAIN_BLOCK_COPY_REUSE_EXT)) { mirror->format = disk->src->format; } else { /* If the user passed the REUSE_EXT flag, then either they - * also passed the RAW flag (and format is non-NULL), or it is - * safe for us to probe the format from the file that we will - * be using. */ - mirror->format = virStorageFileProbeFormat(dest, cfg->user, + * can also pass the RAW flag or use XML to tell us the format. + * So if we get here, we assume it is safe for us to probe the + * format from the file that we will be using. */ + mirror->format = virStorageFileProbeFormat(mirror->path, cfg->user, cfg->group); } } /* pre-create the image file */ - if (!(flags & VIR_DOMAIN_BLOCK_REBASE_REUSE_EXT)) { - int fd = qemuOpenFile(driver, vm, dest, O_WRONLY | O_TRUNC | O_CREAT, + if (!(flags & VIR_DOMAIN_BLOCK_COPY_REUSE_EXT)) { + int fd = qemuOpenFile(driver, vm, mirror->path, + O_WRONLY | O_TRUNC | O_CREAT, &need_unlink, NULL); if (fd < 0) goto endjob; VIR_FORCE_CLOSE(fd); } - if (!format && mirror->format > 0) + if (mirror->format > 0) format = virStorageFileFormatTypeToString(mirror->format); - if (VIR_STRDUP(mirror->path, dest) < 0) - goto endjob; if (virStorageSourceInitChainElement(mirror, disk->src, false) < 0) goto endjob; @@ -15434,8 +15431,8 @@ qemuDomainBlockCopy(virDomainObjPtr vm, /* Actually start the mirroring */ qemuDomainObjEnterMonitor(driver, vm); - ret = qemuMonitorDriveMirror(priv->mon, device, dest, format, bandwidth, - flags); + ret = qemuMonitorDriveMirror(priv->mon, device, mirror->path, format, + bandwidth, flags); virDomainAuditDisk(vm, NULL, mirror, "mirror", ret >= 0); qemuDomainObjExitMonitor(driver, vm); if (ret < 0) { @@ -15455,8 +15452,8 @@ qemuDomainBlockCopy(virDomainObjPtr vm, vm->def->name); endjob: - if (need_unlink && unlink(dest)) - VIR_WARN("unable to unlink just-created %s", dest); + if (need_unlink && unlink(mirror->path)) + VIR_WARN("unable to unlink just-created %s", mirror->path); virStorageSourceFree(mirror); if (!qemuDomainObjEndJob(driver, vm)) vm = NULL; @@ -15474,9 +15471,9 @@ qemuDomainBlockRebase(virDomainPtr dom, const char *path, const char *base, unsigned long bandwidth, unsigned int flags) { virDomainObjPtr vm; - const char *format = NULL; int ret = -1; unsigned long long speed = bandwidth; + virStorageSourcePtr dest = NULL; virCheckFlags(VIR_DOMAIN_BLOCK_REBASE_SHALLOW | VIR_DOMAIN_BLOCK_REBASE_REUSE_EXT | @@ -15498,8 +15495,14 @@ qemuDomainBlockRebase(virDomainPtr dom, const char *path, const char *base, BLOCK_JOB_PULL, flags); /* If we got here, we are doing a block copy rebase. */ + if (VIR_ALLOC(dest) < 0) + goto cleanup; + dest->type = (flags & VIR_DOMAIN_BLOCK_REBASE_COPY_DEV) ? + VIR_STORAGE_TYPE_BLOCK : VIR_STORAGE_TYPE_FILE; + if (VIR_STRDUP(dest->path, base) < 0) + goto cleanup; if (flags & VIR_DOMAIN_BLOCK_REBASE_COPY_RAW) - format = "raw"; + dest->format = VIR_STORAGE_FILE_RAW; /* Convert bandwidth MiB to bytes */ if (speed > LLONG_MAX >> 20) { @@ -15519,15 +15522,20 @@ qemuDomainBlockRebase(virDomainPtr dom, const char *path, const char *base, goto cleanup; } + /* We rely on the fact that VIR_DOMAIN_BLOCK_REBASE_SHALLOW + * and VIR_DOMAIN_BLOCK_REBASE_REUSE_EXT map to the same values + * as for block copy. */ flags &= (VIR_DOMAIN_BLOCK_REBASE_SHALLOW | - VIR_DOMAIN_BLOCK_REBASE_REUSE_EXT | - VIR_DOMAIN_BLOCK_REBASE_COPY_DEV); - ret = qemuDomainBlockCopy(vm, dom->conn, path, base, format, - bandwidth, flags); + VIR_DOMAIN_BLOCK_REBASE_REUSE_EXT); + ret = qemuDomainBlockCopyCommon(vm, dom->conn, path, dest, + bandwidth, flags); vm = NULL; + dest = NULL; + cleanup: if (vm) virObjectUnlock(vm); + virStorageSourceFree(dest); return ret; } -- 1.9.3
On 09/12/14 05:55, Eric Blake wrote:
In order to implement the new virDomainBlockCopy, the existing block copy internal implementation needs to be adjusted. The new function will parse XML into a storage source, and parse typed parameters into integers, then call into the same common backend. For now, it's easier to keep the same implementation limits that only local file destinations are suported, but now the check needs to be explicit. Similar to qemuDomainBlockJobImpl consuming 'vm', this code also consumes the caller's 'mirror' description of the destination.
* src/qemu/qemu_driver.c (qemuDomainBlockCopy): Rename... (qemuDomainBlockCopyCommon): ...and adjust parameters. (qemuDomainBlockRebase): Adjust caller.
Signed-off-by: Eric Blake <eblake@redhat.com> --- src/qemu/qemu_driver.c | 118 ++++++++++++++++++++++++++----------------------- 1 file changed, 63 insertions(+), 55 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 917b286..75c529b 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c
ACK, Peter
The hard part of managing the disk copy is already coded; all this had to do was convert the XML and virTypedParameters into the internal representation. With this patch, all blockcopy operations that used the old API should also work via the new API. Additional extensions, such as supporting the granularity tunable or a network rather than file destination, will be added as later patches. * src/qemu/qemu_driver.c (qemuDomainBlockCopy): New function. Signed-off-by: Eric Blake <eblake@redhat.com> --- src/qemu/qemu_driver.c | 84 ++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 84 insertions(+) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 75c529b..6895b23 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -15539,6 +15539,89 @@ qemuDomainBlockRebase(virDomainPtr dom, const char *path, const char *base, return ret; } + +static int +qemuDomainBlockCopy(virDomainPtr dom, const char *disk, const char *destxml, + virTypedParameterPtr params, int nparams, + unsigned int flags) +{ + virQEMUDriverPtr driver = dom->conn->privateData; + virDomainObjPtr vm; + int ret = -1; + unsigned long long bandwidth = 0; + unsigned int granularity = 0; + unsigned long long buf_size = 0; + virStorageSourcePtr dest = NULL; + size_t i; + + virCheckFlags(VIR_DOMAIN_BLOCK_COPY_SHALLOW | + VIR_DOMAIN_BLOCK_COPY_REUSE_EXT, -1); + if (virTypedParamsValidate(params, nparams, + VIR_DOMAIN_BLOCK_COPY_BANDWIDTH, + VIR_TYPED_PARAM_ULLONG, + VIR_DOMAIN_BLOCK_COPY_GRANULARITY, + VIR_TYPED_PARAM_UINT, + VIR_DOMAIN_BLOCK_COPY_BUF_SIZE, + VIR_TYPED_PARAM_ULLONG, + NULL) < 0) + return -1; + + if (!(vm = qemuDomObjFromDomain(dom))) + return -1; + + if (virDomainBlockCopyEnsureACL(dom->conn, vm->def) < 0) + goto cleanup; + + for (i = 0; i < nparams; i++) { + virTypedParameterPtr param = ¶ms[i]; + + /* Typed params (wisely) refused to expose unsigned long, but + * back-compat demands that we stick with a maximum of + * unsigned long bandwidth in MiB/s, while our value is + * unsigned long long in bytes/s. Hence, we have to do + * overflow detection if this is a 32-bit server handling a + * 64-bit client. */ + if (STREQ(param->field, VIR_DOMAIN_BLOCK_COPY_BANDWIDTH)) { + if (sizeof(unsigned long) < sizeof(bandwidth) && + param->value.ul > ULONG_MAX * (1ULL << 20)) { + virReportError(VIR_ERR_OVERFLOW, + _("bandwidth must be less than %llu bytes"), + ULONG_MAX * (1ULL << 20)); + goto cleanup; + } + bandwidth = param->value.ul; + } else if (STREQ(param->field, VIR_DOMAIN_BLOCK_COPY_GRANULARITY)) { + granularity = param->value.ui; + } else if (STREQ(param->field, VIR_DOMAIN_BLOCK_COPY_BUF_SIZE)) { + buf_size = param->value.ul; + } + } + if (granularity) { + virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s", + _("granularity tuning not supported yet")); + goto cleanup; + } + if (buf_size) { + virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s", + _("buffer size tuning not supported yet")); + goto cleanup; + } + + if (!(dest = virDomainDiskDefSourceParse(destxml, vm->def, driver->xmlopt, + VIR_DOMAIN_XML_INACTIVE))) + goto cleanup; + + ret = qemuDomainBlockCopyCommon(vm, dom->conn, disk, dest, + bandwidth, flags); + vm = NULL; + + cleanup: + if (vm) + virObjectUnlock(vm); + return ret; +} + + static int qemuDomainBlockPull(virDomainPtr dom, const char *path, unsigned long bandwidth, unsigned int flags) @@ -17611,6 +17694,7 @@ static virDriver qemuDriver = { .domainBlockJobSetSpeed = qemuDomainBlockJobSetSpeed, /* 0.9.4 */ .domainBlockPull = qemuDomainBlockPull, /* 0.9.4 */ .domainBlockRebase = qemuDomainBlockRebase, /* 0.9.10 */ + .domainBlockCopy = qemuDomainBlockCopy, /* 1.2.9 */ .domainBlockCommit = qemuDomainBlockCommit, /* 1.0.0 */ .connectIsAlive = qemuConnectIsAlive, /* 0.9.8 */ .nodeSuspendForDuration = qemuNodeSuspendForDuration, /* 0.9.8 */ -- 1.9.3
On 09/12/14 05:55, Eric Blake wrote:
The hard part of managing the disk copy is already coded; all this had to do was convert the XML and virTypedParameters into the internal representation.
With this patch, all blockcopy operations that used the old API should also work via the new API. Additional extensions, such as supporting the granularity tunable or a network rather than file destination, will be added as later patches.
* src/qemu/qemu_driver.c (qemuDomainBlockCopy): New function.
Signed-off-by: Eric Blake <eblake@redhat.com> --- src/qemu/qemu_driver.c | 84 ++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 84 insertions(+)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 75c529b..6895b23 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -15539,6 +15539,89 @@ qemuDomainBlockRebase(virDomainPtr dom, const char *path, const char *base, return ret; }
+ +static int +qemuDomainBlockCopy(virDomainPtr dom, const char *disk, const char *destxml, + virTypedParameterPtr params, int nparams, + unsigned int flags) +{ + virQEMUDriverPtr driver = dom->conn->privateData; + virDomainObjPtr vm; + int ret = -1; + unsigned long long bandwidth = 0; + unsigned int granularity = 0; + unsigned long long buf_size = 0; + virStorageSourcePtr dest = NULL; + size_t i; + + virCheckFlags(VIR_DOMAIN_BLOCK_COPY_SHALLOW | + VIR_DOMAIN_BLOCK_COPY_REUSE_EXT, -1); + if (virTypedParamsValidate(params, nparams, + VIR_DOMAIN_BLOCK_COPY_BANDWIDTH, + VIR_TYPED_PARAM_ULLONG, + VIR_DOMAIN_BLOCK_COPY_GRANULARITY, + VIR_TYPED_PARAM_UINT, + VIR_DOMAIN_BLOCK_COPY_BUF_SIZE, + VIR_TYPED_PARAM_ULLONG, + NULL) < 0) + return -1; + + if (!(vm = qemuDomObjFromDomain(dom))) + return -1; + + if (virDomainBlockCopyEnsureACL(dom->conn, vm->def) < 0) + goto cleanup; + + for (i = 0; i < nparams; i++) { + virTypedParameterPtr param = ¶ms[i]; + + /* Typed params (wisely) refused to expose unsigned long, but + * back-compat demands that we stick with a maximum of + * unsigned long bandwidth in MiB/s, while our value is + * unsigned long long in bytes/s. Hence, we have to do + * overflow detection if this is a 32-bit server handling a + * 64-bit client. */ + if (STREQ(param->field, VIR_DOMAIN_BLOCK_COPY_BANDWIDTH)) { + if (sizeof(unsigned long) < sizeof(bandwidth) && + param->value.ul > ULONG_MAX * (1ULL << 20)) { + virReportError(VIR_ERR_OVERFLOW, + _("bandwidth must be less than %llu bytes"), + ULONG_MAX * (1ULL << 20)); + goto cleanup; + } + bandwidth = param->value.ul; + } else if (STREQ(param->field, VIR_DOMAIN_BLOCK_COPY_GRANULARITY)) { + granularity = param->value.ui; + } else if (STREQ(param->field, VIR_DOMAIN_BLOCK_COPY_BUF_SIZE)) { + buf_size = param->value.ul; + } + } + if (granularity) { + virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s", + _("granularity tuning not supported yet")); + goto cleanup; + } + if (buf_size) { + virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s", + _("buffer size tuning not supported yet")); + goto cleanup; + } + + if (!(dest = virDomainDiskDefSourceParse(destxml, vm->def, driver->xmlopt, + VIR_DOMAIN_XML_INACTIVE)))
IIRC you added "VIR_DOMAIN_XML_INTERNAL_DISK_SOURCE" so that some of the members of the <disk> structure can be omitted, so I presume you want to pass it here.
+ goto cleanup; + + ret = qemuDomainBlockCopyCommon(vm, dom->conn, disk, dest, + bandwidth, flags); + vm = NULL; + + cleanup: + if (vm) + virObjectUnlock(vm); + return ret; +} + + static int qemuDomainBlockPull(virDomainPtr dom, const char *path, unsigned long bandwidth, unsigned int flags)
ACK if you pass VIR_DOMAIN_XML_INTERNAL_DISK_SOURCE or explain why not appropriately. Peter
On 09/12/2014 05:41 AM, Peter Krempa wrote:
On 09/12/14 05:55, Eric Blake wrote:
The hard part of managing the disk copy is already coded; all this had to do was convert the XML and virTypedParameters into the internal representation.
With this patch, all blockcopy operations that used the old API should also work via the new API. Additional extensions, such as supporting the granularity tunable or a network rather than file destination, will be added as later patches.
* src/qemu/qemu_driver.c (qemuDomainBlockCopy): New function.
Signed-off-by: Eric Blake <eblake@redhat.com> --- src/qemu/qemu_driver.c | 84 ++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 84 insertions(+)
+ if (!(dest = virDomainDiskDefSourceParse(destxml, vm->def, driver->xmlopt, + VIR_DOMAIN_XML_INACTIVE)))
IIRC you added "VIR_DOMAIN_XML_INTERNAL_DISK_SOURCE" so that some of the members of the <disk> structure can be omitted, so I presume you want to pass it here.
No, all the VIR_DOMAIN_XML_INTERNAL_* flags are intentionally private to the domain_conf.c file; you get them not by using them here, but by calling the correct entry point (that is, this code calls virDomainDiskDefSourceParse, and THAT function adds the internal flag before calling into the real workhorse of virDomainDiskDefParseXML).
ACK if you pass VIR_DOMAIN_XML_INTERNAL_DISK_SOURCE or explain why not appropriately.
Assuming my explanation is good enough, I'm pushing 1-4. I'll leave the virsh changes of 5-7 for later in case there is more conversation on how complex or simple the user interface should be (I'll reply more on 7 to get the ball rolling). -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Upstream qemu 1.4 added some drive-mirror tunables not present when it was first introduced in 1.3. Management apps may want to set these in some cases (for example, without tuning granularity down to sector size, a copy may end up occupying more bytes than the original because an entire cluster is copied even when only a sector within the cluster is dirty, although tuning it down results in more CPU time to do the copy). I haven't personally needed to use the parameters, but since they exist, and since the new API supports virTypedParams, we might as well expose them. Since the tuning parameters aren't often used, and omitted from the QMP command when unspecified, I think it is safe to rely on qemu 1.3 to issue an error about them being unsupported, rather than trying to create a new capability bit in libvirt. Meanwhile, all versions of qemu from 1.4 to 2.1 have a bug where a bad granularity (such as non-power-of-2) gives a poor message: error: internal error: unable to execute QEMU command 'drive-mirror': Invalid parameter 'drive-virtio-disk0' because of abuse of QERR_INVALID_PARAMETER (which is supposed to name the parameter that was given a bad value, rather than the value passed to some other parameter). I don't see that a capability check will help, so we'll just live with it (and it has since been improved in upstream qemu). * src/qemu/qemu_monitor.h (qemuMonitorDriveMirror): Add parameters. * src/qemu/qemu_monitor.c (qemuMonitorDriveMirror): Likewise. * src/qemu/qemu_monitor_json.h (qemuMonitorJSONDriveMirror): Likewise. * src/qemu/qemu_monitor_json.c (qemuMonitorJSONDriveMirror): Likewise. * src/qemu/qemu_driver.c (qemuDomainBlockCopyCommon): Likewise. (qemuDomainBlockRebase, qemuDomainBlockCopy): Adjust callers. * src/qemu/qemu_migration.c (qemuMigrationDriveMirror): Likewise. * tests/qemumonitorjsontest.c (qemuMonitorJSONDriveMirror): Likewise. Signed-off-by: Eric Blake <eblake@redhat.com> --- src/qemu/qemu_driver.c | 18 +++++------------- src/qemu/qemu_migration.c | 2 +- src/qemu/qemu_monitor.c | 8 +++++--- src/qemu/qemu_monitor.h | 2 ++ src/qemu/qemu_monitor_json.c | 4 ++++ src/qemu/qemu_monitor_json.h | 2 ++ tests/qemumonitorjsontest.c | 2 +- 7 files changed, 20 insertions(+), 18 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 6895b23..1673c7b 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -15287,6 +15287,8 @@ qemuDomainBlockCopyCommon(virDomainObjPtr vm, const char *path, virStorageSourcePtr mirror, unsigned long long bandwidth, + unsigned int granularity, + unsigned long long buf_size, unsigned int flags) { virQEMUDriverPtr driver = conn->privateData; @@ -15432,7 +15434,7 @@ qemuDomainBlockCopyCommon(virDomainObjPtr vm, /* Actually start the mirroring */ qemuDomainObjEnterMonitor(driver, vm); ret = qemuMonitorDriveMirror(priv->mon, device, mirror->path, format, - bandwidth, flags); + bandwidth, granularity, buf_size, flags); virDomainAuditDisk(vm, NULL, mirror, "mirror", ret >= 0); qemuDomainObjExitMonitor(driver, vm); if (ret < 0) { @@ -15528,7 +15530,7 @@ qemuDomainBlockRebase(virDomainPtr dom, const char *path, const char *base, flags &= (VIR_DOMAIN_BLOCK_REBASE_SHALLOW | VIR_DOMAIN_BLOCK_REBASE_REUSE_EXT); ret = qemuDomainBlockCopyCommon(vm, dom->conn, path, dest, - bandwidth, flags); + bandwidth, 0, 0, flags); vm = NULL; dest = NULL; @@ -15596,23 +15598,13 @@ qemuDomainBlockCopy(virDomainPtr dom, const char *disk, const char *destxml, buf_size = param->value.ul; } } - if (granularity) { - virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s", - _("granularity tuning not supported yet")); - goto cleanup; - } - if (buf_size) { - virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s", - _("buffer size tuning not supported yet")); - goto cleanup; - } if (!(dest = virDomainDiskDefSourceParse(destxml, vm->def, driver->xmlopt, VIR_DOMAIN_XML_INACTIVE))) goto cleanup; ret = qemuDomainBlockCopyCommon(vm, dom->conn, disk, dest, - bandwidth, flags); + bandwidth, granularity, buf_size, flags); vm = NULL; cleanup: diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index 4819c04..ce1a5cd 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -1461,7 +1461,7 @@ qemuMigrationDriveMirror(virQEMUDriverPtr driver, QEMU_ASYNC_JOB_MIGRATION_OUT) < 0) goto error; mon_ret = qemuMonitorDriveMirror(priv->mon, diskAlias, nbd_dest, - NULL, speed, mirror_flags); + NULL, speed, 0, 0, mirror_flags); qemuDomainObjExitMonitor(driver, vm); if (mon_ret < 0) diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c index 702404a..6059133 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -3189,17 +3189,19 @@ int qemuMonitorDriveMirror(qemuMonitorPtr mon, const char *device, const char *file, const char *format, unsigned long long bandwidth, + unsigned int granularity, unsigned long long buf_size, unsigned int flags) { int ret = -1; VIR_DEBUG("mon=%p, device=%s, file=%s, format=%s, bandwidth=%lld, " - "flags=%x", - mon, device, file, NULLSTR(format), bandwidth, flags); + "granularity=%#x, buf_size=%lld, flags=%x", + mon, device, file, NULLSTR(format), bandwidth, granularity, + buf_size, flags); if (mon->json) ret = qemuMonitorJSONDriveMirror(mon, device, file, format, bandwidth, - flags); + granularity, buf_size, flags); else virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s", _("drive-mirror requires JSON monitor")); diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h index ced198e..c32001d 100644 --- a/src/qemu/qemu_monitor.h +++ b/src/qemu/qemu_monitor.h @@ -650,6 +650,8 @@ int qemuMonitorDriveMirror(qemuMonitorPtr mon, const char *file, const char *format, unsigned long long bandwidth, + unsigned int granularity, + unsigned long long buf_size, unsigned int flags) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3); int qemuMonitorDrivePivot(qemuMonitorPtr mon, diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index 30f9ffb..0c4832a 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -3422,6 +3422,8 @@ int qemuMonitorJSONDriveMirror(qemuMonitorPtr mon, const char *device, const char *file, const char *format, unsigned long long speed, + unsigned int granularity, + unsigned long long buf_size, unsigned int flags) { int ret = -1; @@ -3434,6 +3436,8 @@ qemuMonitorJSONDriveMirror(qemuMonitorPtr mon, "s:device", device, "s:target", file, "Y:speed", speed, + "z:granularity", granularity, + "P:buf-size", buf_size, "s:sync", shallow ? "top" : "full", "s:mode", reuse ? "existing" : "absolute-paths", "S:format", format, diff --git a/src/qemu/qemu_monitor_json.h b/src/qemu/qemu_monitor_json.h index a6d05b5..b5c61ca 100644 --- a/src/qemu/qemu_monitor_json.h +++ b/src/qemu/qemu_monitor_json.h @@ -249,6 +249,8 @@ int qemuMonitorJSONDriveMirror(qemuMonitorPtr mon, const char *file, const char *format, unsigned long long speed, + unsigned int granularity, + unsigned long long buf_size, unsigned int flags) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3); int qemuMonitorJSONDrivePivot(qemuMonitorPtr mon, diff --git a/tests/qemumonitorjsontest.c b/tests/qemumonitorjsontest.c index e3fb4f7..afbf13a 100644 --- a/tests/qemumonitorjsontest.c +++ b/tests/qemumonitorjsontest.c @@ -1176,7 +1176,7 @@ GEN_TEST_FUNC(qemuMonitorJSONRemoveNetdev, "net0") GEN_TEST_FUNC(qemuMonitorJSONDelDevice, "ide0") GEN_TEST_FUNC(qemuMonitorJSONAddDevice, "some_dummy_devicestr") GEN_TEST_FUNC(qemuMonitorJSONSetDrivePassphrase, "vda", "secret_passhprase") -GEN_TEST_FUNC(qemuMonitorJSONDriveMirror, "vdb", "/foo/bar", NULL, 1024, +GEN_TEST_FUNC(qemuMonitorJSONDriveMirror, "vdb", "/foo/bar", NULL, 1024, 0, 0, VIR_DOMAIN_BLOCK_REBASE_SHALLOW | VIR_DOMAIN_BLOCK_REBASE_REUSE_EXT) GEN_TEST_FUNC(qemuMonitorJSONBlockCommit, "vdb", "/foo/bar1", "/foo/bar2", NULL, 1024) GEN_TEST_FUNC(qemuMonitorJSONDrivePivot, "vdb", NULL, NULL) -- 1.9.3
On 09/12/14 05:55, Eric Blake wrote:
Upstream qemu 1.4 added some drive-mirror tunables not present when it was first introduced in 1.3. Management apps may want to set these in some cases (for example, without tuning granularity down to sector size, a copy may end up occupying more bytes than the original because an entire cluster is copied even when only a sector within the cluster is dirty, although tuning it down results in more CPU time to do the copy). I haven't personally needed to use the parameters, but since they exist, and since the new API supports virTypedParams, we might as well expose them.
Since the tuning parameters aren't often used, and omitted from the QMP command when unspecified, I think it is safe to rely on qemu 1.3 to issue an error about them being unsupported, rather than trying to create a new capability bit in libvirt.
Meanwhile, all versions of qemu from 1.4 to 2.1 have a bug where a bad granularity (such as non-power-of-2) gives a poor message: error: internal error: unable to execute QEMU command 'drive-mirror': Invalid parameter 'drive-virtio-disk0'
because of abuse of QERR_INVALID_PARAMETER (which is supposed to name the parameter that was given a bad value, rather than the value passed to some other parameter). I don't see that a capability check will help, so we'll just live with it (and it has since been improved in upstream qemu).
* src/qemu/qemu_monitor.h (qemuMonitorDriveMirror): Add parameters. * src/qemu/qemu_monitor.c (qemuMonitorDriveMirror): Likewise. * src/qemu/qemu_monitor_json.h (qemuMonitorJSONDriveMirror): Likewise. * src/qemu/qemu_monitor_json.c (qemuMonitorJSONDriveMirror): Likewise. * src/qemu/qemu_driver.c (qemuDomainBlockCopyCommon): Likewise. (qemuDomainBlockRebase, qemuDomainBlockCopy): Adjust callers. * src/qemu/qemu_migration.c (qemuMigrationDriveMirror): Likewise. * tests/qemumonitorjsontest.c (qemuMonitorJSONDriveMirror): Likewise.
Signed-off-by: Eric Blake <eblake@redhat.com> --- src/qemu/qemu_driver.c | 18 +++++------------- src/qemu/qemu_migration.c | 2 +- src/qemu/qemu_monitor.c | 8 +++++--- src/qemu/qemu_monitor.h | 2 ++ src/qemu/qemu_monitor_json.c | 4 ++++ src/qemu/qemu_monitor_json.h | 2 ++ tests/qemumonitorjsontest.c | 2 +- 7 files changed, 20 insertions(+), 18 deletions(-)
ACK, Peter
We stupidly modeled block job bandwidth after migration bandwidth, which in turn was an 'unsigned long' and therefore subject to 32-bit vs. 64-bit interpretations. To work around the fact that 10-gigabit interfaces are possible but don't fit within 32 bits, the original interface took the number scaled as MiB/sec. But this scaling is rather coarse, and it might be nice to tune bandwidth finer than in megabyte chunks. Several of the block job calls that can set speed are fed through a common interface, so it was easier to adjust them all at once. Note that there is intentionally no flag for the new virDomainBlockCopy; there, since the API already uses a 64-bit type always, instead of a possible 32-bit type, and is brand new, it was easier to just avoid scaling issues. As with the previous patch that adjusted the query side (commit db33cc24), omitting the new flag preserves old behavior, and the documentation now mentions limits of what happens when a 32-bit machine is on either client or server side. * include/libvirt/libvirt.h.in (virDomainBlockJobSetSpeedFlags) (virDomainBlockPullFlags) (VIR_DOMAIN_BLOCK_REBASE_BANDWIDTH_BYTES) (VIR_DOMAIN_BLOCK_COMMIT_BANDWIDTH_BYTES): New enums. * src/libvirt.c (virDomainBlockJobSetSpeed, virDomainBlockPull) (virDomainBlockRebase, virDomainBlockCommit): Document them. * src/qemu/qemu_driver.c (qemuDomainBlockJobSetSpeed) (qemuDomainBlockPull, qemuDomainBlockRebase) (qemuDomainBlockCommit, qemuDomainBlockJobImpl): Support new flag. Signed-off-by: Eric Blake <eblake@redhat.com> --- include/libvirt/libvirt.h.in | 31 +++++++++++++---- src/libvirt.c | 83 +++++++++++++++++++++++++++++++------------- src/qemu/qemu_driver.c | 61 +++++++++++++++++++------------- 3 files changed, 118 insertions(+), 57 deletions(-) diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in index 94b942c..729bc82 100644 --- a/include/libvirt/libvirt.h.in +++ b/include/libvirt/libvirt.h.in @@ -2617,11 +2617,24 @@ int virDomainGetBlockJobInfo(virDomainPtr dom, const char *disk, virDomainBlockJobInfoPtr info, unsigned int flags); -int virDomainBlockJobSetSpeed(virDomainPtr dom, const char *disk, - unsigned long bandwidth, unsigned int flags); +/* Flags for use with virDomainBlockJobSetSpeed */ +typedef enum { + VIR_DOMAIN_BLOCK_JOB_SPEED_BANDWIDTH_BYTES = 1 << 0, /* bandwidth in bytes/s + instead of MiB/s */ +} virDomainBlockJobSetSpeedFlags; -int virDomainBlockPull(virDomainPtr dom, const char *disk, - unsigned long bandwidth, unsigned int flags); +int virDomainBlockJobSetSpeed(virDomainPtr dom, const char *disk, + unsigned long bandwidth, unsigned int flags); + +/* Flags for use with virDomainBlockPull (values chosen to be a subset + * of the flags for virDomainBlockRebase) */ +typedef enum { + VIR_DOMAIN_BLOCK_PULL_BANDWIDTH_BYTES = 1 << 6, /* bandwidth in bytes/s + instead of MiB/s */ +} virDomainBlockPullFlags; + +int virDomainBlockPull(virDomainPtr dom, const char *disk, + unsigned long bandwidth, unsigned int flags); /** * virDomainBlockRebaseFlags: @@ -2640,11 +2653,13 @@ typedef enum { names */ VIR_DOMAIN_BLOCK_REBASE_COPY_DEV = 1 << 5, /* Treat destination as block device instead of file */ + VIR_DOMAIN_BLOCK_REBASE_BANDWIDTH_BYTES = 1 << 6, /* bandwidth in bytes/s + instead of MiB/s */ } virDomainBlockRebaseFlags; -int virDomainBlockRebase(virDomainPtr dom, const char *disk, - const char *base, unsigned long bandwidth, - unsigned int flags); +int virDomainBlockRebase(virDomainPtr dom, const char *disk, + const char *base, unsigned long bandwidth, + unsigned int flags); /** * virDomainBlockCopyFlags: @@ -2719,6 +2734,8 @@ typedef enum { VIR_DOMAIN_BLOCK_COMMIT_RELATIVE = 1 << 3, /* keep the backing chain referenced using relative names */ + VIR_DOMAIN_BLOCK_COMMIT_BANDWIDTH_BYTES = 1 << 4, /* bandwidth in bytes/s + instead of MiB/s */ } virDomainBlockCommitFlags; int virDomainBlockCommit(virDomainPtr dom, const char *disk, const char *base, diff --git a/src/libvirt.c b/src/libvirt.c index 941c518..f7e5a37 100644 --- a/src/libvirt.c +++ b/src/libvirt.c @@ -19733,11 +19733,20 @@ virDomainGetBlockJobInfo(virDomainPtr dom, const char *disk, * virDomainBlockJobSetSpeed: * @dom: pointer to domain object * @disk: path to the block device, or device shorthand - * @bandwidth: specify bandwidth limit in MiB/s - * @flags: extra flags; not used yet, so callers should always pass 0 + * @bandwidth: specify bandwidth limit; flags determine the unit + * @flags: bitwise-OR of virDomainBlockJobSetSpeedFlags * * Set the maximimum allowable bandwidth that a block job may consume. If - * bandwidth is 0, the limit will revert to the hypervisor default. + * bandwidth is 0, the limit will revert to the hypervisor default of + * unlimited. + * + * If @flags contains VIR_DOMAIN_BLOCK_JOB_SPEED_BANDWIDTH_BYTES, @bandwidth + * is in bytes/second; otherwise, it is in MiB/second. Values larger than + * 2^52 bytes/sec may be rejected due to overflow considerations based on + * the word size of both client and server, and values larger than 2^31 + * bytes/sec may cause overflow problems if later queried by + * virDomainGetBlockJobInfo() without scaling. Hypervisors may further + * restrict the range of valid bandwidth values. * * The @disk parameter is either an unambiguous source name of the * block device (the <source file='...'/> sub-element, such as @@ -19785,8 +19794,8 @@ virDomainBlockJobSetSpeed(virDomainPtr dom, const char *disk, * virDomainBlockPull: * @dom: pointer to domain object * @disk: path to the block device, or device shorthand - * @bandwidth: (optional) specify copy bandwidth limit in MiB/s - * @flags: extra flags; not used yet, so callers should always pass 0 + * @bandwidth: (optional) specify bandwidth limit; flags determine the unit + * @flags: bitwise-OR of virDomainBlockPullFlags * * Populate a disk image with data from its backing image. Once all data from * its backing image has been pulled, the disk no longer depends on a backing @@ -19803,12 +19812,20 @@ virDomainBlockJobSetSpeed(virDomainPtr dom, const char *disk, * can be found by calling virDomainGetXMLDesc() and inspecting * elements within //domain/devices/disk. * - * The maximum bandwidth (in MiB/s) that will be used to do the copy can be - * specified with the bandwidth parameter. If set to 0, libvirt will choose a - * suitable default. Some hypervisors do not support this feature and will - * return an error if bandwidth is not 0; in this case, it might still be - * possible for a later call to virDomainBlockJobSetSpeed() to succeed. - * The actual speed can be determined with virDomainGetBlockJobInfo(). + * The maximum bandwidth that will be used to do the copy can be + * specified with the @bandwidth parameter. If set to 0, there is no + * limit. If @flags includes VIR_DOMAIN_BLOCK_PULL_BANDWIDTH_BYTES, + * @bandwidth is in bytes/second; otherwise, it is in MiB/second. + * Values larger than 2^52 bytes/sec may be rejected due to overflow + * considerations based on the word size of both client and server, + * and values larger than 2^31 bytes/sec may cause overflow problems + * if later queried by virDomainGetBlockJobInfo() without scaling. + * Hypervisors may further restrict the range of valid bandwidth + * values. Some hypervisors do not support this feature and will + * return an error if bandwidth is not 0; in this case, it might still + * be possible for a later call to virDomainBlockJobSetSpeed() to + * succeed. The actual speed can be determined with + * virDomainGetBlockJobInfo(). * * This is shorthand for virDomainBlockRebase() with a NULL base. * @@ -19853,7 +19870,7 @@ virDomainBlockPull(virDomainPtr dom, const char *disk, * @disk: path to the block device, or device shorthand * @base: path to backing file to keep, or device shorthand, * or NULL for no backing file - * @bandwidth: (optional) specify copy bandwidth limit in MiB/s + * @bandwidth: (optional) specify bandwidth limit; flags determine the unit * @flags: bitwise-OR of virDomainBlockRebaseFlags * * Populate a disk image with data from its backing image chain, and @@ -19932,12 +19949,20 @@ virDomainBlockPull(virDomainPtr dom, const char *disk, * example, "vda[3]" refers to the backing store with index equal to "3" * in the chain of disk "vda". * - * The maximum bandwidth (in MiB/s) that will be used to do the copy can be - * specified with the bandwidth parameter. If set to 0, libvirt will choose a - * suitable default. Some hypervisors do not support this feature and will - * return an error if bandwidth is not 0; in this case, it might still be - * possible for a later call to virDomainBlockJobSetSpeed() to succeed. - * The actual speed can be determined with virDomainGetBlockJobInfo(). + * The maximum bandwidth that will be used to do the copy can be + * specified with the @bandwidth parameter. If set to 0, there is no + * limit. If @flags includes VIR_DOMAIN_BLOCK_REBASE_BANDWIDTH_BYTES, + * @bandwidth is in bytes/second; otherwise, it is in MiB/second. + * Values larger than 2^52 bytes/sec may be rejected due to overflow + * considerations based on the word size of both client and server, + * and values larger than 2^31 bytes/sec may cause overflow problems + * if later queried by virDomainGetBlockJobInfo() without scaling. + * Hypervisors may further restrict the range of valid bandwidth + * values. Some hypervisors do not support this feature and will + * return an error if bandwidth is not 0; in this case, it might still + * be possible for a later call to virDomainBlockJobSetSpeed() to + * succeed. The actual speed can be determined with + * virDomainGetBlockJobInfo(). * * When @base is NULL and @flags is 0, this is identical to * virDomainBlockPull(). When @flags contains VIR_DOMAIN_BLOCK_REBASE_COPY, @@ -20119,7 +20144,7 @@ virDomainBlockCopy(virDomainPtr dom, const char *disk, * or NULL for default * @top: path to file within backing chain that contains data to be merged, * or device shorthand, or NULL to merge all possible data - * @bandwidth: (optional) specify commit bandwidth limit in MiB/s + * @bandwidth: (optional) specify bandwidth limit; flags determine the unit * @flags: bitwise-OR of virDomainBlockCommitFlags * * Commit changes that were made to temporary top-level files within a disk @@ -20197,12 +20222,20 @@ virDomainBlockCopy(virDomainPtr dom, const char *disk, * example, "vda[3]" refers to the backing store with index equal to "3" * in the chain of disk "vda". * - * The maximum bandwidth (in MiB/s) that will be used to do the commit can be - * specified with the bandwidth parameter. If set to 0, libvirt will choose a - * suitable default. Some hypervisors do not support this feature and will - * return an error if bandwidth is not 0; in this case, it might still be - * possible for a later call to virDomainBlockJobSetSpeed() to succeed. - * The actual speed can be determined with virDomainGetBlockJobInfo(). + * The maximum bandwidth that will be used to do the commit can be + * specified with the @bandwidth parameter. If set to 0, there is no + * limit. If @flags includes VIR_DOMAIN_BLOCK_COMMIT_BANDWIDTH_BYTES, + * @bandwidth is in bytes/second; otherwise, it is in MiB/second. + * Values larger than 2^52 bytes/sec may be rejected due to overflow + * considerations based on the word size of both client and server, + * and values larger than 2^31 bytes/sec may cause overflow problems + * if later queried by virDomainGetBlockJobInfo() without scaling. + * Hypervisors may further restrict the range of valid bandwidth + * values. Some hypervisors do not support this feature and will + * return an error if bandwidth is not 0; in this case, it might still + * be possible for a later call to virDomainBlockJobSetSpeed() to + * succeed. The actual speed can be determined with + * virDomainGetBlockJobInfo(). * * Returns 0 if the operation has started, -1 on failure. */ diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 1673c7b..92869c0 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -15052,14 +15052,19 @@ qemuDomainBlockJobImpl(virDomainObjPtr vm, } } - /* Convert bandwidth MiB to bytes */ - if (speed > LLONG_MAX >> 20) { - virReportError(VIR_ERR_OVERFLOW, - _("bandwidth must be less than %llu"), - LLONG_MAX >> 20); - goto endjob; + /* Convert bandwidth MiB to bytes, if needed */ + if ((mode == BLOCK_JOB_SPEED && + !(flags & VIR_DOMAIN_BLOCK_JOB_SPEED_BANDWIDTH_BYTES)) || + (mode == BLOCK_JOB_PULL && + !(flags & VIR_DOMAIN_BLOCK_PULL_BANDWIDTH_BYTES))) { + if (speed > LLONG_MAX >> 20) { + virReportError(VIR_ERR_OVERFLOW, + _("bandwidth must be less than %llu"), + LLONG_MAX >> 20); + goto endjob; + } + speed <<= 20; } - speed <<= 20; qemuDomainObjEnterMonitor(driver, vm); ret = qemuMonitorBlockJob(priv->mon, device, basePath, backingPath, @@ -15263,7 +15268,7 @@ qemuDomainBlockJobSetSpeed(virDomainPtr dom, const char *path, unsigned long bandwidth, unsigned int flags) { virDomainObjPtr vm; - virCheckFlags(0, -1); + virCheckFlags(VIR_DOMAIN_BLOCK_JOB_SPEED_BANDWIDTH_BYTES, -1); if (!(vm = qemuDomObjFromDomain(dom))) return -1; @@ -15482,7 +15487,8 @@ qemuDomainBlockRebase(virDomainPtr dom, const char *path, const char *base, VIR_DOMAIN_BLOCK_REBASE_COPY | VIR_DOMAIN_BLOCK_REBASE_COPY_RAW | VIR_DOMAIN_BLOCK_REBASE_RELATIVE | - VIR_DOMAIN_BLOCK_REBASE_COPY_DEV, -1); + VIR_DOMAIN_BLOCK_REBASE_COPY_DEV | + VIR_DOMAIN_BLOCK_REBASE_BANDWIDTH_BYTES, -1); if (!(vm = qemuDomObjFromDomain(dom))) return -1; @@ -15506,14 +15512,16 @@ qemuDomainBlockRebase(virDomainPtr dom, const char *path, const char *base, if (flags & VIR_DOMAIN_BLOCK_REBASE_COPY_RAW) dest->format = VIR_STORAGE_FILE_RAW; - /* Convert bandwidth MiB to bytes */ - if (speed > LLONG_MAX >> 20) { - virReportError(VIR_ERR_OVERFLOW, - _("bandwidth must be less than %llu"), - LLONG_MAX >> 20); - goto cleanup; + /* Convert bandwidth MiB to bytes, if necessary */ + if (!(flags & VIR_DOMAIN_BLOCK_REBASE_BANDWIDTH_BYTES)) { + if (speed > LLONG_MAX >> 20) { + virReportError(VIR_ERR_OVERFLOW, + _("bandwidth must be less than %llu"), + LLONG_MAX >> 20); + goto cleanup; + } + speed <<= 20; } - speed <<= 20; /* XXX: If we are doing a shallow copy but not reusing an external * file, we should attempt to pre-create the destination with a @@ -15619,7 +15627,7 @@ qemuDomainBlockPull(virDomainPtr dom, const char *path, unsigned long bandwidth, unsigned int flags) { virDomainObjPtr vm; - virCheckFlags(0, -1); + virCheckFlags(VIR_DOMAIN_BLOCK_PULL_BANDWIDTH_BYTES, -1); if (!(vm = qemuDomObjFromDomain(dom))) return -1; @@ -15664,7 +15672,8 @@ qemuDomainBlockCommit(virDomainPtr dom, /* XXX Add support for COMMIT_DELETE */ virCheckFlags(VIR_DOMAIN_BLOCK_COMMIT_SHALLOW | VIR_DOMAIN_BLOCK_COMMIT_ACTIVE | - VIR_DOMAIN_BLOCK_COMMIT_RELATIVE, -1); + VIR_DOMAIN_BLOCK_COMMIT_RELATIVE | + VIR_DOMAIN_BLOCK_COMMIT_BANDWIDTH_BYTES, -1); if (!(vm = qemuDomObjFromDomain(dom))) goto cleanup; @@ -15690,14 +15699,16 @@ qemuDomainBlockCommit(virDomainPtr dom, goto endjob; } - /* Convert bandwidth MiB to bytes */ - if (speed > LLONG_MAX >> 20) { - virReportError(VIR_ERR_OVERFLOW, - _("bandwidth must be less than %llu"), - LLONG_MAX >> 20); - goto endjob; + /* Convert bandwidth MiB to bytes, if necessary */ + if (!(flags & VIR_DOMAIN_BLOCK_COMMIT_BANDWIDTH_BYTES)) { + if (speed > LLONG_MAX >> 20) { + virReportError(VIR_ERR_OVERFLOW, + _("bandwidth must be less than %llu"), + LLONG_MAX >> 20); + goto endjob; + } + speed <<= 20; } - speed <<= 20; device = qemuDiskPathToAlias(vm, path, &idx); if (!device) -- 1.9.3
On 09/12/14 05:55, Eric Blake wrote:
We stupidly modeled block job bandwidth after migration bandwidth, which in turn was an 'unsigned long' and therefore subject to 32-bit vs. 64-bit interpretations. To work around the fact that 10-gigabit interfaces are possible but don't fit within 32 bits, the original interface took the number scaled as MiB/sec. But this scaling is rather coarse, and it might be nice to tune bandwidth finer than in megabyte chunks.
Several of the block job calls that can set speed are fed through a common interface, so it was easier to adjust them all at once. Note that there is intentionally no flag for the new virDomainBlockCopy; there, since the API already uses a 64-bit type always, instead of a possible 32-bit type, and is brand new, it was easier to just avoid scaling issues. As with the previous patch that adjusted the query side (commit db33cc24), omitting the new flag preserves old behavior, and the documentation now mentions limits of what happens when a 32-bit machine is on either client or server side.
* include/libvirt/libvirt.h.in (virDomainBlockJobSetSpeedFlags) (virDomainBlockPullFlags) (VIR_DOMAIN_BLOCK_REBASE_BANDWIDTH_BYTES) (VIR_DOMAIN_BLOCK_COMMIT_BANDWIDTH_BYTES): New enums. * src/libvirt.c (virDomainBlockJobSetSpeed, virDomainBlockPull) (virDomainBlockRebase, virDomainBlockCommit): Document them. * src/qemu/qemu_driver.c (qemuDomainBlockJobSetSpeed) (qemuDomainBlockPull, qemuDomainBlockRebase) (qemuDomainBlockCommit, qemuDomainBlockJobImpl): Support new flag.
Signed-off-by: Eric Blake <eblake@redhat.com> --- include/libvirt/libvirt.h.in | 31 +++++++++++++---- src/libvirt.c | 83 +++++++++++++++++++++++++++++++------------- src/qemu/qemu_driver.c | 61 +++++++++++++++++++------------- 3 files changed, 118 insertions(+), 57 deletions(-)
ACK, Peter
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 @@ -1460,6 +1460,9 @@ cmdBlkiotune(vshControl * ctl, const vshCmd * cmd) goto cleanup; } + +/* Code common between blockpull, blockcommit, blockcopy, blockjob */ + typedef enum { VSH_CMD_BLOCK_JOB_ABORT, VSH_CMD_BLOCK_JOB_SPEED, @@ -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) @@ -1485,10 +1538,8 @@ blockJobImpl(vshControl *ctl, const vshCmd *cmd, if (vshCommandOptStringReq(ctl, cmd, "path", &path) < 0) goto cleanup; - if (vshCommandOptULWrap(cmd, "bandwidth", &bandwidth) < 0) { - vshError(ctl, "%s", _("bandwidth must be a number")); + if (blockJobBandwidth(ctl, cmd, &bandwidth) < 0) goto cleanup; - } switch (mode) { case VSH_CMD_BLOCK_JOB_ABORT: @@ -1610,7 +1661,7 @@ static const vshCmdOptDef opts_block_commit[] = { }, {.name = "bandwidth", .type = VSH_OT_DATA, - .help = N_("bandwidth limit in MiB/s") + .help = N_("bandwidth limit, as scaled integer (default MiB/s)") }, {.name = "base", .type = VSH_OT_DATA, @@ -1826,7 +1877,7 @@ static const vshCmdOptDef opts_block_copy[] = { }, {.name = "bandwidth", .type = VSH_OT_DATA, - .help = N_("bandwidth limit in MiB/s") + .help = N_("bandwidth limit, as scaled integer (default MiB/s)") }, {.name = "shallow", .type = VSH_OT_BOOL, @@ -1959,14 +2010,8 @@ cmdBlockCopy(vshControl *ctl, const vshCmd *cmd) if (!(dom = vshCommandOptDomain(ctl, cmd, NULL))) goto cleanup; - /* XXX: Parse bandwidth as scaled input, rather than forcing - * MiB/s, and either reject negative input or treat it as 0 rather - * than trying to guess which value will work well across both - * APIs with their different sizes and scales. */ - if (vshCommandOptULWrap(cmd, "bandwidth", &bandwidth) < 0) { - vshError(ctl, "%s", _("bandwidth must be a number")); + if (blockJobBandwidth(ctl, cmd, &bandwidth) < 0) goto cleanup; - } if (vshCommandOptUInt(cmd, "granularity", &granularity) < 0) { vshError(ctl, "%s", _("granularity must be a number")); goto cleanup; @@ -2182,7 +2227,7 @@ static const vshCmdOptDef opts_block_job[] = { }, {.name = "bandwidth", .type = VSH_OT_DATA, - .help = N_("set the Bandwidth limit in MiB/s") + .help = N_("bandwidth limit, as scaled integer (default MiB/s)") }, {.name = NULL} }; @@ -2341,7 +2386,7 @@ static const vshCmdOptDef opts_block_pull[] = { }, {.name = "bandwidth", .type = VSH_OT_DATA, - .help = N_("bandwidth limit in MiB/s") + .help = N_("bandwidth limit, as scaled integer (default MiB/s)") }, {.name = "base", .type = VSH_OT_DATA, diff --git a/tools/virsh.pod b/tools/virsh.pod index 60ee515..7aba571 100644 --- a/tools/virsh.pod +++ b/tools/virsh.pod @@ -892,11 +892,12 @@ I<path> specifies fully-qualified path of the disk; it corresponds to a unique target name (<target dev='name'/>) or source file (<source file='name'/>) for one of the disk devices attached to I<domain> (see also B<domblklist> for listing these names). -I<bandwidth> specifies copying bandwidth limit in MiB/s, although for -qemu, it may be non-zero only for an online domain. Specifying a negative -value is interpreted as an unsigned long long value or essentially -unlimited. The hypervisor can choose whether to reject the value or -convert it to the maximum value allowed. +I<bandwidth> is a scaled integer (see B<NOTES> above) which defaults to +MiB/s if there is no suffix. It can be used to set a bandwidth limit for +the commit job. For backward compatibility, a negative value is treated +the same as 0 (unlimited); the actual upper limit accepted by the command +may depend on the word size of both client and server, or by further limits +of the hypervisor. =item B<blockcopy> I<domain> I<path> { I<dest> [I<format>] [I<--blockdev>] | I<xml> } [I<--shallow>] [I<--reuse-external>] [I<bandwidth>] @@ -943,10 +944,13 @@ as fast as possible, otherwise the command may continue to block a little while longer until the job has actually cancelled. I<path> specifies fully-qualified path of the disk. -I<bandwidth> specifies copying bandwidth limit in MiB/s. Specifying a negative -value is interpreted as an unsigned long long value that might be essentially -unlimited, but more likely would overflow; it is safer to use 0 for that -purpose. Specifying I<granularity> allows fine-tuning of the granularity that +I<bandwidth> is a scaled integer (see B<NOTES> above) which defaults to +MiB/s if there is no suffix. It can be used to set a bandwidth limit for +the copy job. For backward compatibility, a negative value is treated +the same as 0 (unlimited); the actual upper limit accepted by the command +may depend on the word size of both client and server, or by further limits +of the hypervisor. +Specifying I<granularity> allows fine-tuning of the granularity that will be copied when a dirty region is detected; larger values trigger less I/O overhead but may end up copying more data overall (the default value is usually correct); this value must be a power of two. Specifying I<buf-size> @@ -983,10 +987,12 @@ I<path> specifies fully-qualified path of the disk; it corresponds to a unique target name (<target dev='name'/>) or source file (<source file='name'/>) for one of the disk devices attached to I<domain> (see also B<domblklist> for listing these names). -I<bandwidth> specifies copying bandwidth limit in MiB/s. Specifying a negative -value is interpreted as an unsigned long long value or essentially -unlimited. The hypervisor can choose whether to reject the value or -convert it to the maximum value allowed. +I<bandwidth> is a scaled integer (see B<NOTES> above) which defaults to +MiB/s if there is no suffix. It can be used to set a bandwidth limit for +the pull job. For backward compatibility, a negative value is treated +the same as 0 (unlimited); the actual upper limit accepted by the command +may depend on the word size of both client and server, or by further limits +of the hypervisor. =item B<blkdeviotune> I<domain> I<device> [[I<--config>] [I<--live>] | [I<--current>]] @@ -1050,10 +1056,12 @@ 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 -value or essentially unlimited. The hypervisor can choose whether to -reject the value or convert it to the maximum value allowed. +I<bandwidth> is a scaled integer (see B<NOTES> above) which defaults to +MiB/s if there is no suffix. It can be used to set bandwidth limit for +the active job. For backward compatibility, a negative value is treated +the same as 0 (unlimited); the actual upper limit accepted by the command +may depend on the word size of both client and server, or by further limits +of the hypervisor. =item B<blockresize> I<domain> I<path> I<size> -- 1.9.3
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
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@redhat.com> --- 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(¶ms[nparams++], - VIR_DOMAIN_BLOCK_COPY_BANDWIDTH, - VIR_TYPED_PARAM_ULLONG, - bandwidth << 20ULL) < 0) - goto cleanup; - } + if (bandwidthBytes && + virTypedParameterAssign(¶ms[nparams++], + VIR_DOMAIN_BLOCK_COPY_BANDWIDTH, + VIR_TYPED_PARAM_ULLONG, + bandwidthBytes) < 0) + goto cleanup; if (granularity && virTypedParameterAssign(¶ms[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
On 09/12/14 05:55, Eric Blake wrote:
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@redhat.com> --- tools/virsh-domain.c | 154 +++++++++++++++++++++++++++++++++++++-------------- 1 file changed, 113 insertions(+), 41 deletions(-)
Uff this logic is fairly complex, but looks functional to me. ACK, Peter
Take the previous patch one step further. In addition to automatically selecting byte mode with fallback if the user's result would be rounded, we also want to give the user a way to guarantee byte mode even if rounding is not required, with no fallback if the remote side doesn't support byte mode. This way, virsh can be used to more fully test the impact of setting the bytes flag. * tools/virsh-domain.c (blockJobBandwidth): Adjust return type and check for --bytes flag. (cmdBlockCommit, cmdBlockPull, cmdBlockJob): Add --bytes flag. (cmdBlockCopy): Likewise, and adjust caller. (blockJobImpl): Adjust caller. * tools/virsh.pod (blockcommit, blockcopy, blockpull, blockjob): Document this. Signed-off-by: Eric Blake <eblake@redhat.com> --- tools/virsh-domain.c | 57 ++++++++++++++++++++++++++++++++++------------------ tools/virsh.pod | 48 +++++++++++++++++++++---------------------- 2 files changed, 61 insertions(+), 44 deletions(-) diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index 14d6921..c6ed90e 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -1474,14 +1474,16 @@ typedef enum { /* 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. */ + * to non-zero MiB/s, 2 if *bandwidth or *bandwidthBytes is non-zero + * bytes/s but fallback to MiB/s is okay, and 3 if *bandwidth or + * *bandwidthBytes is non-zero bytes/s and fallback is not okay. */ static int blockJobBandwidth(vshControl *ctl, const vshCmd *cmd, unsigned long *bandwidth, unsigned long long *bandwidthBytes) { const char *value = NULL; int ret = vshCommandOptString(cmd, "bandwidth", &value); + bool bytes = vshCommandOptBool(cmd, "bytes"); unsigned long long speed; unsigned long long limit = ULLONG_MAX; @@ -1489,7 +1491,7 @@ blockJobBandwidth(vshControl *ctl, const vshCmd *cmd, *bandwidth = 0; bandwidthBytes = &speed; if (sizeof(*bandwidth) < sizeof(speed)) - limit = ULONG_MAX << 20ULL; + limit = ULONG_MAX << (bytes ? 0 : 20ULL); } else { *bandwidthBytes = 0; } @@ -1515,20 +1517,23 @@ 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", bandwidthBytes, 1024 * 1024, - limit) < 0) { + if (vshCommandOptScaledInt(cmd, "bandwidth", bandwidthBytes, + bytes ? 1 : 1024 * 1024, limit) < 0) { vshError(ctl, _("could not parse bandwidth '%s'"), value); return -1; } if (!bandwidth) /* bandwidthBytes is already correct */ - return 2; + return 3; - /* 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) { + /* If the caller did not force bytes, the number fits in unsigned + * long, the number 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 (bytes) { + ret = 3; + } else if ((unsigned long) speed == speed && speed & (1024 * 1024 - 1) && + !ctl->blockJobNoBytes) { ret = 2; } else { speed = VIR_DIV_UP(speed, 1024 * 1024); @@ -1577,7 +1582,7 @@ blockJobImpl(vshControl *ctl, const vshCmd *cmd, if (bandwidthScale > 1) flags |= VIR_DOMAIN_BLOCK_JOB_SPEED_BANDWIDTH_BYTES; rc = virDomainBlockJobSetSpeed(dom, path, bandwidth, flags); - if (rc < 0 && bandwidthScale > 1 && + if (rc < 0 && bandwidthScale == 2 && last_error->code == VIR_ERR_INVALID_ARG) { /* try again with MiB/s */ ctl->blockJobNoBytes = true; @@ -1602,7 +1607,7 @@ blockJobImpl(vshControl *ctl, const vshCmd *cmd, else rc = virDomainBlockPull(dom, path, bandwidth, flags); - if (rc < 0 && bandwidthScale > 1 && + if (rc < 0 && bandwidthScale == 2 && last_error->code == VIR_ERR_INVALID_ARG) { /* try again with MiB/s */ ctl->blockJobNoBytes = true; @@ -1634,7 +1639,7 @@ blockJobImpl(vshControl *ctl, const vshCmd *cmd, if (bandwidthScale > 1) flags |= VIR_DOMAIN_BLOCK_COMMIT_BANDWIDTH_BYTES; rc = virDomainBlockCommit(dom, path, base, top, bandwidth, flags); - if (rc < 0 && bandwidthScale > 1 && + if (rc < 0 && bandwidthScale == 2 && last_error->code == VIR_ERR_INVALID_ARG) { /* try again with MiB/s */ ctl->blockJobNoBytes = true; @@ -1723,6 +1728,10 @@ static const vshCmdOptDef opts_block_commit[] = { .type = VSH_OT_DATA, .help = N_("bandwidth limit, as scaled integer (default MiB/s)") }, + {.name = "bytes", + .type = VSH_OT_BOOL, + .help = N_("treat bandwidth as bytes/s") + }, {.name = "base", .type = VSH_OT_DATA, .help = N_("path of base file to commit into (default bottom of chain)") @@ -1939,6 +1948,10 @@ static const vshCmdOptDef opts_block_copy[] = { .type = VSH_OT_DATA, .help = N_("bandwidth limit, as scaled integer (default MiB/s)") }, + {.name = "bytes", + .type = VSH_OT_BOOL, + .help = N_("treat bandwidth as bytes/s") + }, {.name = "shallow", .type = VSH_OT_BOOL, .help = N_("make the copy share a backing chain") @@ -2158,7 +2171,7 @@ cmdBlockCopy(vshControl *ctl, const vshCmd *cmd) flags |= VIR_DOMAIN_BLOCK_REBASE_BANDWIDTH_BYTES; rc = virDomainBlockRebase(dom, path, dest, bandwidth, flags); - if (rc < 0 && bandwidthScale > 1 && + if (rc < 0 && bandwidthScale == 2 && last_error->code == VIR_ERR_INVALID_ARG) { /* try again with MiB/s */ ctl->blockJobNoBytes = true; @@ -2291,7 +2304,8 @@ static const vshCmdOptDef opts_block_job[] = { }, {.name = "bytes", .type = VSH_OT_BOOL, - .help = N_("with --info, get bandwidth in bytes rather than MiB/s") + .help = N_("with --info or --bandwidth, treat bandwidth in bytes " + "rather than MiB/s") }, {.name = "raw", .type = VSH_OT_BOOL, @@ -2343,9 +2357,8 @@ cmdBlockJob(vshControl *ctl, const vshCmd *cmd) _("conflict between abort, info, and bandwidth modes")); return false; } - /* XXX also support --bytes with bandwidth mode */ - if (bytes && (abortMode || bandwidth)) { - vshError(ctl, "%s", _("--bytes requires info mode")); + if (bytes && abortMode) { + vshError(ctl, "%s", _("--bytes not compatible with abort mode")); return false; } @@ -2460,6 +2473,10 @@ static const vshCmdOptDef opts_block_pull[] = { .type = VSH_OT_DATA, .help = N_("bandwidth limit, as scaled integer (default MiB/s)") }, + {.name = "bytes", + .type = VSH_OT_BOOL, + .help = N_("treat bandwidth as bytes/s") + }, {.name = "base", .type = VSH_OT_DATA, .help = N_("path of backing file in chain for a partial pull") diff --git a/tools/virsh.pod b/tools/virsh.pod index 7aba571..c9b4c31 100644 --- a/tools/virsh.pod +++ b/tools/virsh.pod @@ -847,7 +847,7 @@ currently in use by a running domain. Other contexts that require a MAC address of virtual interface (such as I<detach-interface> or I<domif-setlink>) will accept the MAC address printed by this command. -=item B<blockcommit> I<domain> I<path> [I<bandwidth>] +=item B<blockcommit> I<domain> I<path> [I<bandwidth> [I<--bytes>]] [I<base>] [I<--shallow>] [I<top>] [I<--delete>] [I<--keep-relative>] [I<--wait> [I<--async>] [I<--verbose>]] [I<--timeout> B<seconds>] [I<--active>] [{I<--pivot> | I<--keep-overlay>}] @@ -893,14 +893,14 @@ to a unique target name (<target dev='name'/>) or source file (<source file='name'/>) for one of the disk devices attached to I<domain> (see also B<domblklist> for listing these names). I<bandwidth> is a scaled integer (see B<NOTES> above) which defaults to -MiB/s if there is no suffix. It can be used to set a bandwidth limit for -the commit job. For backward compatibility, a negative value is treated -the same as 0 (unlimited); the actual upper limit accepted by the command -may depend on the word size of both client and server, or by further limits -of the hypervisor. +MiB/s if there is no suffix, or to bytes/s if I<--bytes> is present. It +can be used to set a bandwidth limit for the commit job. For backward +compatibility, a negative value is treated the same as 0 (unlimited); the +actual upper limit accepted by the command may depend on the word size of +both client and server, or by further limits of the hypervisor. =item B<blockcopy> I<domain> I<path> { I<dest> [I<format>] [I<--blockdev>] -| I<xml> } [I<--shallow>] [I<--reuse-external>] [I<bandwidth>] +| I<xml> } [I<--shallow>] [I<--reuse-external>] [I<bandwidth> [I<--bytes>]] [I<--wait> [I<--async>] [I<--verbose>]] [{I<--pivot> | I<--finish>}] [I<--timeout> B<seconds>] [I<granularity>] [I<buf-size>] @@ -945,11 +945,11 @@ while longer until the job has actually cancelled. I<path> specifies fully-qualified path of the disk. I<bandwidth> is a scaled integer (see B<NOTES> above) which defaults to -MiB/s if there is no suffix. It can be used to set a bandwidth limit for -the copy job. For backward compatibility, a negative value is treated -the same as 0 (unlimited); the actual upper limit accepted by the command -may depend on the word size of both client and server, or by further limits -of the hypervisor. +MiB/s if there is no suffix, or to bytes/s if I<--bytes> is present. It +can be used to set a bandwidth limit for the copy job. For backward +compatibility, a negative value is treated the same as 0 (unlimited); the +actual upper limit accepted by the command may depend on the word size of +both client and server, or by further limits of the hypervisor. Specifying I<granularity> allows fine-tuning of the granularity that will be copied when a dirty region is detected; larger values trigger less I/O overhead but may end up copying more data overall (the default value is @@ -958,7 +958,7 @@ will control how much data can be simultaneously in-flight during the copy; larger values use more memory but may allow faster completion (the default value is usually correct). -=item B<blockpull> I<domain> I<path> [I<bandwidth>] [I<base>] +=item B<blockpull> I<domain> I<path> [I<bandwidth> [I<--bytes>]] [I<base>] [I<--wait> [I<--verbose>] [I<--timeout> B<seconds>] [I<--async>]] [I<--keep-relative>] @@ -988,11 +988,11 @@ to a unique target name (<target dev='name'/>) or source file (<source file='name'/>) for one of the disk devices attached to I<domain> (see also B<domblklist> for listing these names). I<bandwidth> is a scaled integer (see B<NOTES> above) which defaults to -MiB/s if there is no suffix. It can be used to set a bandwidth limit for -the pull job. For backward compatibility, a negative value is treated -the same as 0 (unlimited); the actual upper limit accepted by the command -may depend on the word size of both client and server, or by further limits -of the hypervisor. +MiB/s if there is no suffix, or to bytes/s if I<--bytes> is present. It +can be used to set a bandwidth limit for the pull job. For backward +compatibility, a negative value is treated the same as 0 (unlimited); the +actual upper limit accepted by the command may depend on the word size of +both client and server, or by further limits of the hypervisor. =item B<blkdeviotune> I<domain> I<device> [[I<--config>] [I<--live>] | [I<--current>]] @@ -1029,7 +1029,7 @@ exclusive. If no flag is specified, behavior is different depending on hypervisor. =item B<blockjob> I<domain> I<path> { [I<--abort>] [I<--async>] [I<--pivot>] | -[I<--info>] [I<--raw>] [I<--bytes>] | [I<bandwidth>] } +[I<--info>] [I<--raw>] | [I<bandwidth>] } [I<--bytes>] Manage active block operations. There are three mutually-exclusive modes: I<--info>, I<bandwidth>, and I<--abort>. I<--async> and I<--pivot> imply @@ -1057,11 +1057,11 @@ listed in MiB/s and human-readable output automatically selects the best resolution supported by the server. I<bandwidth> is a scaled integer (see B<NOTES> above) which defaults to -MiB/s if there is no suffix. It can be used to set bandwidth limit for -the active job. For backward compatibility, a negative value is treated -the same as 0 (unlimited); the actual upper limit accepted by the command -may depend on the word size of both client and server, or by further limits -of the hypervisor. +MiB/s if there is no suffix, or to bytes/s if I<--bytes> is present. It +can be used to set bandwidth limit for the active job. For backward +compatibility, a negative value is treated the same as 0 (unlimited); the +actual upper limit accepted by the command may depend on the word size of +both client and server, or by further limits of the hypervisor. =item B<blockresize> I<domain> I<path> I<size> -- 1.9.3
On 09/12/14 05:55, Eric Blake wrote:
Take the previous patch one step further. In addition to automatically selecting byte mode with fallback if the user's result would be rounded, we also want to give the user a way to guarantee byte mode even if rounding is not required, with no fallback if the remote side doesn't support byte mode. This way, virsh can be used to more fully test the impact of setting the bytes flag.
* tools/virsh-domain.c (blockJobBandwidth): Adjust return type and check for --bytes flag. (cmdBlockCommit, cmdBlockPull, cmdBlockJob): Add --bytes flag. (cmdBlockCopy): Likewise, and adjust caller. (blockJobImpl): Adjust caller. * tools/virsh.pod (blockcommit, blockcopy, blockpull, blockjob): Document this.
Signed-off-by: Eric Blake <eblake@redhat.com> --- tools/virsh-domain.c | 57 ++++++++++++++++++++++++++++++++++------------------ tools/virsh.pod | 48 +++++++++++++++++++++---------------------- 2 files changed, 61 insertions(+), 44 deletions(-)
diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
Whoah, this is getting a bit too complex IMHO for a simple task. How about not doing any automagic switching and if the user specifies the --bytes flag then it would be passed and otherwise just the old MiB/s value. The --bytes flag could also parse the value as a scaled integer then. This would allow to drop a lot of the complexity while still allow to use the new flags as we unfortunately have to stick with MiB/s in virsh. That on the other hand doesn't force us to do overly complex logic that can be left on the operator. Peter
On 09/12/2014 06:32 AM, Peter Krempa wrote:
On 09/12/14 05:55, Eric Blake wrote:
Take the previous patch one step further. In addition to automatically selecting byte mode with fallback if the user's result would be rounded, we also want to give the user a way to guarantee byte mode even if rounding is not required, with no fallback if the remote side doesn't support byte mode. This way, virsh can be used to more fully test the impact of setting the bytes flag.
* tools/virsh-domain.c (blockJobBandwidth): Adjust return type and check for --bytes flag. (cmdBlockCommit, cmdBlockPull, cmdBlockJob): Add --bytes flag. (cmdBlockCopy): Likewise, and adjust caller. (blockJobImpl): Adjust caller. * tools/virsh.pod (blockcommit, blockcopy, blockpull, blockjob): Document this.
Signed-off-by: Eric Blake <eblake@redhat.com> --- tools/virsh-domain.c | 57 ++++++++++++++++++++++++++++++++++------------------ tools/virsh.pod | 48 +++++++++++++++++++++---------------------- 2 files changed, 61 insertions(+), 44 deletions(-)
diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
Whoah, this is getting a bit too complex IMHO for a simple task.
How about not doing any automagic switching and if the user specifies the --bytes flag then it would be passed and otherwise just the old MiB/s value. The --bytes flag could also parse the value as a scaled integer then.
So if I understand your proposal, old call, in MiB: blockjob $dom $disk --bandwidth 1 invalid (attempt to scale, but no --bytes present) blockjob $dom $disk --bandwidth 1B new call, in bytes (awfully small value!), fails if server doesn't work blockjob $dom $disk --bandwidth 1 --bytes new call, in bytes, but using scaling to match old call blockjob $dom $disk --bandwidth 1M --bytes and no automatic fallbacks. I can live with that, but it means ditching patches 5 and 6, and posting a complete alternative. Give me a couple days (I've got another project I'm working at the moment, for image allocation high-watermark reporting), and we'll compare the two approaches.
This would allow to drop a lot of the complexity while still allow to use the new flags as we unfortunately have to stick with MiB/s in virsh. That on the other hand doesn't force us to do overly complex logic that can be left on the operator.
Any other opinions on whether virsh should be user-friendly (automagic fallback) vs. more literal exposure (no scaled input without --bytes, but maps closer to API)? -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
participants (2)
-
Eric Blake -
Peter Krempa