On 30/09/16 13:47, John Ferlan wrote:
>
>
> On 09/30/2016 07:08 AM, Erik Skultety wrote:
>> On 30/09/16 12:57, John Ferlan wrote:
>>>
>>>
>>> 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);
>>>
>>
>> Yeah, I also posted some suggestion in my original reply, have a look at
>> that, to make it short I think that construction like this might work as
>> well and is still slightly more readable:
>>
>> SET_IOTUNE_FIELD(total_bytes_sec, bytes, TOTAL_BYTES_SEC);
>> SET_IOTUNE_FIELD(read_bytes_sec, bytes, READ_BYTES_SEC);
>> SET_IOTUNE_FIELD(write_bytes_sec, bytes, WRITE_BYTES_SEC);
>>
>> I purposely got rid of the if-else-if construction altogether because
>> the compiler might actually optimize it, as I said, see my original
>> reply to this patch and tell me what you think.
>>
>
> So essentially I end up with two macros and 7 calls to those macros:
>
> #define SET_IOTUNE(FIELD, BOOL, CONST) \
> do { \
> info.FIELD = params->value.ul; \
> set_##BOOL = true; \
> if (virTypedParamsAddULLong(&eventParams, &eventNparams, \
> &eventMaxparams, \
> VIR_DOMAIN_TUNABLE_BLKDEV_##CONST, \
> param->value.ul) < 0) \
> goto endjob; \
> } while (0);
>
> #define SET_IOTUNE_FIELD(FIELD, BOOL, CONST) \
> if (STREQ(param->field, VIR_DOMAIN_BLOCK_IOTUNE_##CONST)) { \
> SET_IOTUNE(FIELD, BOOL, CONST); \
> } else if (STREQ(param->field, VIR_DOMAIN_BLOCK_IOTUNE_##CONST##_MAX)) { \
> SET_IOTUNE(FIELD##_max, BOOL##_max, CONST##_MAX); \
> } else if (STREQ(param->field, \
> VIR_DOMAIN_BLOCK_IOTUNE_##CONST##_MAX_LENGTH)) { \
> SET_IOTUNE(FIELD##_max_length, BOOL##_max_length, CONST##_MAX_LENGTH); \
> }
>
>
> SET_IOTUNE_FIELD(total_bytes_sec, bytes, TOTAL_BYTES_SEC);
> SET_IOTUNE_FIELD(read_bytes_sec, bytes, READ_BYTES_SEC);
> SET_IOTUNE_FIELD(write_bytes_sec, bytes, WRITE_BYTES_SEC);
> SET_IOTUNE_FIELD(total_iops_sec, iops, TOTAL_IOPS_SEC);
> SET_IOTUNE_FIELD(read_iops_sec, iops, READ_IOPS_SEC);
> SET_IOTUNE_FIELD(write_iops_sec, iops, WRITE_IOPS_SEC);
> if (STREQ(param->field, VIR_DOMAIN_BLOCK_IOTUNE_SIZE_IOPS_SEC))
> SET_IOTUNE(size_iops_sec, size_iops, SIZE_IOPS_SEC);
>
> #undef SET_IOTUNE
> #undef SET_IOTUNE_FIELD
>
>
Almost, but what I had in mind was to just keep one of the macros - be
it SET_IOTUNE or SET_IOTUNE_FIELD or whatever the naming is - and then
end up with a snippet like the one in the attached patch.
Erik
PS: Sorry for attaching a patch, but thunderbird's editor truly sucks
(pasting from vim is a total nightmare)...big time...and I really have
to switch to some normal mail client.
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 2b5b6fc..706b368 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -17321,6 +17321,19 @@ qemuDomainSetBlockIoTune(virDomainPtr dom,
VIR_DOMAIN_TUNABLE_BLKDEV_DISK, path) < 0)
goto endjob;
+#define SET_IOTUNE_FIELD(FIELD, BOOL, CONST) \
+ do { \
+ if (STREQ(param->field, VIR_DOMAIN_BLOCK_IOTUNE_##CONST)) { \
+ 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);
+
for (i = 0; i < nparams; i++) {
virTypedParameterPtr param = ¶ms[i];
@@ -17331,178 +17344,35 @@ 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;
- }
+ SET_IOTUNE_FIELD(total_bytes_sec, set_bytes, TOTAL_BYTES_SEC);
+ SET_IOTUNE_FIELD(read_bytes_sec, set_bytes, READ_BYTES_SEC);
+ SET_IOTUNE_FIELD(write_bytes_sec, set_bytes, WRITE_BYTES_SEC);
+ SET_IOTUNE_FIELD(total_iops_sec, set_iops, TOTAL_IOPS_SEC);
+ SET_IOTUNE_FIELD(read_iops_sec, set_iops, READ_IOPS_SEC);
+ SET_IOTUNE_FIELD(write_iops_sec, set_iops, WRITE_IOPS_SEC);
+ SET_IOTUNE_FIELD(total_bytes_sec_max, set_bytes_max, TOTAL_BYTES_SEC_MAX);
+ SET_IOTUNE_FIELD(read_bytes_sec_max, set_bytes_max, READ_BYTES_SEC_MAX);
+ SET_IOTUNE_FIELD(write_bytes_sec_max, set_bytes_max, WRITE_BYTES_SEC_MAX);
+ SET_IOTUNE_FIELD(total_iops_sec_max, set_iops_max, TOTAL_IOPS_SEC_MAX);
+ SET_IOTUNE_FIELD(read_iops_sec_max, set_iops_max, READ_IOPS_SEC_MAX);
+ SET_IOTUNE_FIELD(write_iops_sec_max, set_iops_max, WRITE_IOPS_SEC_MAX);
+ SET_IOTUNE_FIELD(total_bytes_sec_max_length, set_bytes_max_length,
+ TOTAL_BYTES_SEC_MAX_LENGTH);
+ SET_IOTUNE_FIELD(read_bytes_sec_max_length, set_bytes_max_length,
+ READ_BYTES_SEC_MAX_LENGTH);
+ SET_IOTUNE_FIELD(write_bytes_sec_max_length, set_bytes_max_length,
+ WRITE_BYTES_SEC_MAX_LENGTH);
+ SET_IOTUNE_FIELD(total_iops_sec_max_length, set_iops_max_length,
+ TOTAL_IOPS_SEC_MAX_LENGTH);
+ SET_IOTUNE_FIELD(read_iops_sec_max_length, set_iops_max_length,
+ READ_IOPS_SEC_MAX_LENGTH);
+ SET_IOTUNE_FIELD(write_iops_sec_max_length, set_iops_max_length,
+ WRITE_IOPS_SEC_MAX_LENGTH);
+ 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
if ((info.total_bytes_sec && info.read_bytes_sec) ||
(info.total_bytes_sec && info.write_bytes_sec)) {
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list