Re: [libvirt] [PATCH 2/2] test_driver: implement virDomainSetBlockIoTune

On Sun, Aug 4, 2019 at 6:32 PM Erik Skultety <eskultet(a)redhat.com&gt; wrote: > > On Sat, Jul 27, 2019 at 05:04:38PM +0200, Ilias Stamatis wrote: > > Signed-off-by: Ilias Stamatis <stamatis.iliass(a)gmail.com&gt; > > --- > > > > I deliberately omitted the logic from qemuDomainSetBlockIoTuneDefaults > > in order to leave the function simpler. I think that in the case of the > > test driver it is ok. > > > > After all, how we handle the parameters it is supposed to be > > hypervisor-specific. > > > > src/test/test_driver.c | 196 +++++++++++++++++++++++++++++++++++++++++ > > 1 file changed, 196 insertions(+) > > > > diff --git a/src/test/test_driver.c b/src/test/test_driver.c > > index 9f4e255e35..3272ecf4b7 100755 > > --- a/src/test/test_driver.c > > +++ b/src/test/test_driver.c > > @@ -3502,6 +3502,201 @@ testDomainGetInterfaceParameters(virDomainPtr dom, > > } > > > > > > +static int > > +testDomainSetBlockIoTune(virDomainPtr dom, > > + const char *path, > > + virTypedParameterPtr params, > > + int nparams, > > + unsigned int flags) > > +{ > > + virDomainObjPtr vm = NULL; > > + virDomainDefPtr def = NULL; > > + virDomainBlockIoTuneInfo info = {0}; > > + virDomainDiskDefPtr conf_disk = NULL; > > + virTypedParameterPtr eventParams = NULL; > > + int eventNparams = 0; > > + int eventMaxparams = 0; > > + size_t i; > > + int ret = -1; > > + > > + virCheckFlags(VIR_DOMAIN_AFFECT_LIVE | > > + VIR_DOMAIN_AFFECT_CONFIG, -1); > > + > > + if (virTypedParamsValidate(params, nparams, > > + VIR_DOMAIN_BLOCK_IOTUNE_TOTAL_BYTES_SEC, > > + VIR_TYPED_PARAM_ULLONG, > > + VIR_DOMAIN_BLOCK_IOTUNE_READ_BYTES_SEC, > > + VIR_TYPED_PARAM_ULLONG, > > + VIR_DOMAIN_BLOCK_IOTUNE_WRITE_BYTES_SEC, > > + VIR_TYPED_PARAM_ULLONG, > > + VIR_DOMAIN_BLOCK_IOTUNE_TOTAL_IOPS_SEC, > > + VIR_TYPED_PARAM_ULLONG, > > + VIR_DOMAIN_BLOCK_IOTUNE_READ_IOPS_SEC, > > + VIR_TYPED_PARAM_ULLONG, > > + VIR_DOMAIN_BLOCK_IOTUNE_WRITE_IOPS_SEC, > > + VIR_TYPED_PARAM_ULLONG, > > + VIR_DOMAIN_BLOCK_IOTUNE_TOTAL_BYTES_SEC_MAX, > > + VIR_TYPED_PARAM_ULLONG, > > + VIR_DOMAIN_BLOCK_IOTUNE_READ_BYTES_SEC_MAX, > > + VIR_TYPED_PARAM_ULLONG, > > + VIR_DOMAIN_BLOCK_IOTUNE_WRITE_BYTES_SEC_MAX, > > + VIR_TYPED_PARAM_ULLONG, > > + VIR_DOMAIN_BLOCK_IOTUNE_TOTAL_IOPS_SEC_MAX, > > + VIR_TYPED_PARAM_ULLONG, > > + VIR_DOMAIN_BLOCK_IOTUNE_READ_IOPS_SEC_MAX, > > + VIR_TYPED_PARAM_ULLONG, > > + VIR_DOMAIN_BLOCK_IOTUNE_WRITE_IOPS_SEC_MAX, > > + VIR_TYPED_PARAM_ULLONG, > > + VIR_DOMAIN_BLOCK_IOTUNE_SIZE_IOPS_SEC, > > + VIR_TYPED_PARAM_ULLONG, > > + VIR_DOMAIN_BLOCK_IOTUNE_GROUP_NAME, > > + VIR_TYPED_PARAM_STRING, > > + VIR_DOMAIN_BLOCK_IOTUNE_TOTAL_BYTES_SEC_MAX_LENGTH, > > + VIR_TYPED_PARAM_ULLONG, > > + VIR_DOMAIN_BLOCK_IOTUNE_READ_BYTES_SEC_MAX_LENGTH, > > + VIR_TYPED_PARAM_ULLONG, > > + VIR_DOMAIN_BLOCK_IOTUNE_WRITE_BYTES_SEC_MAX_LENGTH, > > + VIR_TYPED_PARAM_ULLONG, > > + VIR_DOMAIN_BLOCK_IOTUNE_TOTAL_IOPS_SEC_MAX_LENGTH, > > + VIR_TYPED_PARAM_ULLONG, > > + VIR_DOMAIN_BLOCK_IOTUNE_READ_IOPS_SEC_MAX_LENGTH, > > + VIR_TYPED_PARAM_ULLONG, > > + VIR_DOMAIN_BLOCK_IOTUNE_WRITE_IOPS_SEC_MAX_LENGTH, > > + VIR_TYPED_PARAM_ULLONG, > > + NULL) < 0) > > + return -1; > > + > > + if (!(vm = testDomObjFromDomain(dom))) > > + return -1; > > + > > + if (!(def = virDomainObjGetOneDef(vm, flags))) > > + goto cleanup; > > + > > + if (!(conf_disk = virDomainDiskByName(def, path, true))) { > > + virReportError(VIR_ERR_INVALID_ARG, > > + _("missing persistent configuration for disk '%s'"), > > + path); > > + goto cleanup; > > + } > > + > > + info = conf_disk->blkdeviotune; > > + if (VIR_STRDUP(info.group_name, conf_disk->blkdeviotune.group_name) < 0) > > + goto cleanup; > > + > > + if (virTypedParamsAddString(&eventParams, &eventNparams, &eventMaxparams, > > + VIR_DOMAIN_TUNABLE_BLKDEV_DISK, path) < 0) > > + goto cleanup; > > + > > +#define SET_IOTUNE_FIELD(FIELD, BOOL, CONST) \ > > redundant BOOL argument... > > > + if (STREQ(param->field, VIR_DOMAIN_BLOCK_IOTUNE_##CONST)) { \ > > + info.FIELD = param->value.ul; \ > > + if (virTypedParamsAddULLong(&eventParams, &eventNparams, \ > > + &eventMaxparams, \ > > + VIR_DOMAIN_TUNABLE_BLKDEV_##CONST, \ > > We've already discussed this and I believe we decided we're not going to > oncatenate the enum names, so let's do it differently in the test driver. Haha. Alright, so I believe that in 1/2 I thought that there is a macro that we can re-use (TEST_SET_PARAM) and re-using stuff is good. On the other hand, for 2/2 a new macro "had to" be introduced. I copied the one from the QEMU implementation, and indeed the macro string concatenation done here is particularly useful because it's used twice and with different prefixes.