On Sun, Aug 4, 2019 at 5:32 PM Erik Skultety <eskultet@xxxxxxxxxx> wrote:
>
> On Sun, Aug 04, 2019 at 03:46:03PM +0200, Ilias Stamatis wrote:
> > On Sun, Aug 4, 2019 at 11:12 AM Erik Skultety <eskultet@xxxxxxxxxx> wrote:
> > >
> > > On Sat, Jul 27, 2019 at 05:04:37PM +0200, Ilias Stamatis wrote:
> > > > Signed-off-by: Ilias Stamatis <stamatis.iliass@xxxxxxxxx>
> > > > ---
> > > > src/test/test_driver.c | 90 ++++++++++++++++++++++++++++++++++++++++++
> > > > 1 file changed, 90 insertions(+)
> > > >
> > > > diff --git a/src/test/test_driver.c b/src/test/test_driver.c
> > > > index ab0f8b06d6..9f4e255e35 100755
> > > > --- a/src/test/test_driver.c
> > > > +++ b/src/test/test_driver.c
> > > > @@ -3500,6 +3500,95 @@ testDomainGetInterfaceParameters(virDomainPtr dom,
> > > > virDomainObjEndAPI(&vm);
> > > > return ret;
> > > > }
> > > > +
> > > > +
> > > > +static int
> > > > +testDomainGetBlockIoTune(virDomainPtr dom,
> > > > + const char *path,
> > > > + virTypedParameterPtr params,
> > > > + int *nparams,
> > > > + unsigned int flags)
> > > > +{
> > > > + virDomainObjPtr vm = NULL;
> > > > + virDomainDefPtr def = NULL;
> > > > + virDomainDiskDefPtr disk;
> > > > + virDomainBlockIoTuneInfo reply = {0};
> > > > + int ret = -1;
> > > > +
> > > > + virCheckFlags(VIR_DOMAIN_AFFECT_LIVE |
> > > > + VIR_DOMAIN_AFFECT_CONFIG |
> > > > + VIR_TYPED_PARAM_STRING_OKAY, -1);
> > > > +
> > > > + flags &= ~VIR_TYPED_PARAM_STRING_OKAY;
> > > > +
> > > > + if (*nparams == 0) {
> > > > + *nparams = 20;
> > > > + return 0;
> > > > + }
> > > > +
> > > > + if (!(vm = testDomObjFromDomain(dom)))
> > > > + return -1;
> > > > +
> > > > + if (!(def = virDomainObjGetOneDef(vm, flags)))
> > > > + goto cleanup;
> > > > +
> > > > + if (!(disk = virDomainDiskByName(def, path, true))) {
> > > > + virReportError(VIR_ERR_INVALID_ARG,
> > > > + _("disk '%s' was not found in the domain config"),
> > > > + path);
> > > > + goto cleanup;
> > > > + }
> > > > +
> > > > + reply = disk->blkdeviotune;
> > > > + if (VIR_STRDUP(reply.group_name, disk->blkdeviotune.group_name) < 0)
> > > > + goto cleanup;
> > > > +
> > > > +#define ULL VIR_TYPED_PARAM_ULLONG
> > >
> > > I don't see a point in doing ^this just to save a few characters, we're way
> > > over the 80 columns already anyway, so wrap the long lines and span the macro
> > > call over multiple lines.
> >
> > That's true that we're already over the 80 columns, but not by that
> > much so I thought that by doing it like that it allows us to "save" 13
> > new lines.
>
> Well, there isn't strictly a rule of thumb here. It's always a personal
> preference, but I look at it whether we gained anything by this "translation",
> we saved 13 lines, great. However, if you split the lines properly, are we
> really going to lose anything? No, not even in terms of readability.
Okay, I see your point and it makes sense.
>
> >
> > > Funny that I remember us having a discussion about macros doing string
> > > concatenation (which I don't really mind that much) but prevents you from jumping
> > > directly to the symbol definition - that's exactly what the QEMU alternative
> > > does, I guess just to save some common prefixes :D.
> >
> > Actually in this case it doesn't really prevent you. Just you need an
> > extra jump. One to jump to the definition of the ULL and from there to
> > jump to the next definition (even though all the ULL usage fits in a
> > single screen).
> >
> > Instead if there are many (instead of a single one) strings like like
> > "TOTAL_BYTES_SEC", "READ_BYTES_SEC" etc that are concatenated/extended
> > magically by some macro it makes it trickier to find the original
> > definitions. But that's just my opinion.
>
> Ironically, patch 2 does exactly the concatenation magic ;).
Haha, well played. ;D I didn't notice / remember that. I will reply to
this on the other e-mail.
So I will resend this after removing the ULL macro.
Thanks,
Ilias
>
> >
> > For me it's alright to use the QEMU macro as well, but we have already
> > the TEST_SET_PARAM which is used everywhere else so it makes more
> > sense to use it here too for consistency reasons I believe.
>
> I didn't object to using TEST_SET_PARAM, I merely suggested a minor adjustment
> to it with other potentially necessary minor adjustments along the way.
>
> Erik
>
> >
> > >
> > > Looking at the functions that use the typed parameters, an improvement we could
> > > definitely do (in a standalone patch) is to ditch the index from the macro
> > > definition since it's quite fragile, by doing:
> > >
> > > int maxparams = X;
> > >
> > > if (*nparams == 0) {
> > > *nparams = maxparams;
> > > return 0;
> > > }
> > >
> > > *nparams = 0;
> > >
> > > TEST_SET_PARAM(¶m[*nparams++],...)
> > >
> > > > + TEST_SET_PARAM(0, VIR_DOMAIN_BLOCK_IOTUNE_TOTAL_BYTES_SEC, ULL, reply.total_bytes_sec);
> > > > + TEST_SET_PARAM(1, VIR_DOMAIN_BLOCK_IOTUNE_READ_BYTES_SEC, ULL, reply.read_bytes_sec);
> > > > + TEST_SET_PARAM(2, VIR_DOMAIN_BLOCK_IOTUNE_WRITE_BYTES_SEC, ULL, reply.write_bytes_sec);
> > > > +
> > > > + TEST_SET_PARAM(3, VIR_DOMAIN_BLOCK_IOTUNE_TOTAL_IOPS_SEC, ULL, reply.total_iops_sec);
> > > > + TEST_SET_PARAM(4, VIR_DOMAIN_BLOCK_IOTUNE_READ_IOPS_SEC, ULL, reply.read_iops_sec);
> > > > + TEST_SET_PARAM(5, VIR_DOMAIN_BLOCK_IOTUNE_WRITE_IOPS_SEC, ULL, reply.write_iops_sec);
> > > > +
> > > > + TEST_SET_PARAM(6, VIR_DOMAIN_BLOCK_IOTUNE_TOTAL_BYTES_SEC_MAX, ULL, reply.total_bytes_sec_max);
> > > > + TEST_SET_PARAM(7, VIR_DOMAIN_BLOCK_IOTUNE_READ_BYTES_SEC_MAX, ULL, reply.read_bytes_sec_max);
> > > > + TEST_SET_PARAM(8, VIR_DOMAIN_BLOCK_IOTUNE_WRITE_BYTES_SEC_MAX, ULL, reply.write_bytes_sec_max);
> > > > +
> > > > + TEST_SET_PARAM(9, VIR_DOMAIN_BLOCK_IOTUNE_TOTAL_IOPS_SEC_MAX, ULL, reply.total_iops_sec_max);
> > > > + TEST_SET_PARAM(10, VIR_DOMAIN_BLOCK_IOTUNE_READ_IOPS_SEC_MAX, ULL, reply.read_iops_sec_max);
> > > > + TEST_SET_PARAM(11, VIR_DOMAIN_BLOCK_IOTUNE_WRITE_IOPS_SEC_MAX, ULL, reply.write_iops_sec_max);
> > > > +
> > > > + TEST_SET_PARAM(12, VIR_DOMAIN_BLOCK_IOTUNE_SIZE_IOPS_SEC, ULL, reply.size_iops_sec);
> > > > +
> > > > + TEST_SET_PARAM(13, VIR_DOMAIN_BLOCK_IOTUNE_GROUP_NAME, VIR_TYPED_PARAM_STRING, reply.group_name);
> > > > + reply.group_name = NULL;
> > > > +
> > > > + TEST_SET_PARAM(14, VIR_DOMAIN_BLOCK_IOTUNE_TOTAL_BYTES_SEC_MAX_LENGTH, ULL,
> > > > + reply.total_bytes_sec_max_length);
> > > > + TEST_SET_PARAM(15, VIR_DOMAIN_BLOCK_IOTUNE_READ_BYTES_SEC_MAX_LENGTH, ULL,
> > > > + reply.read_bytes_sec_max_length);
> > > > + TEST_SET_PARAM(16, VIR_DOMAIN_BLOCK_IOTUNE_WRITE_BYTES_SEC_MAX_LENGTH, ULL,
> > > > + reply.write_bytes_sec_max_length);
> > > > +
> > > > + TEST_SET_PARAM(17, VIR_DOMAIN_BLOCK_IOTUNE_TOTAL_IOPS_SEC_MAX_LENGTH, ULL,
> > > > + reply.total_iops_sec_max_length);
> > > > + TEST_SET_PARAM(18, VIR_DOMAIN_BLOCK_IOTUNE_READ_IOPS_SEC_MAX_LENGTH, ULL,
> > > > + reply.read_iops_sec_max_length);
> > > > + TEST_SET_PARAM(19, VIR_DOMAIN_BLOCK_IOTUNE_WRITE_IOPS_SEC_MAX_LENGTH, ULL,
> > > > + reply.write_iops_sec_max_length);
> > > > +#undef ULL
> > > > +
> > > > + if (*nparams > 20)
> > > > + *nparams = 20;
> > > > +
> > > > + ret = 0;
> > > > + cleanup:
> > > > + VIR_FREE(reply.group_name);
> > > > + virDomainObjEndAPI(&vm);
> > > > + return ret;
> > > > +}
> > > > #undef TEST_SET_PARAM
> > > >
> > > >
> > > > @@ -8512,6 +8601,7 @@ static virHypervisorDriver testHypervisorDriver = {
> > > > .domainGetNumaParameters = testDomainGetNumaParameters, /* 5.6.0 */
> > > > .domainSetInterfaceParameters = testDomainSetInterfaceParameters, /* 5.6.0 */
> > > > .domainGetInterfaceParameters = testDomainGetInterfaceParameters, /* 5.6.0 */
> > > > + .domainGetBlockIoTune = testDomainGetBlockIoTune, /* 5.6.0 */
> > >
> > > 5.7.0 (so that we/I don't forget about to change it ;))
> > >
> > > With breaking the long lines:
> > > Reviewed-by: Erik Skultety <eskultet@xxxxxxxxxx>
> >
> > --
> > libvir-list mailing list
> > libvir-list@xxxxxxxxxx
> > https://www.redhat.com/mailman/listinfo/libvir-list
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
