2:56 a.m.
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(a)redhat.com>
>>>> ---
>>>>
>>>> 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.