Re: [libvirt] [PATCH 3/6] qemu: Add support for parsing iotune group setting

On Mon, Oct 31, 2016 at 05:22:59PM -0400, John Ferlan wrote: > Add support to read/parse the iotune group setting for qemu. > > Signed-off-by: John Ferlan <jferlan(a)redhat.com&gt; > --- > include/libvirt/libvirt-domain.h | 8 +++++ > src/conf/domain_conf.c | 1 + > src/conf/domain_conf.h | 1 + > src/qemu/qemu_driver.c | 50 ++++++++++++++++++++++++--- > src/qemu/qemu_monitor.c | 2 ++ > src/qemu/qemu_monitor.h | 1 + > src/qemu/qemu_monitor_json.c | 21 ++++++++++-- > src/qemu/qemu_monitor_json.h | 1 + > tests/qemumonitorjsontest.c | 74 > +++++++++++++++++++++++++++++++--------- > 9 files changed, 134 insertions(+), 25 deletions(-) > > diff --git a/include/libvirt/libvirt-domain.h > b/include/libvirt/libvirt-domain.h > index 8c9876c..1212e5a 100644 > --- a/include/libvirt/libvirt-domain.h > +++ b/include/libvirt/libvirt-domain.h > @@ -3854,6 +3854,14 @@ typedef void > (*virConnectDomainEventJobCompletedCallback)(virConnectPtr conn, > # define VIR_DOMAIN_TUNABLE_BLKDEV_SIZE_IOPS_SEC > "blkdeviotune.size_iops_sec" > > /** > + * VIR_DOMAIN_TUNABLE_BLKDEV_GROUP_NAME: > + * > + * Macro represents the group name to be used, > + * as VIR_TYPED_PARAM_STRING. > + */ > +# define VIR_DOMAIN_TUNABLE_BLKDEV_GROUP_NAME "blkdeviotune.group_name" > + > +/** > * VIR_DOMAIN_TUNABLE_BLKDEV_TOTAL_BYTES_SEC_MAX_LENGTH: > * > * Macro represents the length in seconds allowed for a burst period > diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c > index 03506cb..f41a783 100644 > --- a/src/conf/domain_conf.c > +++ b/src/conf/domain_conf.c > @@ -1644,6 +1644,7 @@ virDomainDiskDefFree(virDomainDiskDefPtr def) > VIR_FREE(def->vendor); > VIR_FREE(def->product); > VIR_FREE(def->domain_name); > + VIR_FREE(def->blkdeviotune.group_name); > virDomainDeviceInfoClear(&def->info); > virObjectUnref(def->privateData); > > diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h > index 24aa79c..cb32685 100644 > --- a/src/conf/domain_conf.h > +++ b/src/conf/domain_conf.h > @@ -548,6 +548,7 @@ struct _virDomainBlockIoTuneInfo { > unsigned long long read_iops_sec_max; > unsigned long long write_iops_sec_max; > unsigned long long size_iops_sec; > + char *group_name; > unsigned long long total_bytes_sec_max_length; > unsigned long long read_bytes_sec_max_length; > unsigned long long write_bytes_sec_max_length; > diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c > index b8cec49..4af1cb0 100644 > --- a/src/qemu/qemu_driver.c > +++ b/src/qemu/qemu_driver.c > @@ -114,7 +114,8 @@ VIR_LOG_INIT("qemu.qemu_driver"); > > #define QEMU_NB_BLOCK_IO_TUNE_PARAM 6 > #define QEMU_NB_BLOCK_IO_TUNE_PARAM_MAX 13 > -#define QEMU_NB_BLOCK_IO_TUNE_PARAM_MAX_LENGTH 19 > +#define QEMU_NB_BLOCK_IO_TUNE_PARAM_GROUP 14 > +#define QEMU_NB_BLOCK_IO_TUNE_PARAM_MAX_LENGTH 20 > > #define QEMU_NB_NUMA_PARAM 2 > > @@ -17316,7 +17317,8 @@ > qemuDomainSetBlockIoTuneDefaults(virDomainBlockIoTuneInfoPtr newinfo, > bool set_iops_max, > bool set_size_iops, > bool set_bytes_max_length, > - bool set_iops_max_length) > + bool set_iops_max_length, > + bool set_group_name) > { > #define SET_IOTUNE_DEFAULTS(BOOL, > FIELD) \ > if (!BOOL) > { \ > @@ -17333,6 +17335,8 @@ > qemuDomainSetBlockIoTuneDefaults(virDomainBlockIoTuneInfoPtr newinfo, > > if (!set_size_iops) > newinfo->size_iops_sec = oldinfo->size_iops_sec; > + if (!set_group_name) > + VIR_STEAL_PTR(newinfo->group_name, oldinfo->group_name); > This changes the first structure, and because of that it can leak in qemuDomainSetBlockIoTune() where it is not free()'d (because it didn't have to be before). For example if qemuMonitorSetBlockIoThrottle() fails. It also looks like it might be uninitialized because instead of doing ifo = {0}; we call memset() lot of lines later. Just something you might need to wonder about when changing it.

> /* The length field is handled a bit differently. If not defined/set, > * QEMU will default these to 0 or 1 depending on whether > something in > @@ -17389,9 +17393,11 @@ qemuDomainSetBlockIoTune(virDomainPtr dom, > bool set_bytes_max = false; > bool set_iops_max = false; > bool set_size_iops = false; > + bool set_group_name = false; > bool set_bytes_max_length = false; > bool set_iops_max_length = false; > bool supportMaxOptions = true; > + bool supportGroupNameOption = true; > bool supportMaxLengthOptions = true; > virQEMUDriverConfigPtr cfg = NULL; > virObjectEventPtr event = NULL; > @@ -17428,6 +17434,8 @@ qemuDomainSetBlockIoTune(virDomainPtr dom, > VIR_TYPED_PARAM_ULLONG, > VIR_DOMAIN_BLOCK_IOTUNE_SIZE_IOPS_SEC, > VIR_TYPED_PARAM_ULLONG, > + VIR_DOMAIN_BLOCK_IOTUNE_GROUP_NAME, > + VIR_TYPED_PARAM_STRING, > > VIR_DOMAIN_BLOCK_IOTUNE_TOTAL_BYTES_SEC_MAX_LENGTH, > VIR_TYPED_PARAM_ULLONG, > > VIR_DOMAIN_BLOCK_IOTUNE_READ_BYTES_SEC_MAX_LENGTH, > @@ -17507,6 +17515,16 @@ qemuDomainSetBlockIoTune(virDomainPtr dom, > SET_IOTUNE_FIELD(write_iops_sec_max, set_iops_max, > WRITE_IOPS_SEC_MAX); > SET_IOTUNE_FIELD(size_iops_sec, set_size_iops, SIZE_IOPS_SEC); > + if (STREQ(param->field, VIR_DOMAIN_BLOCK_IOTUNE_GROUP_NAME)) { > + if (VIR_STRDUP(info.group_name, params->value.s) < 0) > + goto endjob; > + set_group_name = true; > + if (virTypedParamsAddString(&eventParams, &eventNparams, > + &eventMaxparams, > + > VIR_DOMAIN_TUNABLE_BLKDEV_GROUP_NAME, > + param->value.s) < 0) > + goto endjob; > + } > > SET_IOTUNE_FIELD(total_bytes_sec_max_length, > set_bytes_max_length, > TOTAL_BYTES_SEC_MAX_LENGTH); > @@ -17559,6 +17577,8 @@ qemuDomainSetBlockIoTune(virDomainPtr dom, > if (def) { > supportMaxOptions = virQEMUCapsGet(priv->qemuCaps, > QEMU_CAPS_DRIVE_IOTUNE_MAX); > + supportGroupNameOption = virQEMUCapsGet(priv->qemuCaps, > + > QEMU_CAPS_DRIVE_IOTUNE_GROUP); > supportMaxLengthOptions = > virQEMUCapsGet(priv->qemuCaps, > QEMU_CAPS_DRIVE_IOTUNE_MAX_LENGTH); > > @@ -17577,6 +17597,13 @@ qemuDomainSetBlockIoTune(virDomainPtr dom, > goto endjob; > } > > + if (!supportGroupNameOption && set_group_name) { > + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", > + _("the block I/O throttling group > parameter is not " > + "supported with this QEMU binary")); > + goto endjob; > + } > + > if (!supportMaxLengthOptions && > (set_iops_max_length || set_bytes_max_length)) { > virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", > @@ -17595,7 +17622,8 @@ qemuDomainSetBlockIoTune(virDomainPtr dom, > set_bytes, set_iops, > set_bytes_max, > set_iops_max, set_size_iops, > set_bytes_max_length, > - set_iops_max_length); > + set_iops_max_length, > + set_group_name); > > #define CHECK_MAX(val) \ > do { \ > @@ -17623,6 +17651,7 @@ qemuDomainSetBlockIoTune(virDomainPtr dom, > qemuDomainObjEnterMonitor(driver, vm); > ret = qemuMonitorSetBlockIoThrottle(priv->mon, device, > &info, supportMaxOptions, > + supportGroupNameOption, > supportMaxLengthOptions); > if (qemuDomainObjExitMonitor(driver, vm) < 0) > ret = -1; > @@ -17652,7 +17681,8 @@ qemuDomainSetBlockIoTune(virDomainPtr dom, > set_bytes, set_iops, > set_bytes_max, > set_iops_max, set_size_iops, > set_bytes_max_length, > - set_iops_max_length); > + set_iops_max_length, > + set_group_name); > conf_disk->blkdeviotune = info; > ret = virDomainSaveConfig(cfg->configDir, driver->caps, > persistentDef); > if (ret < 0) > @@ -17725,8 +17755,11 @@ qemuDomainGetBlockIoTune(virDomainPtr dom, > if (!virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DRIVE_IOTUNE_MAX)) > maxparams = QEMU_NB_BLOCK_IO_TUNE_PARAM; > else if (!virQEMUCapsGet(priv->qemuCaps, > - QEMU_CAPS_DRIVE_IOTUNE_MAX_LENGTH)) > + QEMU_CAPS_DRIVE_IOTUNE_GROUP)) > maxparams = QEMU_NB_BLOCK_IO_TUNE_PARAM_MAX; > + else if (!virQEMUCapsGet(priv->qemuCaps, > + QEMU_CAPS_DRIVE_IOTUNE_MAX_LENGTH)) > + maxparams = QEMU_NB_BLOCK_IO_TUNE_PARAM_GROUP; This won't work in a corner-case if there's a QEMU out there that has backported support for just some things out of the order in which they were introduced. I don't know why we don't just easily do: if (supportGroupNameOption) maxparams++; if (supportMaxLengthOptions) maxparams += 6; where the numbers can be macros as well, of course.

> } > > if (*nparams == 0) { > @@ -17790,6 +17823,13 @@ qemuDomainGetBlockIoTune(virDomainPtr dom, > > BLOCK_IOTUNE_ASSIGN(SIZE_IOPS_SEC, size_iops_sec); > > + if (*nparams < maxparams && > + virTypedParameterAssign(&params[(*nparams)++], > + VIR_DOMAIN_BLOCK_IOTUNE_GROUP_NAME, > + VIR_TYPED_PARAM_STRING, > + reply.group_name) < 0) > + goto endjob; > + As much as I'm looking at it, I still can't find why can't you use BLOCK_IOTUNE_ASSIGN. What's the reason for that?

> BLOCK_IOTUNE_ASSIGN(TOTAL_BYTES_SEC_MAX_LENGTH, > total_bytes_sec_max_length); > BLOCK_IOTUNE_ASSIGN(READ_BYTES_SEC_MAX_LENGTH, > read_bytes_sec_max_length); > BLOCK_IOTUNE_ASSIGN(WRITE_BYTES_SEC_MAX_LENGTH, > write_bytes_sec_max_length); > diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c > index a5e14b2..950d476 100644 > --- a/src/qemu/qemu_monitor.c > +++ b/src/qemu/qemu_monitor.c > @@ -3428,6 +3428,7 @@ qemuMonitorSetBlockIoThrottle(qemuMonitorPtr mon, > const char *device, > virDomainBlockIoTuneInfoPtr info, > bool supportMaxOptions, > + bool supportGroupNameOption, > bool supportMaxLengthOptions) > { > VIR_DEBUG("device=%p, info=%p", device, info); > @@ -3437,6 +3438,7 @@ qemuMonitorSetBlockIoThrottle(qemuMonitorPtr mon, > if (mon->json) > return qemuMonitorJSONSetBlockIoThrottle(mon, device, info, > supportMaxOptions, > + supportGroupNameOption, > > supportMaxLengthOptions); > else > return qemuMonitorTextSetBlockIoThrottle(mon, device, info); > diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h > index c3133c4..7476c11 100644 > --- a/src/qemu/qemu_monitor.h > +++ b/src/qemu/qemu_monitor.h > @@ -877,6 +877,7 @@ int qemuMonitorSetBlockIoThrottle(qemuMonitorPtr mon, > const char *device, > virDomainBlockIoTuneInfoPtr info, > bool supportMaxOptions, > + bool supportGroupNameOption, > bool supportMaxLengthOptions); > > int qemuMonitorGetBlockIoThrottle(qemuMonitorPtr mon, > diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c > index 6c13832..7a08253 100644 > --- a/src/qemu/qemu_monitor_json.c > +++ b/src/qemu/qemu_monitor_json.c > @@ -4524,6 +4524,7 @@ > qemuMonitorJSONBlockIoThrottleInfo(virJSONValuePtr result, > virJSONValuePtr temp_dev = virJSONValueArrayGet(io_throttle, i); > virJSONValuePtr inserted; > const char *current_dev; > + const char *group_name; > > if (!temp_dev || temp_dev->type != VIR_JSON_TYPE_OBJECT) { > virReportError(VIR_ERR_INTERNAL_ERROR, "%s", > @@ -4549,7 +4550,6 @@ > qemuMonitorJSONBlockIoThrottleInfo(virJSONValuePtr result, > "was not in expected format")); > goto cleanup; > } > - > GET_THROTTLE_STATS("bps", total_bytes_sec); > GET_THROTTLE_STATS("bps_rd", read_bytes_sec); > GET_THROTTLE_STATS("bps_wr", write_bytes_sec); > @@ -4563,6 +4563,12 @@ > qemuMonitorJSONBlockIoThrottleInfo(virJSONValuePtr result, > GET_THROTTLE_STATS_OPTIONAL("iops_rd_max", read_iops_sec_max); > GET_THROTTLE_STATS_OPTIONAL("iops_wr_max", write_iops_sec_max); > GET_THROTTLE_STATS_OPTIONAL("iops_size", size_iops_sec); > + > + if ((group_name = virJSONValueObjectGetString(inserted, > "group"))) { > + if (VIR_STRDUP(reply->group_name, group_name) < 0) > + goto cleanup; > + } If you want to save some indentation, VIR_STRDUP will handle NULLs nicely.

> + > GET_THROTTLE_STATS_OPTIONAL("bps_max_length", > total_bytes_sec_max_length); > GET_THROTTLE_STATS_OPTIONAL("bps_rd_max_length", > read_bytes_sec_max_length); > GET_THROTTLE_STATS_OPTIONAL("bps_wr_max_length", > write_bytes_sec_max_length); > @@ -4591,17 +4597,23 @@ int > qemuMonitorJSONSetBlockIoThrottle(qemuMonitorPtr mon, > const char *device, > virDomainBlockIoTuneInfoPtr info, > bool supportMaxOptions, > + bool supportGroupNameOption, > bool supportMaxLengthOptions) > { > int ret = -1; > virJSONValuePtr cmd = NULL; > virJSONValuePtr result = NULL; > + char *group_name = NULL; > + > + if (supportGroupNameOption && VIR_STRDUP(group_name, > info->group_name) < 0) > + return -1; What's the reason for this? qemuMonitorJSONMakeCommand() doesn't steals the pointer.

> > /* The qemu capability check has already been made in > * qemuDomainSetBlockIoTune. NB, once a NULL is found in > * the sequence, qemuMonitorJSONMakeCommand will stop. So > * let's make use of that when !supportMaxOptions and > - * similarly when !supportMaxLengthOptions */ > + * similarly when !supportGroupNameOption as well as when > + * when !supportMaxLengthOptions */ > cmd = qemuMonitorJSONMakeCommand("block_set_io_throttle", > "s:device", device, > "U:bps", info->total_bytes_sec, > @@ -4618,6 +4630,8 @@ int > qemuMonitorJSONSetBlockIoThrottle(qemuMonitorPtr mon, > "U:iops_rd_max", > info->read_iops_sec_max, > "U:iops_wr_max", > info->write_iops_sec_max, > "U:iops_size", info->size_iops_sec, > + !supportGroupNameOption ? NULL : > + "s:group", group_name, > !supportMaxLengthOptions ? NULL : > "P:bps_max_length", > info->total_bytes_sec_max_length, Same applies here with the back-porting.

> @@ -4633,7 +4647,7 @@ int > qemuMonitorJSONSetBlockIoThrottle(qemuMonitorPtr mon, > info->write_iops_sec_max_length, > NULL); > if (!cmd) > - return -1; > + goto cleanup; > > if (qemuMonitorJSONCommand(mon, cmd, &result) < 0) > goto cleanup; > @@ -4657,6 +4671,7 @@ int > qemuMonitorJSONSetBlockIoThrottle(qemuMonitorPtr mon, > > ret = 0; > cleanup: > + VIR_FREE(group_name); > virJSONValueFree(cmd); > virJSONValueFree(result); > return ret; These two hunks can be skipped if there's no need for the strdup.

> diff --git a/src/qemu/qemu_monitor_json.h b/src/qemu/qemu_monitor_json.h > index 77b2e02..55bdfe8 100644 > --- a/src/qemu/qemu_monitor_json.h > +++ b/src/qemu/qemu_monitor_json.h > @@ -328,6 +328,7 @@ int > qemuMonitorJSONSetBlockIoThrottle(qemuMonitorPtr mon, > const char *device, > virDomainBlockIoTuneInfoPtr info, > bool supportMaxOptions, > + bool supportGroupNameOption, > bool supportMaxLengthOptions); > > int qemuMonitorJSONGetBlockIoThrottle(qemuMonitorPtr mon, > diff --git a/tests/qemumonitorjsontest.c b/tests/qemumonitorjsontest.c > index d8fd958..19bfc2a 100644 > --- a/tests/qemumonitorjsontest.c > +++ b/tests/qemumonitorjsontest.c > @@ -67,12 +67,13 @@ const char *queryBlockReply = > " \"iops_rd_max\": 11," > " \"iops_wr_max\": 12," > " \"iops_size\": 13," > -" \"bps_max_length\": 14," > -" \"bps_rd_max_length\": 15," > -" \"bps_wr_max_length\": 16," > -" \"iops_max_length\": 17," > -" \"iops_rd_max_length\": 18," > -" \"iops_wr_max_length\": 19," > +" \"group\": \"group14\"," > +" \"bps_max_length\": 15," > +" \"bps_rd_max_length\": 16," > +" \"bps_wr_max_length\": 17," > +" \"iops_max_length\": 18," > +" \"iops_rd_max_length\": 19," > +" \"iops_wr_max_length\": 20," > " \"file\": \"/home/zippy/work/tmp/gentoo.qcow2\"," > " \"encryption_key_missing\": false" > " }," > @@ -2002,7 +2003,9 @@ > testQemuMonitorJSONqemuMonitorJSONSetBlockIoThrottle(const void *data) > if (!test) > return -1; > > - expectedInfo = (virDomainBlockIoTuneInfo) {1, 2, 3, 4, 5, 6, 7, > 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19}; > + expectedInfo = (virDomainBlockIoTuneInfo) {1, 2, 3, 4, 5, 6, 7, > 8, 9, 10, 11, 12, 13, NULL, 15, 16, 17, 18, 19, 20}; > + if (VIR_STRDUP(expectedInfo.group_name, "group14") < 0) > + return -1;

> > if (qemuMonitorTestAddItem(test, "query-block", queryBlockReply) < > 0 || > qemuMonitorTestAddItemParams(test, "block_set_io_throttle", > @@ -2014,12 +2017,13 @@ > testQemuMonitorJSONqemuMonitorJSONSetBlockIoThrottle(const void *data) > "bps_wr_max", "9", > "iops_max", "10", "iops_rd_max", > "11", > "iops_wr_max", "12", "iops_size", > "13", > - "bps_max_length", "14", > - "bps_rd_max_length", "15", > - "bps_wr_max_length", "16", > - "iops_max_length", "17", > - "iops_rd_max_length", "18", > - "iops_wr_max_length", "19", > + "group", "\"group14\"", > + "bps_max_length", "15", > + "bps_rd_max_length", "16", > + "bps_wr_max_length", "17", > + "iops_max_length", "18", > + "iops_rd_max_length", "19", > + "iops_wr_max_length", "20", > NULL, NULL) < 0) > goto cleanup; > > @@ -2027,19 +2031,55 @@ > testQemuMonitorJSONqemuMonitorJSONSetBlockIoThrottle(const void *data) > "drive-virtio-disk0", &info) > < 0) > goto cleanup; > > - if (memcmp(&info, &expectedInfo, sizeof(info)) != 0) { > - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", > - "Invalid @info"); > +#define VALIDATE_IOTUNE(field) \ > + if (info.field != expectedInfo.field) { \ > + virReportError(VIR_ERR_INTERNAL_ERROR, \ > + "info.%s=%llu != expected=%llu", \ > + #field, info.field, expectedInfo.field); \ > + goto cleanup; \ > + } \ > + if (info.field##_max != expectedInfo.field##_max) { \ > + virReportError(VIR_ERR_INTERNAL_ERROR, \ > + "info.%s_max=%llu != expected=%llu", \ > + #field, info.field##_max, > expectedInfo.field##_max); \ > + goto cleanup; \ > + } \ > + if (info.field##_max_length != expectedInfo.field##_max_length) { \ > + virReportError(VIR_ERR_INTERNAL_ERROR, \ > + "info.%s_max_length=%llu != expected=%llu", \ > + #field, info.field##_max_length, \ > + expectedInfo.field##_max_length); \ > + goto cleanup; \ > + } > + VALIDATE_IOTUNE(total_bytes_sec); > + VALIDATE_IOTUNE(read_bytes_sec); > + VALIDATE_IOTUNE(write_bytes_sec); > + VALIDATE_IOTUNE(total_iops_sec); > + VALIDATE_IOTUNE(read_iops_sec); > + VALIDATE_IOTUNE(write_iops_sec); > + if (info.size_iops_sec != expectedInfo.size_iops_sec) { > + virReportError(VIR_ERR_INTERNAL_ERROR, > + "info.size_iops_sec=%llu != expected=%llu", > + info.size_iops_sec, expectedInfo.size_iops_sec); > + goto cleanup; > + } > + if (STRNEQ(info.group_name, expectedInfo.group_name)) { > + virReportError(VIR_ERR_INTERNAL_ERROR, > + "info.group_name=%s != expected=%s", > + info.group_name, expectedInfo.group_name); > goto cleanup; > } > +#undef VALIDATE_IOTUNE > I don't feel like this above is cleaner than comparing the strings, clearing them out and then keeping the memcmp(). Well, at least we get better error. But it could be in its own function.

> if > (qemuMonitorJSONSetBlockIoThrottle(qemuMonitorTestGetMonitor(test), > "drive-virtio-disk1", &info, > true, > - true) < 0) > + true, true) < 0) > goto cleanup; > > ret = 0; > cleanup: > + VIR_FREE(info.group_name); > + VIR_FREE(expectedInfo.group_name); > qemuMonitorTestFree(test); > return ret; > } > -- > 2.7.4 > > -- > libvir-list mailing list > libvir-list(a)redhat.com > https://www.redhat.com/mailman/listinfo/libvir-list