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

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.