On 09/30/2016 04:43 AM, Erik Skultety wrote:
> On 28/09/16 22:27, John Ferlan wrote:
>> Rework the code in a set of 3 macros that will use the "base" of
>> 'bytes' or 'iops' and build up the prefixes of 'total_', 'read_',
>> and 'write_' before adding the postfixes of '_sec', '_sec_max',
>> and '_sec_max_length' and making the param->field comparison and
>> adding of the field.
>>
>> Signed-off-by: John Ferlan <jferlan@xxxxxxxxxx>
>> ---
>>
>> NB: Hopefully this applies - my branch is based off of the git head
>> which I refreshed prior to sending the patch
>>
>> Since I missed 2.3, I figured why not try to make the change.
>>
>> src/qemu/qemu_driver.c | 216 +++++++++++--------------------------------------
>> 1 file changed, 45 insertions(+), 171 deletions(-)
>>
>> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
>> index 2b5b6fc..cbf9483 100644
>> --- a/src/qemu/qemu_driver.c
>> +++ b/src/qemu/qemu_driver.c
>> @@ -17321,6 +17321,41 @@ qemuDomainSetBlockIoTune(virDomainPtr dom,
>> VIR_DOMAIN_TUNABLE_BLKDEV_DISK, path) < 0)
>> goto endjob;
>>
>> +#define SET_IOTUNE_FIELD(FIELD, BOOL, CONST) \
>> + do { \
>> + info.FIELD = params->value.ul; \
>> + BOOL = true; \
>> + if (virTypedParamsAddULLong(&eventParams, &eventNparams, \
>> + &eventMaxparams, \
>> + VIR_DOMAIN_TUNABLE_BLKDEV_##CONST, \
>> + param->value.ul) < 0) \
>> + goto endjob; \
>> + } while (0);
>> +
>
> While I totally support ^^this kind of macro-based code cleanup,
>
Ironically it's what I started with, but still seeing:
if (STREQ(param->field, VIR_DOMAIN_BLOCK_IOTUNE_TOTAL_BYTES_SEC))
SET_IOTUNE_FIELD(total_bytes_sec, bytes, TOTAL_BYTES_SEC);
else if (STREQ(param->field,
VIR_DOMAIN_BLOCK_IOTUNE_READ_BYTES_SEC))
SET_IOTUNE_FIELD(read_bytes_sec, bytes, READ_BYTES_SEC);
else if (STREQ(param->field,
VIR_DOMAIN_BLOCK_IOTUNE_WRITE_BYTES_SEC))
SET_IOTUNE_FIELD(write_bytes_sec, bytes, WRITE_BYTES_SEC);
felt like only a slight improvement to visibility. I agree what I ended
up with I agree is overkill with respect to needing to stop, think, and
map out what the macros do. I also know I went from if - else if - else
if - else if (etc) to groups of 3 if - else if - else if. I was going to
mention it, but figured it'd be somewhat obvious.
But no harm in seeing what others thought - it's easy enough to go back
to ^^this^^ version... BTW: eventually the SET_IOTUNE_FIELD spread
across two lines especially with the _MAX_LENGTH parameters.
Thanks for the feedback - I'll post a v2 in a little while...
John
>> +/* Taking a prefix build by CHECK_SET_IOTUNE, add the postfix of "_sec",
>> + * "_sec_max", and "_sec_max_length" to each of the variables in order to
>> + * in order to check each of the ways one of these can be used. The boolean
>> + * doesn't care if "total_", "read_", or "write_" is the prefix. */
>> +#define CHECK_IOTUNE(FIELD, BOOL, CONST) \
>> + if (STREQ(param->field, VIR_DOMAIN_BLOCK_IOTUNE_##CONST##_SEC)) { \
>> + SET_IOTUNE_FIELD(FIELD##_sec, set_##BOOL, CONST##_SEC); \
>> + } else if (STREQ(param->field, \
>> + VIR_DOMAIN_BLOCK_IOTUNE_##CONST##_SEC_MAX)) { \
>> + SET_IOTUNE_FIELD(FIELD##_sec_max, set_##BOOL##_max, CONST##_SEC_MAX); \
>> + } else if (STREQ(param->field, \
>> + VIR_DOMAIN_BLOCK_IOTUNE_##CONST##_SEC_MAX_LENGTH)) { \
>> + SET_IOTUNE_FIELD(FIELD##_sec_max_length, set_##BOOL##_max_length, \
>> + CONST##_SEC_MAX_LENGTH); \
>> + }
>> +
>> +/* For "bytes" and "tune", prepend the "total_", "read_", and "write_" onto
>> + * the field/const names, but also passing the "base" field to be used as the
>> + * basis of the boolean. */
>> +#define CHECK_SET_IOTUNE(FIELD, CONST) \
>> + CHECK_IOTUNE(total_##FIELD, FIELD, TOTAL_##CONST); \
>> + CHECK_IOTUNE(read_##FIELD, FIELD, READ_##CONST); \
>> + CHECK_IOTUNE(write_##FIELD, FIELD, WRITE_##CONST);
>> +
>
> I'm rather skeptic about ^^this because IMHO it's just too much in terms
> of code duplication cleanup. I think that in this case (like in many
> other cases) we should try to find the sweet spot between line removal
> and the time programmer spends reading these macros. We could use
> exactly the same approach as we do with STORE_MEM_RECORD,
> QEMU_ADD_NET_PARAM, PARSE_MEMTUNE_PARAM. Now, I know that in the
> original hunk below was a massive if-else if block which compared to a
> bunch of ifs like these:
>
> SET_IOTUNE_FIELD(ARG1,ARG2,ARG3);
> SET_IOTUNE_FIELD(ARG1,ARG2,ARG3);
> SET_IOTUNE_FIELD(ARG1,ARG2,ARG3);
>
> would prevent testing of any extra conditions once a single condition
> has been satisfied. But I believe that, provided that param->field does
> not change within any of the 'if' blocks (which it doesn't), the
> compiler would optimize the two example hunks below and produce an
> identical assembly for both.
>
> if (foo == 0)
> bar0();
> else if (foo == 1)
> bar1();
> else if (foo == 2)
> bar2();
> ##################
> if (foo == 0)
> bar0();
> if (foo == 1)
> bar1();
> if (foo == 2)
> bar2();
>
> Erik
>
>> for (i = 0; i < nparams; i++) {
>> virTypedParameterPtr param = ¶ms[i];
>>
>> @@ -17331,178 +17366,17 @@ qemuDomainSetBlockIoTune(virDomainPtr dom,
>> goto endjob;
>> }
>>
>> - if (STREQ(param->field, VIR_DOMAIN_BLOCK_IOTUNE_TOTAL_BYTES_SEC)) {
>> - info.total_bytes_sec = param->value.ul;
>> - set_bytes = true;
>> - if (virTypedParamsAddULLong(&eventParams, &eventNparams,
>> - &eventMaxparams,
>> - VIR_DOMAIN_TUNABLE_BLKDEV_TOTAL_BYTES_SEC,
>> - param->value.ul) < 0)
>> - goto endjob;
>> - } else if (STREQ(param->field,
>> - VIR_DOMAIN_BLOCK_IOTUNE_READ_BYTES_SEC)) {
>> - info.read_bytes_sec = param->value.ul;
>> - set_bytes = true;
>> - if (virTypedParamsAddULLong(&eventParams, &eventNparams,
>> - &eventMaxparams,
>> - VIR_DOMAIN_TUNABLE_BLKDEV_READ_BYTES_SEC,
>> - param->value.ul) < 0)
>> - goto endjob;
>> - } else if (STREQ(param->field,
>> - VIR_DOMAIN_BLOCK_IOTUNE_WRITE_BYTES_SEC)) {
>> - info.write_bytes_sec = param->value.ul;
>> - set_bytes = true;
>> - if (virTypedParamsAddULLong(&eventParams, &eventNparams,
>> - &eventMaxparams,
>> - VIR_DOMAIN_TUNABLE_BLKDEV_WRITE_BYTES_SEC,
>> - param->value.ul) < 0)
>> - goto endjob;
>> - } else if (STREQ(param->field,
>> - VIR_DOMAIN_BLOCK_IOTUNE_TOTAL_IOPS_SEC)) {
>> - info.total_iops_sec = param->value.ul;
>> - set_iops = true;
>> - if (virTypedParamsAddULLong(&eventParams, &eventNparams,
>> - &eventMaxparams,
>> - VIR_DOMAIN_TUNABLE_BLKDEV_TOTAL_IOPS_SEC,
>> - param->value.ul) < 0)
>> - goto endjob;
>> - } else if (STREQ(param->field,
>> - VIR_DOMAIN_BLOCK_IOTUNE_READ_IOPS_SEC)) {
>> - info.read_iops_sec = param->value.ul;
>> - set_iops = true;
>> - if (virTypedParamsAddULLong(&eventParams, &eventNparams,
>> - &eventMaxparams,
>> - VIR_DOMAIN_TUNABLE_BLKDEV_READ_IOPS_SEC,
>> - param->value.ul) < 0)
>> - goto endjob;
>> - } else if (STREQ(param->field,
>> - VIR_DOMAIN_BLOCK_IOTUNE_WRITE_IOPS_SEC)) {
>> - info.write_iops_sec = param->value.ul;
>> - set_iops = true;
>> - if (virTypedParamsAddULLong(&eventParams, &eventNparams,
>> - &eventMaxparams,
>> - VIR_DOMAIN_TUNABLE_BLKDEV_WRITE_IOPS_SEC,
>> - param->value.ul) < 0)
>> - goto endjob;
>> - } else if (STREQ(param->field,
>> - VIR_DOMAIN_BLOCK_IOTUNE_TOTAL_BYTES_SEC_MAX)) {
>> - info.total_bytes_sec_max = param->value.ul;
>> - set_bytes_max = true;
>> - if (virTypedParamsAddULLong(&eventParams, &eventNparams,
>> - &eventMaxparams,
>> - VIR_DOMAIN_TUNABLE_BLKDEV_TOTAL_BYTES_SEC_MAX,
>> - param->value.ul) < 0)
>> - goto endjob;
>> - } else if (STREQ(param->field,
>> - VIR_DOMAIN_BLOCK_IOTUNE_READ_BYTES_SEC_MAX)) {
>> - info.read_bytes_sec_max = param->value.ul;
>> - set_bytes_max = true;
>> - if (virTypedParamsAddULLong(&eventParams, &eventNparams,
>> - &eventMaxparams,
>> - VIR_DOMAIN_TUNABLE_BLKDEV_READ_BYTES_SEC_MAX,
>> - param->value.ul) < 0)
>> - goto endjob;
>> - } else if (STREQ(param->field,
>> - VIR_DOMAIN_BLOCK_IOTUNE_WRITE_BYTES_SEC_MAX)) {
>> - info.write_bytes_sec_max = param->value.ul;
>> - set_bytes_max = true;
>> - if (virTypedParamsAddULLong(&eventParams, &eventNparams,
>> - &eventMaxparams,
>> - VIR_DOMAIN_TUNABLE_BLKDEV_WRITE_BYTES_SEC_MAX,
>> - param->value.ul) < 0)
>> - goto endjob;
>> - } else if (STREQ(param->field,
>> - VIR_DOMAIN_BLOCK_IOTUNE_TOTAL_IOPS_SEC_MAX)) {
>> - info.total_iops_sec_max = param->value.ul;
>> - set_iops_max = true;
>> - if (virTypedParamsAddULLong(&eventParams, &eventNparams,
>> - &eventMaxparams,
>> - VIR_DOMAIN_TUNABLE_BLKDEV_TOTAL_IOPS_SEC_MAX,
>> - param->value.ul) < 0)
>> - goto endjob;
>> - } else if (STREQ(param->field,
>> - VIR_DOMAIN_BLOCK_IOTUNE_READ_IOPS_SEC_MAX)) {
>> - info.read_iops_sec_max = param->value.ul;
>> - set_iops_max = true;
>> - if (virTypedParamsAddULLong(&eventParams, &eventNparams,
>> - &eventMaxparams,
>> - VIR_DOMAIN_TUNABLE_BLKDEV_READ_IOPS_SEC_MAX,
>> - param->value.ul) < 0)
>> - goto endjob;
>> - } else if (STREQ(param->field,
>> - VIR_DOMAIN_BLOCK_IOTUNE_WRITE_IOPS_SEC_MAX)) {
>> - info.write_iops_sec_max = param->value.ul;
>> - set_iops_max = true;
>> - if (virTypedParamsAddULLong(&eventParams, &eventNparams,
>> - &eventMaxparams,
>> - VIR_DOMAIN_TUNABLE_BLKDEV_WRITE_IOPS_SEC_MAX,
>> - param->value.ul) < 0)
>> - goto endjob;
>> - } else if (STREQ(param->field,
>> - VIR_DOMAIN_BLOCK_IOTUNE_SIZE_IOPS_SEC)) {
>> - info.size_iops_sec = param->value.ul;
>> - set_size_iops = true;
>> - if (virTypedParamsAddULLong(&eventParams, &eventNparams,
>> - &eventMaxparams,
>> - VIR_DOMAIN_TUNABLE_BLKDEV_SIZE_IOPS_SEC,
>> - param->value.ul) < 0)
>> - goto endjob;
>> - } else if (STREQ(param->field,
>> - VIR_DOMAIN_BLOCK_IOTUNE_TOTAL_BYTES_SEC_MAX_LENGTH)) {
>> - info.total_bytes_sec_max_length = param->value.ul;
>> - set_bytes_max_length = true;
>> - if (virTypedParamsAddULLong(&eventParams, &eventNparams,
>> - &eventMaxparams,
>> - VIR_DOMAIN_TUNABLE_BLKDEV_TOTAL_BYTES_SEC_MAX_LENGTH,
>> - param->value.ul) < 0)
>> - goto endjob;
>> - } else if (STREQ(param->field,
>> - VIR_DOMAIN_BLOCK_IOTUNE_READ_BYTES_SEC_MAX_LENGTH)) {
>> - info.read_bytes_sec_max_length = param->value.ul;
>> - set_bytes_max_length = true;
>> - if (virTypedParamsAddULLong(&eventParams, &eventNparams,
>> - &eventMaxparams,
>> - VIR_DOMAIN_TUNABLE_BLKDEV_READ_BYTES_SEC_MAX_LENGTH,
>> - param->value.ul) < 0)
>> - goto endjob;
>> - } else if (STREQ(param->field,
>> - VIR_DOMAIN_BLOCK_IOTUNE_WRITE_BYTES_SEC_MAX_LENGTH)) {
>> - info.write_bytes_sec_max_length = param->value.ul;
>> - set_bytes_max_length = true;
>> - if (virTypedParamsAddULLong(&eventParams, &eventNparams,
>> - &eventMaxparams,
>> - VIR_DOMAIN_TUNABLE_BLKDEV_WRITE_BYTES_SEC_MAX_LENGTH,
>> - param->value.ul) < 0)
>> - goto endjob;
>> - } else if (STREQ(param->field,
>> - VIR_DOMAIN_BLOCK_IOTUNE_TOTAL_IOPS_SEC_MAX_LENGTH)) {
>> - info.total_iops_sec_max_length = param->value.ul;
>> - set_iops_max_length = true;
>> - if (virTypedParamsAddULLong(&eventParams, &eventNparams,
>> - &eventMaxparams,
>> - VIR_DOMAIN_TUNABLE_BLKDEV_TOTAL_IOPS_SEC_MAX_LENGTH,
>> - param->value.ul) < 0)
>> - goto endjob;
>> - } else if (STREQ(param->field,
>> - VIR_DOMAIN_BLOCK_IOTUNE_READ_IOPS_SEC_MAX_LENGTH)) {
>> - info.read_iops_sec_max_length = param->value.ul;
>> - set_iops_max_length = true;
>> - if (virTypedParamsAddULLong(&eventParams, &eventNparams,
>> - &eventMaxparams,
>> - VIR_DOMAIN_TUNABLE_BLKDEV_READ_IOPS_SEC_MAX_LENGTH,
>> - param->value.ul) < 0)
>> - goto endjob;
>> - } else if (STREQ(param->field,
>> - VIR_DOMAIN_BLOCK_IOTUNE_WRITE_IOPS_SEC_MAX_LENGTH)) {
>> - info.write_iops_sec_max_length = param->value.ul;
>> - set_iops_max_length = true;
>> - if (virTypedParamsAddULLong(&eventParams, &eventNparams,
>> - &eventMaxparams,
>> - VIR_DOMAIN_TUNABLE_BLKDEV_WRITE_IOPS_SEC_MAX_LENGTH,
>> - param->value.ul) < 0)
>> - goto endjob;
>> - }
>> + /* Use the macros to check the different fields that can be set for
>> + * bytes and iops. In the end 18 different combinations are tried. */
>> + CHECK_SET_IOTUNE(bytes, BYTES);
>> + CHECK_SET_IOTUNE(iops, IOPS);
>> + if (STREQ(param->field, VIR_DOMAIN_BLOCK_IOTUNE_SIZE_IOPS_SEC))
>> + SET_IOTUNE_FIELD(size_iops_sec, set_size_iops, SIZE_IOPS_SEC);
>> +
>> }
>> +#undef SET_IOTUNE_FIELD
>> +#undef CHECK_IOTUNE
>> +#undef CHECK_SET_IOTUNE
>>
>> if ((info.total_bytes_sec && info.read_bytes_sec) ||
>> (info.total_bytes_sec && info.write_bytes_sec)) {
>>
>
>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list