Signed-off-by: Ilias Stamatis <stamatis.iliass@gmail.com>
---
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
+ 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 */
.connectListDefinedDomains = testConnectListDefinedDomains, /* 0.1.11 */
.connectNumOfDefinedDomains = testConnectNumOfDefinedDomains, /* 0.1.11 */
.domainCreate = testDomainCreate, /* 0.1.11 */
--
2.22.0
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On Sat, Jul 27, 2019 at 05:04:37PM +0200, Ilias Stamatis wrote:
> Signed-off-by: Ilias Stamatis <stamatis.iliass@gmail.com>
> ---
> 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.
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.
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@redhat.com>
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On Sun, Aug 4, 2019 at 11:12 AM Erik Skultety <eskultet@redhat.com> wrote:
>
> On Sat, Jul 27, 2019 at 05:04:37PM +0200, Ilias Stamatis wrote:
> > Signed-off-by: Ilias Stamatis <stamatis.iliass@gmail.com>
> > ---
> > 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.
> 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.
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.
>
> 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@redhat.com>
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
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@redhat.com> wrote:
> >
> > On Sat, Jul 27, 2019 at 05:04:37PM +0200, Ilias Stamatis wrote:
> > > Signed-off-by: Ilias Stamatis <stamatis.iliass@gmail.com>
> > > ---
> > > 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.
>
> > 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 ;).
>
> 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@redhat.com>
>
> --
> libvir-list mailing list
> libvir-list@redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On Sun, Aug 4, 2019 at 5:32 PM Erik Skultety <eskultet@redhat.com> 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@redhat.com> wrote:
> > >
> > > On Sat, Jul 27, 2019 at 05:04:37PM +0200, Ilias Stamatis wrote:
> > > > Signed-off-by: Ilias Stamatis <stamatis.iliass@gmail.com>
> > > > ---
> > > > 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@redhat.com>
> >
> > --
> > libvir-list mailing list
> > libvir-list@redhat.com
> > https://www.redhat.com/mailman/listinfo/libvir-list
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
© 2016 - 2024 Red Hat, Inc.