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

John Ferlan jferlan at redhat.com
Mon Nov 7 21:39:04 UTC 2016



On 11/02/2016 10:24 AM, Martin Kletzander wrote:
> 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 at redhat.com>
>> ---
>> 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.
> 

Good catch on the leak...  'info' is filled in from parameters provided
via 'params', then any values currently set in the saved domain values
(disk->blkdeviotune and conf_disk->blkdeviotune) are defaulted to the
'info' values since shortly after the setting of the default values, the
'info' value is assigned into the saved domain value location again via
the structure assignment...

So, I'll add a couple of "info.group_name = NULL" after the structure
assignments and a VIR_FREE(info.group_name)

The memset(info, 0, sizeof(info)); is done before any call to set the
defaults, so unless I'm missing something subtle that you're pointing
out, I'll assume the "either" info = {0} or memset would do the proper
initialization. If you'd prefer that the {0} is used, that's fine I can
change it. Since the memset had been used when I started this exercise,
I didn't change 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.
> 

We start with 20 (assume everything is possible) and then reset max to
some lower value based upon what capabilities exist assuming that no one
would really backport a 2.6 feature (_LENGTH) into 2.4 or a 2.4
(group_name) feature into 1.7 or a 1.7 feature (_MAX) into something
before 1.7. Furthermore the _MAX and _LENGTH are "tied" together. That
is there's no way for _LENGTH to exist without _MAX.  The only oddball
is the GROUP_NAME, but it's incestuously related as well ;-)

I'll make a separate patch before this one to address this.

>>     }
>>
>>     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?
> 

The macro uses VIR_TYPED_PARAM_ULLONG while the above uses
VIR_TYPED_PARAM_STRING

>>     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.
> 

OK...  I was blindly copying (to a degree) the _OPTIONAL macro logic.

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

This is a relic of how I initially approached adding group_name support.
I was going to go with a number, then generate a group_name based on
that number and a self defined prefix (e.g. a virAsprintf(group_name,
"libvirt_iotune_group", info->group_num);) - that persisted in patch 4
were you can see the tests used that...

Anyway, I'll undo that here...

>>
>>     /* 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.
> 

Similar to the previous, it does exist this way currently, but I'll make
a separate patch before this one for this as well to address the concern.


>> @@ -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.
> 

yep.

>> 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;

FWIW: From your followup...  Using the numbers as is also makes it
easier to do the "math" of the maxparams logic.

>>
>>     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.
> 

I'll make a separate function. I prefer the changes because they let one
know which parameter was incorrect rather than just a "Invalid @info".


Tks -

John
>>     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 at redhat.com
>> https://www.redhat.com/mailman/listinfo/libvir-list




More information about the libvir-list mailing list