[libvirt] [PATCH 0/2] test_driver: implement get/set BlockIoTune APIs
Ilias Stamatis (2): test_driver: implement virDomainGetBlockIoTune test_driver: implement virDomainSetBlockIoTune src/test/test_driver.c | 286 +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 286 insertions(+) -- 2.22.0
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
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>
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>
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
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
Signed-off-by: Ilias Stamatis <stamatis.iliass@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) \ + if (STREQ(param->field, VIR_DOMAIN_BLOCK_IOTUNE_##CONST)) { \ + info.FIELD = param->value.ul; \ + if (virTypedParamsAddULLong(&eventParams, &eventNparams, \ + &eventMaxparams, \ + VIR_DOMAIN_TUNABLE_BLKDEV_##CONST, \ + param->value.ul) < 0) \ + goto cleanup; \ + continue; \ + } + + for (i = 0; i < nparams; i++) { + virTypedParameterPtr param = ¶ms[i]; + + if (param->value.ul > 1000000000000000LL) { + virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, + _("block I/O throttle limit value must" + " be no more than %llu"), 1000000000000000LL); + goto cleanup; + } + + 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); + + SET_IOTUNE_FIELD(total_bytes_sec_max, BYTES_MAX, TOTAL_BYTES_SEC_MAX); + SET_IOTUNE_FIELD(read_bytes_sec_max, BYTES_MAX, READ_BYTES_SEC_MAX); + SET_IOTUNE_FIELD(write_bytes_sec_max, BYTES_MAX, WRITE_BYTES_SEC_MAX); + SET_IOTUNE_FIELD(total_iops_sec_max, IOPS_MAX, TOTAL_IOPS_SEC_MAX); + SET_IOTUNE_FIELD(read_iops_sec_max, IOPS_MAX, READ_IOPS_SEC_MAX); + SET_IOTUNE_FIELD(write_iops_sec_max, IOPS_MAX, WRITE_IOPS_SEC_MAX); + SET_IOTUNE_FIELD(size_iops_sec, SIZE_IOPS, SIZE_IOPS_SEC); + + if (STREQ(param->field, VIR_DOMAIN_BLOCK_IOTUNE_GROUP_NAME)) { + VIR_FREE(info.group_name); + if (VIR_STRDUP(info.group_name, param->value.s) < 0) + goto cleanup; + if (virTypedParamsAddString(&eventParams, + &eventNparams, + &eventMaxparams, + VIR_DOMAIN_TUNABLE_BLKDEV_GROUP_NAME, + param->value.s) < 0) + goto cleanup; + continue; + } + + SET_IOTUNE_FIELD(total_bytes_sec_max_length, BYTES_MAX_LENGTH, + TOTAL_BYTES_SEC_MAX_LENGTH); + SET_IOTUNE_FIELD(read_bytes_sec_max_length, BYTES_MAX_LENGTH, + READ_BYTES_SEC_MAX_LENGTH); + SET_IOTUNE_FIELD(write_bytes_sec_max_length, BYTES_MAX_LENGTH, + WRITE_BYTES_SEC_MAX_LENGTH); + SET_IOTUNE_FIELD(total_iops_sec_max_length, IOPS_MAX_LENGTH, + TOTAL_IOPS_SEC_MAX_LENGTH); + SET_IOTUNE_FIELD(read_iops_sec_max_length, IOPS_MAX_LENGTH, + READ_IOPS_SEC_MAX_LENGTH); + SET_IOTUNE_FIELD(write_iops_sec_max_length, IOPS_MAX_LENGTH, + WRITE_IOPS_SEC_MAX_LENGTH); + } +#undef SET_IOTUNE_FIELD + + if ((info.total_bytes_sec && info.read_bytes_sec) || + (info.total_bytes_sec && info.write_bytes_sec)) { + virReportError(VIR_ERR_INVALID_ARG, "%s", + _("total and read/write of bytes_sec " + "cannot be set at the same time")); + goto cleanup; + } + + if ((info.total_iops_sec && info.read_iops_sec) || + (info.total_iops_sec && info.write_iops_sec)) { + virReportError(VIR_ERR_INVALID_ARG, "%s", + _("total and read/write of iops_sec " + "cannot be set at the same time")); + goto cleanup; + } + + if ((info.total_bytes_sec_max && info.read_bytes_sec_max) || + (info.total_bytes_sec_max && info.write_bytes_sec_max)) { + virReportError(VIR_ERR_INVALID_ARG, "%s", + _("total and read/write of bytes_sec_max " + "cannot be set at the same time")); + goto cleanup; + } + + if ((info.total_iops_sec_max && info.read_iops_sec_max) || + (info.total_iops_sec_max && info.write_iops_sec_max)) { + virReportError(VIR_ERR_INVALID_ARG, "%s", + _("total and read/write of iops_sec_max " + "cannot be set at the same time")); + goto cleanup; + } + + if (virDomainDiskSetBlockIOTune(conf_disk, &info) < 0) + goto cleanup; + info.group_name = NULL; + + ret = 0; + cleanup: + VIR_FREE(info.group_name); + virDomainObjEndAPI(&vm); + if (eventNparams) + virTypedParamsFree(eventParams, eventNparams); + return ret; +} + + static int testDomainGetBlockIoTune(virDomainPtr dom, const char *path, @@ -8600,6 +8795,7 @@ static virHypervisorDriver testHypervisorDriver = { .domainSetNumaParameters = testDomainSetNumaParameters, /* 5.6.0 */ .domainGetNumaParameters = testDomainGetNumaParameters, /* 5.6.0 */ .domainSetInterfaceParameters = testDomainSetInterfaceParameters, /* 5.6.0 */ + .domainSetBlockIoTune = testDomainSetBlockIoTune, /* 5.6.0 */ .domainGetInterfaceParameters = testDomainGetInterfaceParameters, /* 5.6.0 */ .domainGetBlockIoTune = testDomainGetBlockIoTune, /* 5.6.0 */ .connectListDefinedDomains = testConnectListDefinedDomains, /* 0.1.11 */ -- 2.22.0
On Sat, Jul 27, 2019 at 05:04:38PM +0200, Ilias Stamatis wrote:
Signed-off-by: Ilias Stamatis <stamatis.iliass@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.
+ param->value.ul) < 0) \ + goto cleanup; \ + continue; \ + } + + for (i = 0; i < nparams; i++) { + virTypedParameterPtr param = ¶ms[i]; + + if (param->value.ul > 1000000000000000LL) {
I think we may want to define ^this as a macro constant for test driver as well.
+ virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, + _("block I/O throttle limit value must" + " be no more than %llu"), 1000000000000000LL); + goto cleanup; + } + + 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); + + SET_IOTUNE_FIELD(total_bytes_sec_max, BYTES_MAX, TOTAL_BYTES_SEC_MAX); + SET_IOTUNE_FIELD(read_bytes_sec_max, BYTES_MAX, READ_BYTES_SEC_MAX); + SET_IOTUNE_FIELD(write_bytes_sec_max, BYTES_MAX, WRITE_BYTES_SEC_MAX); + SET_IOTUNE_FIELD(total_iops_sec_max, IOPS_MAX, TOTAL_IOPS_SEC_MAX); + SET_IOTUNE_FIELD(read_iops_sec_max, IOPS_MAX, READ_IOPS_SEC_MAX); + SET_IOTUNE_FIELD(write_iops_sec_max, IOPS_MAX, WRITE_IOPS_SEC_MAX); + SET_IOTUNE_FIELD(size_iops_sec, SIZE_IOPS, SIZE_IOPS_SEC); + + if (STREQ(param->field, VIR_DOMAIN_BLOCK_IOTUNE_GROUP_NAME)) { + VIR_FREE(info.group_name); + if (VIR_STRDUP(info.group_name, param->value.s) < 0) + goto cleanup; + if (virTypedParamsAddString(&eventParams, + &eventNparams, + &eventMaxparams, + VIR_DOMAIN_TUNABLE_BLKDEV_GROUP_NAME, + param->value.s) < 0) + goto cleanup; + continue; + } + + SET_IOTUNE_FIELD(total_bytes_sec_max_length, BYTES_MAX_LENGTH, + TOTAL_BYTES_SEC_MAX_LENGTH); + SET_IOTUNE_FIELD(read_bytes_sec_max_length, BYTES_MAX_LENGTH, + READ_BYTES_SEC_MAX_LENGTH); + SET_IOTUNE_FIELD(write_bytes_sec_max_length, BYTES_MAX_LENGTH, + WRITE_BYTES_SEC_MAX_LENGTH); + SET_IOTUNE_FIELD(total_iops_sec_max_length, IOPS_MAX_LENGTH, + TOTAL_IOPS_SEC_MAX_LENGTH); + SET_IOTUNE_FIELD(read_iops_sec_max_length, IOPS_MAX_LENGTH, + READ_IOPS_SEC_MAX_LENGTH); + SET_IOTUNE_FIELD(write_iops_sec_max_length, IOPS_MAX_LENGTH, + WRITE_IOPS_SEC_MAX_LENGTH); + } +#undef SET_IOTUNE_FIELD + + if ((info.total_bytes_sec && info.read_bytes_sec) || + (info.total_bytes_sec && info.write_bytes_sec)) { + virReportError(VIR_ERR_INVALID_ARG, "%s", + _("total and read/write of bytes_sec " + "cannot be set at the same time")); + goto cleanup; + } + + if ((info.total_iops_sec && info.read_iops_sec) || + (info.total_iops_sec && info.write_iops_sec)) { + virReportError(VIR_ERR_INVALID_ARG, "%s", + _("total and read/write of iops_sec " + "cannot be set at the same time")); + goto cleanup; + } + + if ((info.total_bytes_sec_max && info.read_bytes_sec_max) || + (info.total_bytes_sec_max && info.write_bytes_sec_max)) { + virReportError(VIR_ERR_INVALID_ARG, "%s", + _("total and read/write of bytes_sec_max " + "cannot be set at the same time")); + goto cleanup; + } + + if ((info.total_iops_sec_max && info.read_iops_sec_max) || + (info.total_iops_sec_max && info.write_iops_sec_max)) { + virReportError(VIR_ERR_INVALID_ARG, "%s", + _("total and read/write of iops_sec_max " + "cannot be set at the same time")); + goto cleanup; + } + + if (virDomainDiskSetBlockIOTune(conf_disk, &info) < 0) + goto cleanup; + info.group_name = NULL;
You should also be checking violations against the max settings, like you mentioned in the commit note, some of the checks are hypervisor dependent, but some are purely logical, e.g. setting one of the limits higher than the corresponding MAX feels odd. I'm not insisting on the need to specify the limit along the max (that really feels like per-hypervisor detail).
+ + ret = 0; + cleanup: + VIR_FREE(info.group_name); + virDomainObjEndAPI(&vm); + if (eventNparams) + virTypedParamsFree(eventParams, eventNparams); + return ret; +} + + static int testDomainGetBlockIoTune(virDomainPtr dom, const char *path, @@ -8600,6 +8795,7 @@ static virHypervisorDriver testHypervisorDriver = { .domainSetNumaParameters = testDomainSetNumaParameters, /* 5.6.0 */ .domainGetNumaParameters = testDomainGetNumaParameters, /* 5.6.0 */ .domainSetInterfaceParameters = testDomainSetInterfaceParameters, /* 5.6.0 */ + .domainSetBlockIoTune = testDomainSetBlockIoTune, /* 5.6.0 */
Since you're going to re-spin, don't forget to update ^this. Erik
.domainGetInterfaceParameters = testDomainGetInterfaceParameters, /* 5.6.0 */ .domainGetBlockIoTune = testDomainGetBlockIoTune, /* 5.6.0 */ .connectListDefinedDomains = testConnectListDefinedDomains, /* 0.1.11 */ -- 2.22.0
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On Sun, Aug 4, 2019 at 6:32 PM Erik Skultety <eskultet@redhat.com> wrote:
On Sat, Jul 27, 2019 at 05:04:38PM +0200, Ilias Stamatis wrote:
Signed-off-by: Ilias Stamatis <stamatis.iliass@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. So if you also think that it's fine we can probably leave it as is here? (by removing the redundant argument of course)
+ param->value.ul) < 0) \ + goto cleanup; \ + continue; \ + } + + for (i = 0; i < nparams; i++) { + virTypedParameterPtr param = ¶ms[i]; + + if (param->value.ul > 1000000000000000LL) {
I think we may want to define ^this as a macro constant for test driver as well.
Sure.
+ virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, + _("block I/O throttle limit value must" + " be no more than %llu"), 1000000000000000LL); + goto cleanup; + } + + 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); + + SET_IOTUNE_FIELD(total_bytes_sec_max, BYTES_MAX, TOTAL_BYTES_SEC_MAX); + SET_IOTUNE_FIELD(read_bytes_sec_max, BYTES_MAX, READ_BYTES_SEC_MAX); + SET_IOTUNE_FIELD(write_bytes_sec_max, BYTES_MAX, WRITE_BYTES_SEC_MAX); + SET_IOTUNE_FIELD(total_iops_sec_max, IOPS_MAX, TOTAL_IOPS_SEC_MAX); + SET_IOTUNE_FIELD(read_iops_sec_max, IOPS_MAX, READ_IOPS_SEC_MAX); + SET_IOTUNE_FIELD(write_iops_sec_max, IOPS_MAX, WRITE_IOPS_SEC_MAX); + SET_IOTUNE_FIELD(size_iops_sec, SIZE_IOPS, SIZE_IOPS_SEC); + + if (STREQ(param->field, VIR_DOMAIN_BLOCK_IOTUNE_GROUP_NAME)) { + VIR_FREE(info.group_name); + if (VIR_STRDUP(info.group_name, param->value.s) < 0) + goto cleanup; + if (virTypedParamsAddString(&eventParams, + &eventNparams, + &eventMaxparams, + VIR_DOMAIN_TUNABLE_BLKDEV_GROUP_NAME, + param->value.s) < 0) + goto cleanup; + continue; + } + + SET_IOTUNE_FIELD(total_bytes_sec_max_length, BYTES_MAX_LENGTH, + TOTAL_BYTES_SEC_MAX_LENGTH); + SET_IOTUNE_FIELD(read_bytes_sec_max_length, BYTES_MAX_LENGTH, + READ_BYTES_SEC_MAX_LENGTH); + SET_IOTUNE_FIELD(write_bytes_sec_max_length, BYTES_MAX_LENGTH, + WRITE_BYTES_SEC_MAX_LENGTH); + SET_IOTUNE_FIELD(total_iops_sec_max_length, IOPS_MAX_LENGTH, + TOTAL_IOPS_SEC_MAX_LENGTH); + SET_IOTUNE_FIELD(read_iops_sec_max_length, IOPS_MAX_LENGTH, + READ_IOPS_SEC_MAX_LENGTH); + SET_IOTUNE_FIELD(write_iops_sec_max_length, IOPS_MAX_LENGTH, + WRITE_IOPS_SEC_MAX_LENGTH); + } +#undef SET_IOTUNE_FIELD + + if ((info.total_bytes_sec && info.read_bytes_sec) || + (info.total_bytes_sec && info.write_bytes_sec)) { + virReportError(VIR_ERR_INVALID_ARG, "%s", + _("total and read/write of bytes_sec " + "cannot be set at the same time")); + goto cleanup; + } + + if ((info.total_iops_sec && info.read_iops_sec) || + (info.total_iops_sec && info.write_iops_sec)) { + virReportError(VIR_ERR_INVALID_ARG, "%s", + _("total and read/write of iops_sec " + "cannot be set at the same time")); + goto cleanup; + } + + if ((info.total_bytes_sec_max && info.read_bytes_sec_max) || + (info.total_bytes_sec_max && info.write_bytes_sec_max)) { + virReportError(VIR_ERR_INVALID_ARG, "%s", + _("total and read/write of bytes_sec_max " + "cannot be set at the same time")); + goto cleanup; + } + + if ((info.total_iops_sec_max && info.read_iops_sec_max) || + (info.total_iops_sec_max && info.write_iops_sec_max)) { + virReportError(VIR_ERR_INVALID_ARG, "%s", + _("total and read/write of iops_sec_max " + "cannot be set at the same time")); + goto cleanup; + } + + if (virDomainDiskSetBlockIOTune(conf_disk, &info) < 0) + goto cleanup; + info.group_name = NULL;
You should also be checking violations against the max settings, like you mentioned in the commit note, some of the checks are hypervisor dependent, but some are purely logical, e.g. setting one of the limits higher than the corresponding MAX feels odd. I'm not insisting on the need to specify the limit along the max (that really feels like per-hypervisor detail).
Aah, right. I'll fix that. Thanks! Ilias
+ + ret = 0; + cleanup: + VIR_FREE(info.group_name); + virDomainObjEndAPI(&vm); + if (eventNparams) + virTypedParamsFree(eventParams, eventNparams); + return ret; +} + + static int testDomainGetBlockIoTune(virDomainPtr dom, const char *path, @@ -8600,6 +8795,7 @@ static virHypervisorDriver testHypervisorDriver = { .domainSetNumaParameters = testDomainSetNumaParameters, /* 5.6.0 */ .domainGetNumaParameters = testDomainGetNumaParameters, /* 5.6.0 */ .domainSetInterfaceParameters = testDomainSetInterfaceParameters, /* 5.6.0 */ + .domainSetBlockIoTune = testDomainSetBlockIoTune, /* 5.6.0 */
Since you're going to re-spin, don't forget to update ^this.
Erik
.domainGetInterfaceParameters = testDomainGetInterfaceParameters, /* 5.6.0 */ .domainGetBlockIoTune = testDomainGetBlockIoTune, /* 5.6.0 */ .connectListDefinedDomains = testConnectListDefinedDomains, /* 0.1.11 */ -- 2.22.0
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
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@redhat.com> wrote:
On Sat, Jul 27, 2019 at 05:04:38PM +0200, Ilias Stamatis wrote:
Signed-off-by: Ilias Stamatis <stamatis.iliass@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
participants (2)
-
Erik Skultety -
Ilias Stamatis