[libvirt] [PATCH 2/2] test_driver: implement virDomainSetBlockIoTune
Erik Skultety
eskultet at redhat.com
Thu Aug 8 10:56:55 UTC 2019
On Wed, Aug 07, 2019 at 12:52:17PM +0200, Ilias Stamatis wrote:
> On Sun, Aug 4, 2019 at 6:32 PM Erik Skultety <eskultet at redhat.com> wrote:
> >
> > On Sat, Jul 27, 2019 at 05:04:38PM +0200, Ilias Stamatis wrote:
> > > Signed-off-by: Ilias Stamatis <stamatis.iliass at gmail.com>
> > > ---
> > >
> > > 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.
Although true, we should remain consistent with our decisions, even though the
macro call can be eye pleasing, we still cannot jump to the symbol directly.
It's even worse here exactly because of the two different prefixes. The reason
is the macro hides this information from you, which can only add to the
confusion when you're reading the code. Yes, it's very unfortunate that we have
to work with 2 different enums here, but I think the consistency is worth the
extra lines.
Erik
More information about the libvir-list
mailing list