Re: [libvirt] [PATCH 08.5/12] qemu: Create a set of macros to handle setting block iotune values

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&gt; >>>>> --- >>>>> >>>>> 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.