On 11/15/2011 02:02 AM, Lei Li wrote:
> Implement the block I/O throttle setting and getting support to qemu
> driver.
>
> Signed-off-by: Lei Li <lilei@xxxxxxxxxxxxxxxxxx>
> Signed-off-by: Zhi Yong Wu <wuzhy@xxxxxxxxxxxxxxxxxx>
> ---
> src/qemu/qemu_driver.c | 342 ++++++++++++++++++++++++++++++++++++++++++
> src/qemu/qemu_monitor.c | 37 +++++
> src/qemu/qemu_monitor.h | 22 +++
> src/qemu/qemu_monitor_json.c | 186 +++++++++++++++++++++++
> src/qemu/qemu_monitor_json.h | 10 ++
> src/qemu/qemu_monitor_text.c | 165 ++++++++++++++++++++
> src/qemu/qemu_monitor_text.h | 10 ++
> 7 files changed, 772 insertions(+), 0 deletions(-)
Now that I've finished reviewing 4/8 and 6/8, I'm back to this one. In
addition to the syntax check fixes I previously mentioned...
> +static int
> +qemuDomainSetBlockIoTune(virDomainPtr dom,
> + const char *disk,
> + virTypedParameterPtr params,
> + int nparams,
> + unsigned int flags)
> +{
> +
> + device = qemuDiskPathToAlias(vm, disk);
Huh, I just noticed that qemuDiskPathToAlias returns a malloc'd string
as const char*, which is not const-correct. I'll fix that separately.
> + if (qemuDomainObjBeginJobWithDriver(driver, vm, QEMU_JOB_MODIFY) < 0)
> + goto cleanup;
> +
> + isActive = virDomainObjIsActive(vm);
> +
> + if (flags == VIR_DOMAIN_AFFECT_CURRENT) {
> + if (isActive)
> + flags = VIR_DOMAIN_AFFECT_LIVE;
> + else
> + flags = VIR_DOMAIN_AFFECT_CONFIG;
> + }
> +
> + if (!isActive && (flags & VIR_DOMAIN_AFFECT_LIVE)) {
> + qemuReportError(VIR_ERR_OPERATION_INVALID, "%s",
> + _("domain is not running"));
> + goto endjob;
> + }
> +
> + if (flags & VIR_DOMAIN_AFFECT_CONFIG) {
> + if (!vm->persistent) {
> + qemuReportError(VIR_ERR_OPERATION_INVALID, "%s",
> + _("cannot change persistent config of a transient domain"));
> + goto endjob;
> + }
> + if (!(persistentDef = virDomainObjGetPersistentDef(driver->caps, vm)))
> + goto endjob;
> + }
Hmm, we repeat this chunk of code in several functions; maybe it's time
to factor it into a helper method. But that's a patch for another day.
> +
> + for (i = 0; i < nparams; i++) {
> + virTypedParameterPtr param = ¶ms[i];
> +
> + if (STREQ(param->field, VIR_DOMAIN_BLOCK_IOTUNE_TOTAL_BYTES_SEC)) {
> +
> + info.total_bytes_sec = params[i].value.ul;
> + } else if (STREQ(param->field, VIR_DOMAIN_BLOCK_IOTUNE_READ_BYTES_SEC)) {
> + info.read_bytes_sec = params[i].value.ul;
> + } else if (STREQ(param->field, VIR_DOMAIN_BLOCK_IOTUNE_WRITE_BYTES_SEC)) {
> + info.write_bytes_sec = params[i].value.ul;
> + } else if (STREQ(param->field, VIR_DOMAIN_BLOCK_IOTUNE_TOTAL_IOPS_SEC)) {
> + info.total_iops_sec = params[i].value.ul;
> + } else if (STREQ(param->field, VIR_DOMAIN_BLOCK_IOTUNE_READ_IOPS_SEC)) {
> + info.read_iops_sec = params[i].value.ul;
> + } else if (STREQ(param->field, VIR_DOMAIN_BLOCK_IOTUNE_WRITE_IOPS_SEC)) {
> + info.write_iops_sec = params[i].value.ul;
This is not type-safe. You have to also validate that the user passed a
ullong value before you blindly dereference the value.ul union memory.
Hmm - passing nparams==1 to set just read_bytes_sec has the side effect
of wiping out any existing total_iops_sec, even though those two
parameters seem somewhat orthogonal, all because we zero-initialized the
struct that we pass on to the monitor command. Is that intended? I can
live with it (but it probably ought to be documented), but we may want
to consider being more flexible, by using '0' to clear a previous limit,
but initializing to '-1' to imply the limit does not change. Then the
qemu_monitor_json code should only emit the arguments that are actually
being changed, rather than blindly always outputting 6 parameters even
when the user only passed in one parameter. But I'm okay delaying that
to a separate patch based on whether others think it would be a good
improvement.
> + } else {
> + qemuReportError(VIR_ERR_OPERATION_INVALID, "%s",
> + _("Unrecognized parameter"));
Why not tell the user which parameter name was not recognized? Also,
this is not an OPERATION_INVALID, but an INVALID_ARG (it is bad input,
not input that might be valid if the domain were in a different state).
> + goto endjob;
> + }
> + }
> +
> + if ((info.total_bytes_sec && info.read_bytes_sec) ||
> + (info.total_bytes_sec && info.write_bytes_sec)) {
> + qemuReportError(VIR_ERR_INVALID_ARG, "%s",
> + _("total and read/write of bytes_sec cannot be set at the same time"));
> + goto endjob;
> + }
> +
> + if ((info.total_iops_sec && info.read_iops_sec) ||
> + (info.total_iops_sec && info.write_iops_sec)) {
> + qemuReportError(VIR_ERR_INVALID_ARG, "%s",
> + _("total and read/write of iops_sec cannot be set at the same time"));
> + goto endjob;
> + }
Good - this is consistent with the domain_conf restrictions on the XML
parse.
> +
> + if (flags & VIR_DOMAIN_AFFECT_LIVE) {
> + priv = vm->privateData;
> + qemuDomainObjEnterMonitorWithDriver(driver, vm);
> + ret = qemuMonitorSetBlockIoThrottle(priv->mon, device, &info, 1);
> + qemuDomainObjExitMonitorWithDriver(driver, vm);
> + }
> +
> + if (flags & VIR_DOMAIN_AFFECT_CONFIG) {
> + sa_assert(persistentDef);
> + int idx = virDomainDiskIndexByName(vm->def, disk, true);
Here, you want false for the third parameter, to ensure we don't get
confused by an ambiguous name.
> + if (i < 0)
> + goto endjob;
> + persistentDef->disks[idx]->blkdeviotune.total_bytes_sec = info.total_bytes_sec;
> + persistentDef->disks[idx]->blkdeviotune.read_bytes_sec = info.read_bytes_sec;
> + persistentDef->disks[idx]->blkdeviotune.write_bytes_sec = info.write_bytes_sec;
> + persistentDef->disks[idx]->blkdeviotune.total_iops_sec = info.total_iops_sec;
> + persistentDef->disks[idx]->blkdeviotune.read_iops_sec = info.read_iops_sec;
> + persistentDef->disks[idx]->blkdeviotune.write_iops_sec = info.write_iops_sec;
> + persistentDef->disks[idx]->blkdeviotune.mark = 1;
I got rid of the mark field during my edits to 4/8. And thinking about
it further, field-by-field copying is tedious. If we move
virDomainBlockIoTuneInfo out of qemu_monitor.h (which is a fishy name,
anyways, since structs in that file should be in the qemuMonitor
namespace), and into domain_conf.h (where the name is already
appropriate), then we can reuse that named struct instead of an
anonymous one, allowing direct struct copying.
> + }
> +
> + if (flags & VIR_DOMAIN_AFFECT_CONFIG) {
No need for two adjacent if blocks with the same condition. Conversely,
we want to do the disk lookup prior to making any modification to live
settings if both LIVE and CONFIG are present, to ensure that we don't
strand the user with a live change but then can't make the persistent
change.
> +static int
> +qemuDomainGetBlockIoTune(virDomainPtr dom,
> + const char *disk,
> + virTypedParameterPtr params,
> + int *nparams,
> + unsigned int flags)
> +{
> +
> + device = qemuDiskPathToAlias(vm, disk);
> +
> + if (!device) {
> + goto cleanup;
> + }
> +
> +
> + if ((*nparams) == 0) {
> + /* Current number of parameters supported by QEMU Block I/O Throttling */
> + *nparams = QEMU_NB_BLKIOTHROTTLE_PARAM;
> + ret = 0;
> + goto endjob;
> + }
These need re-ordering, given my change in 1/8 to allow NULL disk when
probing max params.
> +
> + if (flags & VIR_DOMAIN_AFFECT_LIVE) {
> + priv = vm->privateData;
> + qemuDomainObjEnterMonitorWithDriver(driver, vm);
> + ret = qemuMonitorGetBlockIoThrottle(priv->mon, device, &reply, 0);
No need for a flags argument if you never pass any flags down to the
monitor level.
> + qemuDomainObjExitMonitorWithDriver(driver, vm);
> + }
> +
> + if (flags & VIR_DOMAIN_AFFECT_CONFIG) {
> + if (!vm->persistent) {
> + qemuReportError(VIR_ERR_OPERATION_INVALID, "%s",
> + _("domain is transient"));
> + goto endjob;
> + }
> + if (!(persistentDef = virDomainObjGetPersistentDef(driver->caps, vm)))
> + goto endjob;
> +
> + sa_assert(persistentDef);
No need for this particular assert (it only helps static analyzers when
you have split if() conditionals; but here there is no split).
> + int idx = virDomainDiskIndexByName(vm->def, disk, true);
> + if (idx < 0)
> + goto endjob;
> + reply.total_bytes_sec = persistentDef->disks[idx]->blkdeviotune.total_bytes_sec;
> + reply.read_bytes_sec = persistentDef->disks[idx]->blkdeviotune.read_bytes_sec;
> + reply.write_bytes_sec = persistentDef->disks[idx]->blkdeviotune.write_bytes_sec;
> + reply.total_iops_sec = persistentDef->disks[idx]->blkdeviotune.total_iops_sec;
> + reply.read_iops_sec = persistentDef->disks[idx]->blkdeviotune.read_iops_sec;
> + reply.write_iops_sec = persistentDef->disks[idx]->blkdeviotune.write_iops_sec;
Another place where struct copy is nicer.
> + ret = 0;
> + }
> +
> + if ((ret != 0) && ((*nparams) < QEMU_NB_BLKIOTHROTTLE_PARAM)) {
> + qemuReportError(VIR_ERR_OPERATION_INVALID, "%s",
> + _("Cannot get block I/O parameters"));
Why the comparison of *nparams? That has no bearing on whether the
monitor command succeeded. Besides, the monitor call should have
already reported a message. This can simply be: if (ret < 0) goto endjob.
> + goto endjob;
> + }
> +
> + for (i = 0; i < QEMU_NB_BLKIOTHROTTLE_PARAM; i++) {
This needs to check that we don't exceed nparams as well.
> + param->value.ul = reply.write_iops_sec;
> + break;
> + default:
> + break;
> + }
> + }
> + ret = 0;
You need to properly set *nparams before returning.
> +++ b/src/qemu/qemu_monitor.c
> @@ -2567,6 +2567,43 @@ int qemuMonitorBlockJob(qemuMonitorPtr mon,
> return ret;
> }
>
> +int qemuMonitorSetBlockIoThrottle(qemuMonitorPtr mon,
> + const char *device,
> + virDomainBlockIoTuneInfoPtr info,
> + unsigned int flags)
> +{
> + int ret;
> +
> + VIR_DEBUG("mon=%p, device=%p, info=%p, flags=%x",
> + mon, device, info, flags);
See my earlier comment about removing the flags argument, since we
weren't exploiting it for any changes in behavior.
> +++ b/src/qemu/qemu_monitor.h
> @@ -521,6 +521,28 @@ int qemuMonitorOpenGraphics(qemuMonitorPtr mon,
> const char *fdname,
> bool skipauth);
>
> +
> +typedef struct _virDomainBlockIoTuneInfo virDomainBlockIoTuneInfo;
> +struct _virDomainBlockIoTuneInfo {
> + unsigned long long total_bytes_sec;
> + unsigned long long read_bytes_sec;
> + unsigned long long write_bytes_sec;
> + unsigned long long total_iops_sec;
> + unsigned long long read_iops_sec;
> + unsigned long long write_iops_sec;
> +};
> +typedef virDomainBlockIoTuneInfo *virDomainBlockIoTuneInfoPtr;
I already moved this hunk into 4/8, in order to use struct copying.
> +static int
> +qemuMonitorJSONBlockIoThrottleInfo(virJSONValuePtr result,
> + const char *device,
> + virDomainBlockIoTuneInfoPtr reply)
> +{
> + virJSONValuePtr io_throttle;
> + int ret = -1;
> + int i;
> + int found = 0;
> +
> + io_throttle = virJSONValueObjectGet(result, "return");
> +
> + if (!io_throttle || io_throttle->type != VIR_JSON_TYPE_ARRAY) {
> + qemuReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> + _(" block_io_throttle reply was missing device list "));
trailing space in the message
> +
> + if (virJSONValueObjectGetNumberUlong(inserted, "bps", &reply->total_bytes_sec) < 0) {
> + qemuReportError(VIR_ERR_INTERNAL_ERROR,
> + _("cannot read %s"), "total_bytes_sec");
> + goto cleanup;
> + }
I haven't yet tested this with qemu (right now, I'm focusing on code
review), so I may have to do a further patch: if talking to an older
qemu that doesn't output I/O throttling limits, then we can safely
assume that there are no limits, and we should gracefully return all
fields 0 in that case instead of an error message. I'm also assuming
that patched qemu honors the same rules about total being exclusive with
read/write values, and that qemu always outputs all six values with 0 in
the proper places, if it supports throttling. If my assumptions are
wrong, then libvirt may have to do a bit more work in parsing qemu output.
> + if (!found) {
> + qemuReportError(VIR_ERR_INTERNAL_ERROR,
> + _("cannot found info for device '%s'"),
s/found/find/
> + if (flags) {
> + cmd = qemuMonitorJSONMakeCommand("block_set_io_throttle",
> + "s:device", device,
> + "U:bps", info->total_bytes_sec,
> + "U:bps_rd", info->read_bytes_sec,
> + "U:bps_wr", info->write_bytes_sec,
> + "U:iops", info->total_iops_sec,
> + "U:iops_rd", info->read_iops_sec,
> + "U:iops_wr", info->write_iops_sec,
> + NULL);
> + }
> +
> + if (!cmd)
> + return -1;
> +
> + ret = qemuMonitorJSONCommand(mon, cmd, &result);
> +
> + if (ret == 0 && virJSONValueObjectHasKey(result, "error")) {
This may also need tweaking when talking to older qemu that lacks the
block_set_io_throttle monitor command, to give a sensible error message
(unless all 6 parameters are 0, in which case return success).
> +static int qemuMonitorTextParseBlockIoThrottle(const char *result,
> + const char *device,
> + virDomainBlockIoTuneInfoPtr reply)
> +{
> + char *dummy = NULL;
> + int ret = -1;
> + const char *p, *eol;
> + int devnamelen = strlen(device);
> +
> + p = result;
> +
> + while (*p) {
> + if (STRPREFIX(p, QEMU_DRIVE_HOST_PREFIX))
> + p += strlen(QEMU_DRIVE_HOST_PREFIX);
> +
> + if (STREQLEN(p, device, devnamelen) &&
> + p[devnamelen] == ':' && p[devnamelen+1] == ' ') {
> +
> + eol = strchr(p, '\n');
> + if (!eol)
> + eol = p + strlen(p);
> +
> + p += devnamelen + 2; /*Skip to first label. */
Typically a space between /* and the comment body. This was
copy-and-paste, though.
As written, 3/8 only touched the monitor command; but to be complete,
you must also implement the command-line changes. Thankfully, that was
already done in a hunk of 4/8 that I moved here. The end result -
here's everything I plan on squashing, even without any qemu testing (I
may post further tweaks later once I start testing this with old and new
qemu)
diff --git i/src/qemu/qemu_command.c w/src/qemu/qemu_command.c
index 85b34bb..905afa6 100644
--- i/src/qemu/qemu_command.c
+++ w/src/qemu/qemu_command.c
@@ -1866,6 +1866,37 @@ qemuBuildDriveStr(virConnectPtr conn
ATTRIBUTE_UNUSED,
}
}
+ /*block I/O throttling*/
+ if (disk->blkdeviotune.total_bytes_sec) {
+ virBufferAsprintf(&opt, ",bps=%llu",
+ disk->blkdeviotune.total_bytes_sec);
+ }
+
+ if (disk->blkdeviotune.read_bytes_sec) {
+ virBufferAsprintf(&opt, ",bps_rd=%llu",
+ disk->blkdeviotune.read_bytes_sec);
+ }
+
+ if (disk->blkdeviotune.write_bytes_sec) {
+ virBufferAsprintf(&opt, ",bps_wr=%llu",
+ disk->blkdeviotune.write_bytes_sec);
+ }
+
+ if (disk->blkdeviotune.total_iops_sec) {
+ virBufferAsprintf(&opt, ",iops=%llu",
+ disk->blkdeviotune.total_iops_sec);
+ }
+
+ if (disk->blkdeviotune.read_iops_sec) {
+ virBufferAsprintf(&opt, ",iops_rd=%llu",
+ disk->blkdeviotune.read_iops_sec);
+ }
+
+ if (disk->blkdeviotune.write_iops_sec) {
+ virBufferAsprintf(&opt, ",iops_wr=%llu",
+ disk->blkdeviotune.write_iops_sec);
+ }
+
if (virBufferError(&opt)) {
virReportOOMError();
goto error;
diff --git i/src/qemu/qemu_driver.c w/src/qemu/qemu_driver.c
index e350ffa..0308a41 100644
--- i/src/qemu/qemu_driver.c
+++ w/src/qemu/qemu_driver.c
@@ -93,7 +93,7 @@
#define QEMU_NB_MEM_PARAM 3
-#define QEMU_NB_BLKIOTHROTTLE_PARAM 6
+#define QEMU_NB_BLOCK_IO_TUNE_PARAM 6
#if HAVE_LINUX_KVM_H
# include <linux/kvm.h>
@@ -10833,22 +10833,34 @@ qemuDomainSetBlockIoTune(virDomainPtr dom,
for (i = 0; i < nparams; i++) {
virTypedParameterPtr param = ¶ms[i];
- if (STREQ(param->field, VIR_DOMAIN_BLOCK_IOTUNE_TOTAL_BYTES_SEC)) {
+ if (param->type != VIR_TYPED_PARAM_ULLONG) {
+ qemuReportError(VIR_ERR_INVALID_ARG,
+ _("expected unsigned long long for
parameter %s"),
+ param->field);
+ goto endjob;
+ }
- info.total_bytes_sec = params[i].value.ul;
- } else if (STREQ(param->field,
VIR_DOMAIN_BLOCK_IOTUNE_READ_BYTES_SEC)) {
- info.read_bytes_sec = params[i].value.ul;
- } else if (STREQ(param->field,
VIR_DOMAIN_BLOCK_IOTUNE_WRITE_BYTES_SEC)) {
- info.write_bytes_sec = params[i].value.ul;
- } else if (STREQ(param->field,
VIR_DOMAIN_BLOCK_IOTUNE_TOTAL_IOPS_SEC)) {
- info.total_iops_sec = params[i].value.ul;
- } else if (STREQ(param->field,
VIR_DOMAIN_BLOCK_IOTUNE_READ_IOPS_SEC)) {
- info.read_iops_sec = params[i].value.ul;
- } else if (STREQ(param->field,
VIR_DOMAIN_BLOCK_IOTUNE_WRITE_IOPS_SEC)) {
- info.write_iops_sec = params[i].value.ul;
+ if (STREQ(param->field, VIR_DOMAIN_BLOCK_IOTUNE_TOTAL_BYTES_SEC)) {
+ info.total_bytes_sec = param->value.ul;
+ } else if (STREQ(param->field,
+ VIR_DOMAIN_BLOCK_IOTUNE_READ_BYTES_SEC)) {
+ info.read_bytes_sec = param->value.ul;
+ } else if (STREQ(param->field,
+ VIR_DOMAIN_BLOCK_IOTUNE_WRITE_BYTES_SEC)) {
+ info.write_bytes_sec = param->value.ul;
+ } else if (STREQ(param->field,
+ VIR_DOMAIN_BLOCK_IOTUNE_TOTAL_IOPS_SEC)) {
+ info.total_iops_sec = param->value.ul;
+ } else if (STREQ(param->field,
+ VIR_DOMAIN_BLOCK_IOTUNE_READ_IOPS_SEC)) {
+ info.read_iops_sec = param->value.ul;
+ } else if (STREQ(param->field,
+ VIR_DOMAIN_BLOCK_IOTUNE_WRITE_IOPS_SEC)) {
+ info.write_iops_sec = param->value.ul;
} else {
- qemuReportError(VIR_ERR_OPERATION_INVALID, "%s",
- _("Unrecognized parameter"));
+ qemuReportError(VIR_ERR_INVALID_ARG,
+ _("Unrecognized parameter %s"),
+ param->field);
goto endjob;
}
}
@@ -10867,25 +10879,19 @@ qemuDomainSetBlockIoTune(virDomainPtr dom,
goto endjob;
}
- if (flags & VIR_DOMAIN_AFFECT_LIVE) {
- priv = vm->privateData;
- qemuDomainObjEnterMonitorWithDriver(driver, vm);
- ret = qemuMonitorSetBlockIoThrottle(priv->mon, device, &info, 1);
- qemuDomainObjExitMonitorWithDriver(driver, vm);
- }
-
if (flags & VIR_DOMAIN_AFFECT_CONFIG) {
sa_assert(persistentDef);
int idx = virDomainDiskIndexByName(vm->def, disk, true);
if (i < 0)
goto endjob;
- persistentDef->disks[idx]->blkdeviotune.total_bytes_sec =
info.total_bytes_sec;
- persistentDef->disks[idx]->blkdeviotune.read_bytes_sec =
info.read_bytes_sec;
- persistentDef->disks[idx]->blkdeviotune.write_bytes_sec =
info.write_bytes_sec;
- persistentDef->disks[idx]->blkdeviotune.total_iops_sec =
info.total_iops_sec;
- persistentDef->disks[idx]->blkdeviotune.read_iops_sec =
info.read_iops_sec;
- persistentDef->disks[idx]->blkdeviotune.write_iops_sec =
info.write_iops_sec;
- persistentDef->disks[idx]->blkdeviotune.mark = 1;
+ persistentDef->disks[idx]->blkdeviotune = info;
+ }
+
+ if (flags & VIR_DOMAIN_AFFECT_LIVE) {
+ priv = vm->privateData;
+ qemuDomainObjEnterMonitorWithDriver(driver, vm);
+ ret = qemuMonitorSetBlockIoThrottle(priv->mon, device, &info);
+ qemuDomainObjExitMonitorWithDriver(driver, vm);
}
if (flags & VIR_DOMAIN_AFFECT_CONFIG) {
@@ -10943,6 +10949,13 @@ qemuDomainGetBlockIoTune(virDomainPtr dom,
goto cleanup;
}
+ if ((*nparams) == 0) {
+ /* Current number of parameters supported by QEMU Block I/O
Throttling */
+ *nparams = QEMU_NB_BLOCK_IO_TUNE_PARAM;
+ ret = 0;
+ goto cleanup;
+ }
+
device = qemuDiskPathToAlias(vm, disk);
if (!device) {
@@ -10973,18 +10986,13 @@ qemuDomainGetBlockIoTune(virDomainPtr dom,
goto endjob;
}
- if ((*nparams) == 0) {
- /* Current number of parameters supported by QEMU Block I/O
Throttling */
- *nparams = QEMU_NB_BLKIOTHROTTLE_PARAM;
- ret = 0;
- goto endjob;
- }
-
if (flags & VIR_DOMAIN_AFFECT_LIVE) {
priv = vm->privateData;
qemuDomainObjEnterMonitorWithDriver(driver, vm);
- ret = qemuMonitorGetBlockIoThrottle(priv->mon, device, &reply, 0);
+ ret = qemuMonitorGetBlockIoThrottle(priv->mon, device, &reply);
qemuDomainObjExitMonitorWithDriver(driver, vm);
+ if (ret < 0)
+ goto endjob;
}
if (flags & VIR_DOMAIN_AFFECT_CONFIG) {
@@ -10996,26 +11004,13 @@ qemuDomainGetBlockIoTune(virDomainPtr dom,
if (!(persistentDef =
virDomainObjGetPersistentDef(driver->caps, vm)))
goto endjob;
- sa_assert(persistentDef);
int idx = virDomainDiskIndexByName(vm->def, disk, true);
if (idx < 0)
goto endjob;
- reply.total_bytes_sec =
persistentDef->disks[idx]->blkdeviotune.total_bytes_sec;
- reply.read_bytes_sec =
persistentDef->disks[idx]->blkdeviotune.read_bytes_sec;
- reply.write_bytes_sec =
persistentDef->disks[idx]->blkdeviotune.write_bytes_sec;
- reply.total_iops_sec =
persistentDef->disks[idx]->blkdeviotune.total_iops_sec;
- reply.read_iops_sec =
persistentDef->disks[idx]->blkdeviotune.read_iops_sec;
- reply.write_iops_sec =
persistentDef->disks[idx]->blkdeviotune.write_iops_sec;
- ret = 0;
+ reply = persistentDef->disks[idx]->blkdeviotune;
}
- if ((ret != 0) && ((*nparams) < QEMU_NB_BLKIOTHROTTLE_PARAM)) {
- qemuReportError(VIR_ERR_OPERATION_INVALID, "%s",
- _("Cannot get block I/O parameters"));
- goto endjob;
- }
-
- for (i = 0; i < QEMU_NB_BLKIOTHROTTLE_PARAM; i++) {
+ for (i = 0; i < QEMU_NB_BLOCK_IO_TUNE_PARAM && i < *nparams; i++) {
virTypedParameterPtr param = ¶ms[i];
param->value.ul = 0;
param->type = VIR_TYPED_PARAM_ULLONG;
@@ -11090,6 +11085,9 @@ qemuDomainGetBlockIoTune(virDomainPtr dom,
break;
}
}
+
+ if (*nparams > QEMU_NB_BLOCK_IO_TUNE_PARAM)
+ *nparams = QEMU_NB_BLOCK_IO_TUNE_PARAM;
ret = 0;
endjob:
diff --git i/src/qemu/qemu_monitor.c w/src/qemu/qemu_monitor.c
index c0b711f..660aa11 100644
--- i/src/qemu/qemu_monitor.c
+++ w/src/qemu/qemu_monitor.c
@@ -2569,36 +2569,32 @@ int qemuMonitorBlockJob(qemuMonitorPtr mon,
int qemuMonitorSetBlockIoThrottle(qemuMonitorPtr mon,
const char *device,
- virDomainBlockIoTuneInfoPtr info,
- unsigned int flags)
+ virDomainBlockIoTuneInfoPtr info)
{
int ret;
- VIR_DEBUG("mon=%p, device=%p, info=%p, flags=%x",
- mon, device, info, flags);
+ VIR_DEBUG("mon=%p, device=%p, info=%p", mon, device, info);
if (mon->json) {
- ret = qemuMonitorJSONSetBlockIoThrottle(mon, device, info, flags);
+ ret = qemuMonitorJSONSetBlockIoThrottle(mon, device, info);
} else {
- ret = qemuMonitorTextSetBlockIoThrottle(mon, device, info, flags);
+ ret = qemuMonitorTextSetBlockIoThrottle(mon, device, info);
}
return ret;
}
int qemuMonitorGetBlockIoThrottle(qemuMonitorPtr mon,
const char *device,
- virDomainBlockIoTuneInfoPtr reply,
- unsigned int flags)
+ virDomainBlockIoTuneInfoPtr reply)
{
int ret;
- VIR_DEBUG("mon=%p, device=%p, reply=%p, flags=%x",
- mon, device, reply, flags);
+ VIR_DEBUG("mon=%p, device=%p, reply=%p", mon, device, reply);
if (mon->json) {
- ret = qemuMonitorJSONGetBlockIoThrottle(mon, device, reply, flags);
+ ret = qemuMonitorJSONGetBlockIoThrottle(mon, device, reply);
} else {
- ret = qemuMonitorTextGetBlockIoThrottle(mon, device, reply, flags);
+ ret = qemuMonitorTextGetBlockIoThrottle(mon, device, reply);
}
return ret;
}
diff --git i/src/qemu/qemu_monitor.h w/src/qemu/qemu_monitor.h
index 48c6db0..0b14e0c 100644
--- i/src/qemu/qemu_monitor.h
+++ w/src/qemu/qemu_monitor.h
@@ -521,27 +521,13 @@ int qemuMonitorOpenGraphics(qemuMonitorPtr mon,
const char *fdname,
bool skipauth);
-
-typedef struct _virDomainBlockIoTuneInfo virDomainBlockIoTuneInfo;
-struct _virDomainBlockIoTuneInfo {
- unsigned long long total_bytes_sec;
- unsigned long long read_bytes_sec;
- unsigned long long write_bytes_sec;
- unsigned long long total_iops_sec;
- unsigned long long read_iops_sec;
- unsigned long long write_iops_sec;
-};
-typedef virDomainBlockIoTuneInfo *virDomainBlockIoTuneInfoPtr;
-
int qemuMonitorSetBlockIoThrottle(qemuMonitorPtr mon,
const char *device,
- virDomainBlockIoTuneInfoPtr info,
- unsigned int flags);
+ virDomainBlockIoTuneInfoPtr info);
int qemuMonitorGetBlockIoThrottle(qemuMonitorPtr mon,
const char *device,
- virDomainBlockIoTuneInfoPtr reply,
- unsigned int flags);
+ virDomainBlockIoTuneInfoPtr reply);
/**
* When running two dd process and using <> redirection, we need a
diff --git i/src/qemu/qemu_monitor_json.c w/src/qemu/qemu_monitor_json.c
index 925031e..fb2a1fd 100644
--- i/src/qemu/qemu_monitor_json.c
+++ w/src/qemu/qemu_monitor_json.c
@@ -3292,7 +3292,7 @@ qemuMonitorJSONBlockIoThrottleInfo(virJSONValuePtr
result,
if (!io_throttle || io_throttle->type != VIR_JSON_TYPE_ARRAY) {
qemuReportError(VIR_ERR_INTERNAL_ERROR, "%s",
- _(" block_io_throttle reply was missing device
list "));
+ _(" block_io_throttle reply was missing device
list"));
goto cleanup;
}
@@ -3328,38 +3328,38 @@
qemuMonitorJSONBlockIoThrottleInfo(virJSONValuePtr result,
}
if (virJSONValueObjectGetNumberUlong(inserted, "bps",
&reply->total_bytes_sec) < 0) {
- qemuReportError(VIR_ERR_INTERNAL_ERROR,
- _("cannot read %s"), "total_bytes_sec");
+ qemuReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("cannot read total_bytes_sec"));
goto cleanup;
}
if (virJSONValueObjectGetNumberUlong(inserted, "bps_rd",
&reply->read_bytes_sec) < 0) {
- qemuReportError(VIR_ERR_INTERNAL_ERROR,
- _("cannot read %s"), "read_bytes_sec");
+ qemuReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("cannot read read_bytes_sec"));
goto cleanup;
}
if (virJSONValueObjectGetNumberUlong(inserted, "bps_wr",
&reply->write_bytes_sec) < 0) {
- qemuReportError(VIR_ERR_INTERNAL_ERROR,
- _("cannot read %s"), "write_bytes_sec");
+ qemuReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("cannot read write_bytes_sec"));
goto cleanup;
}
if (virJSONValueObjectGetNumberUlong(inserted, "iops",
&reply->total_iops_sec) < 0) {
- qemuReportError(VIR_ERR_INTERNAL_ERROR,
- _("cannot read %s"), "total_iops_sec");
+ qemuReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("cannot read total_iops_sec"));
goto cleanup;
}
if (virJSONValueObjectGetNumberUlong(inserted, "iops_rd",
&reply->read_iops_sec) < 0) {
- qemuReportError(VIR_ERR_INTERNAL_ERROR,
- _("cannot read %s"), "read_iops_sec");
+ qemuReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("cannot read read_iops_sec"));
goto cleanup;
}
if (virJSONValueObjectGetNumberUlong(inserted, "iops_wr",
&reply->write_iops_sec) < 0) {
- qemuReportError(VIR_ERR_INTERNAL_ERROR,
- _("cannot read %s"), "write_iops_sec");
+ qemuReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("cannot read write_iops_sec"));
goto cleanup;
}
break;
@@ -3367,7 +3367,7 @@ qemuMonitorJSONBlockIoThrottleInfo(virJSONValuePtr
result,
if (!found) {
qemuReportError(VIR_ERR_INTERNAL_ERROR,
- _("cannot found info for device '%s'"),
+ _("cannot find throttling info for device '%s'"),
device);
goto cleanup;
}
@@ -3379,25 +3379,21 @@ cleanup:
int qemuMonitorJSONSetBlockIoThrottle(qemuMonitorPtr mon,
const char *device,
- virDomainBlockIoTuneInfoPtr info,
- unsigned int flags)
+ virDomainBlockIoTuneInfoPtr info)
{
int ret = -1;
virJSONValuePtr cmd = NULL;
virJSONValuePtr result = NULL;
- if (flags) {
- cmd = qemuMonitorJSONMakeCommand("block_set_io_throttle",
- "s:device", device,
- "U:bps", info->total_bytes_sec,
- "U:bps_rd", info->read_bytes_sec,
- "U:bps_wr", info->write_bytes_sec,
- "U:iops", info->total_iops_sec,
- "U:iops_rd", info->read_iops_sec,
- "U:iops_wr", info->write_iops_sec,
- NULL);
- }
-
+ cmd = qemuMonitorJSONMakeCommand("block_set_io_throttle",
+ "s:device", device,
+ "U:bps", info->total_bytes_sec,
+ "U:bps_rd", info->read_bytes_sec,
+ "U:bps_wr", info->write_bytes_sec,
+ "U:iops", info->total_iops_sec,
+ "U:iops_rd", info->read_iops_sec,
+ "U:iops_wr", info->write_iops_sec,
+ NULL);
if (!cmd)
return -1;
@@ -3423,18 +3419,13 @@ int
qemuMonitorJSONSetBlockIoThrottle(qemuMonitorPtr mon,
int qemuMonitorJSONGetBlockIoThrottle(qemuMonitorPtr mon,
const char *device,
- virDomainBlockIoTuneInfoPtr reply,
- unsigned int flags)
+ virDomainBlockIoTuneInfoPtr reply)
{
int ret = -1;
virJSONValuePtr cmd = NULL;
virJSONValuePtr result = NULL;
- if (!flags) {
- cmd = qemuMonitorJSONMakeCommand("query-block",
- NULL);
- }
-
+ cmd = qemuMonitorJSONMakeCommand("query-block", NULL);
if (!cmd) {
return -1;
}
@@ -3454,11 +3445,10 @@ int
qemuMonitorJSONGetBlockIoThrottle(qemuMonitorPtr mon,
ret = -1;
}
- if (ret == 0 && !flags)
+ if (ret == 0)
ret = qemuMonitorJSONBlockIoThrottleInfo(result, device, reply);
virJSONValueFree(cmd);
virJSONValueFree(result);
return ret;
}
-
diff --git i/src/qemu/qemu_monitor_json.h w/src/qemu/qemu_monitor_json.h
index bf12dc5..1b8625f 100644
--- i/src/qemu/qemu_monitor_json.h
+++ w/src/qemu/qemu_monitor_json.h
@@ -257,12 +257,10 @@ int qemuMonitorJSONOpenGraphics(qemuMonitorPtr mon,
int qemuMonitorJSONSetBlockIoThrottle(qemuMonitorPtr mon,
const char *device,
- virDomainBlockIoTuneInfoPtr info,
- unsigned int flags);
+ virDomainBlockIoTuneInfoPtr info);
int qemuMonitorJSONGetBlockIoThrottle(qemuMonitorPtr mon,
const char *device,
- virDomainBlockIoTuneInfoPtr reply,
- unsigned int flags);
+ virDomainBlockIoTuneInfoPtr reply);
#endif /* QEMU_MONITOR_JSON_H */
diff --git i/src/qemu/qemu_monitor_text.c w/src/qemu/qemu_monitor_text.c
index 0ccc111..c734892 100644
--- i/src/qemu/qemu_monitor_text.c
+++ w/src/qemu/qemu_monitor_text.c
@@ -812,7 +812,7 @@ int qemuMonitorTextGetBlockInfo(qemuMonitorPtr mon,
if (!eol)
eol = p + strlen(p);
- p += devnamelen + 2; /*Skip to first label. */
+ p += devnamelen + 2; /* Skip to first label. */
while (*p) {
if (STRPREFIX(p, "removable=")) {
@@ -3433,8 +3433,7 @@ cleanup:
int qemuMonitorTextSetBlockIoThrottle(qemuMonitorPtr mon,
const char *device,
- virDomainBlockIoTuneInfoPtr info,
- unsigned int flags)
+ virDomainBlockIoTuneInfoPtr info)
{
char *cmd = NULL;
char *result = NULL;
@@ -3442,13 +3441,11 @@ int
qemuMonitorTextSetBlockIoThrottle(qemuMonitorPtr mon,
const char *cmd_name = NULL;
/* For the not specified fields, 0 by default */
- if (flags) {
- cmd_name = "block_set_io_throttle";
- ret = virAsprintf(&cmd, "%s %s %llu %llu %llu %llu %llu %llu",
cmd_name,
- device, info->total_bytes_sec,
info->read_bytes_sec,
- info->write_bytes_sec, info->total_iops_sec,
- info->read_iops_sec, info->write_iops_sec);
- }
+ cmd_name = "block_set_io_throttle";
+ ret = virAsprintf(&cmd, "%s %s %llu %llu %llu %llu %llu %llu",
cmd_name,
+ device, info->total_bytes_sec, info->read_bytes_sec,
+ info->write_bytes_sec, info->total_iops_sec,
+ info->read_iops_sec, info->write_iops_sec);
if (ret < 0) {
virReportOOMError();
@@ -3475,9 +3472,10 @@ cleanup:
return ret;
}
-static int qemuMonitorTextParseBlockIoThrottle(const char *result,
- const char *device,
-
virDomainBlockIoTuneInfoPtr reply)
+static int
+qemuMonitorTextParseBlockIoThrottle(const char *result,
+ const char *device,
+ virDomainBlockIoTuneInfoPtr reply)
{
char *dummy = NULL;
int ret = -1;
@@ -3497,7 +3495,7 @@ static int
qemuMonitorTextParseBlockIoThrottle(const char *result,
if (!eol)
eol = p + strlen(p);
- p += devnamelen + 2; /*Skip to first label. */
+ p += devnamelen + 2; /* Skip to first label. */
while (*p) {
if (STRPREFIX(p, "bps=")) {
@@ -3554,25 +3552,13 @@ cleanup:
int qemuMonitorTextGetBlockIoThrottle(qemuMonitorPtr mon,
const char *device,
- virDomainBlockIoTuneInfoPtr reply,
- unsigned int flags)
+ virDomainBlockIoTuneInfoPtr reply)
{
- char *cmd = NULL;
char *result = NULL;
int ret = 0;
- const char *cmd_name = NULL;
+ const char *cmd_name = "info block";
- if (flags) {
- cmd_name = "info block";
- ret = virAsprintf(&cmd, "%s", cmd_name);
- }
-
- if (ret < 0) {
- virReportOOMError();
- return -1;
- }
-
- if (qemuMonitorHMPCommand(mon, cmd, &result) < 0) {
+ if (qemuMonitorHMPCommand(mon, cmd_name, &result) < 0) {
qemuReportError(VIR_ERR_INTERNAL_ERROR,
"%s", _("cannot run monitor command"));
ret = -1;
@@ -3589,8 +3575,6 @@ int
qemuMonitorTextGetBlockIoThrottle(qemuMonitorPtr mon,
ret = qemuMonitorTextParseBlockIoThrottle(result, device, reply);
cleanup:
- VIR_FREE(cmd);
VIR_FREE(result);
return ret;
}
-
diff --git i/src/qemu/qemu_monitor_text.h w/src/qemu/qemu_monitor_text.h
index 1c47d39..c3e97e0 100644
--- i/src/qemu/qemu_monitor_text.h
+++ w/src/qemu/qemu_monitor_text.h
@@ -250,12 +250,10 @@ int qemuMonitorTextOpenGraphics(qemuMonitorPtr mon,
int qemuMonitorTextSetBlockIoThrottle(qemuMonitorPtr mon,
const char *device,
- virDomainBlockIoTuneInfoPtr info,
- unsigned int flags);
+ virDomainBlockIoTuneInfoPtr info);
int qemuMonitorTextGetBlockIoThrottle(qemuMonitorPtr mon,
const char *device,
- virDomainBlockIoTuneInfoPtr reply,
- unsigned int flags);
+ virDomainBlockIoTuneInfoPtr reply);
#endif /* QEMU_MONITOR_TEXT_H */
--
Eric Blake eblake@xxxxxxxxxx +1-919-301-3266
Libvirt virtualization library http://libvirt.org
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list