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@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