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

On 10/03/2016 03:56 AM, Erik Skultety wrote: > 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. > If you 'temporarily' turn off line wrapping for t-bird, then cut-n-paste isn't too bad, but attaching a patch was fine.

Suffice to say what you propose is where I initially started. Then I looked at the extended repetition of lines where the only difference was "_max"/"_MAX" and "_max_length"/"_MAX_LENGTH" and felt it was better to further collapse the way I did... In any case, see patch 10 for another instance of "parsing" 2 things at a time (PARSE_IOTUNE) and patch 11 for adding the _max_length to that. In fact the patch 11 shows my entrails with the commented out macros (since removed from my branch). I chose not do the same in patch 4 (IOTUNE_ADD) or patch 9 (FORMAT_IOTUNE) because I didn't want to mess with the .args or .xml output "adjustments" (e.g. output files). Personally I'd rather see the 3 variables close to each other rather than hunting through the various output trying to see which are set. Way too similarity that can "fool" the eyes. I'd prefer to keep the version of the macro I have since it doesn't affect the output/visible order. John

-- libvir-list mailing list libvir-list(a)redhat.com https://www.redhat.com/mailman/listinfo/libvir-list