[PATCH 0/4] qemuDomainSetThrottleGroup: Fix group name related issues
Peter Krempa (4): qemuDomainSetThrottleGroup: Enforce non-zero 'groupname' string length qemuDomainSetBlockIoTuneField: Move setting of 'group_name' out of the loop qemuDomainSetThrottleGroup: Always honour thottle group name passed as argument qemuDomainSetThrottleGroup: Don't put group name into the 'tunable' event twice src/qemu/qemu_driver.c | 37 +++++++++++++++++++++++++------------ 1 file changed, 25 insertions(+), 12 deletions(-) -- 2.52.0
From: Peter Krempa <pkrempa@redhat.com> Having a name of 0 characters makes no sense. Reject it. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_driver.c | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 3f154969b8..bed60d1ca7 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -20354,6 +20354,12 @@ qemuDomainSetThrottleGroup(virDomainPtr dom, virCheckFlags(VIR_DOMAIN_AFFECT_LIVE | VIR_DOMAIN_AFFECT_CONFIG, -1); + if (strlen(groupname) == 0) { + virReportError(VIR_ERR_INVALID_ARG, "%s", + _("'groupname' parameter string must have non-zero length")); + return -1; + } + if (qemuDomainValidateBlockIoTune(params, nparams) < 0) return -1; -- 2.52.0
From: Peter Krempa <pkrempa@redhat.com> The refactor will simplify further change which will introduce another source for the group name. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_driver.c | 20 ++++++++++++-------- 1 file changed, 12 insertions(+), 8 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index bed60d1ca7..8c7a2e9fe2 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -15182,6 +15182,7 @@ qemuDomainSetBlockIoTuneFields(virDomainBlockIoTuneInfo *info, int *eventNparams, int *eventMaxparams) { + const char *param_group_name = NULL; size_t i; #define SET_IOTUNE_FIELD(FIELD, BOOL, CONST) \ @@ -15227,15 +15228,8 @@ qemuDomainSetBlockIoTuneFields(virDomainBlockIoTuneInfo *info, WRITE_IOPS_SEC_MAX); SET_IOTUNE_FIELD(size_iops_sec, SIZE_IOPS, SIZE_IOPS_SEC); - /* NB: Cannot use macro since this is a value.s not a value.ul */ if (STREQ(param->field, VIR_DOMAIN_BLOCK_IOTUNE_GROUP_NAME)) { - info->group_name = g_strdup(param->value.s); - *set_fields |= QEMU_BLOCK_IOTUNE_SET_GROUP_NAME; - if (virTypedParamsAddString(eventParams, eventNparams, - eventMaxparams, - VIR_DOMAIN_TUNABLE_BLKDEV_GROUP_NAME, - param->value.s) < 0) - return -1; + param_group_name = param->value.s; continue; } @@ -15253,6 +15247,16 @@ qemuDomainSetBlockIoTuneFields(virDomainBlockIoTuneInfo *info, WRITE_IOPS_SEC_MAX_LENGTH); } + if (param_group_name) { + info->group_name = g_strdup(param_group_name); + *set_fields |= QEMU_BLOCK_IOTUNE_SET_GROUP_NAME; + if (virTypedParamsAddString(eventParams, eventNparams, + eventMaxparams, + VIR_DOMAIN_TUNABLE_BLKDEV_GROUP_NAME, + param_group_name) < 0) + return -1; + } + #undef SET_IOTUNE_FIELD return 0; -- 2.52.0
From: Peter Krempa <pkrempa@redhat.com> Due to the code share with 'qemuDomainSetBlockIoTune' the throttle group setting code accepts the throttle group name also via typed parameters. In 'qemuDomainSetThrottleGroup', this means that there are 2 ways to pass it the throttle group name and both are handled slightly differently. Specifically the name of the group used in the list of groups is the name taken from the typed parameters rather than the one passed via API. We also don't validate that they match. Now if the name in the typed parameters is missing we'd add empty string to the group list which would later crash when looking up the group name. To avoid this problem always use the name passed via argument. This is achieved by passing it into 'qemuDomainSetBlockIoTuneFields' so that it overrides whatever is in the typed parameters. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_driver.c | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 8c7a2e9fe2..9addad3b9e 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -15177,6 +15177,7 @@ static int qemuDomainSetBlockIoTuneFields(virDomainBlockIoTuneInfo *info, virTypedParameterPtr params, int nparams, + const char *group_name, qemuBlockIoTuneSetFlags *set_fields, virTypedParameterPtr *eventParams, int *eventNparams, @@ -15247,6 +15248,10 @@ qemuDomainSetBlockIoTuneFields(virDomainBlockIoTuneInfo *info, WRITE_IOPS_SEC_MAX_LENGTH); } + /* The name of the throttle group passed via API always takes precedence */ + if (group_name) + param_group_name = group_name; + if (param_group_name) { info->group_name = g_strdup(param_group_name); *set_fields |= QEMU_BLOCK_IOTUNE_SET_GROUP_NAME; @@ -15394,6 +15399,7 @@ qemuDomainSetBlockIoTune(virDomainPtr dom, if (qemuDomainSetBlockIoTuneFields(&info, params, nparams, + NULL, &set_fields, &eventParams, &eventNparams, @@ -20388,6 +20394,7 @@ qemuDomainSetThrottleGroup(virDomainPtr dom, if (qemuDomainSetBlockIoTuneFields(&info, params, nparams, + groupname, &set_fields, &eventParams, &eventNparams, -- 2.52.0
From: Peter Krempa <pkrempa@redhat.com> 'qemuDomainSetBlockIoTuneFields' already populates the contents of the VIR_DOMAIN_EVENT_ID_TUNABLE params with the group name so there's no need to do it explicitly. We'd report the group name twice: event 'tunable' for domain 'cd': blkdeviotune.group_name: asdf blkdeviotune.total_bytes_sec: 1234 blkdeviotune.group_name: asdf Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_driver.c | 4 ---- 1 file changed, 4 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 9addad3b9e..cdd333c882 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -20387,10 +20387,6 @@ qemuDomainSetThrottleGroup(virDomainPtr dom, if (virDomainObjGetDefs(vm, flags, &def, &persistentDef) < 0) goto endjob; - if (virTypedParamsAddString(&eventParams, &eventNparams, &eventMaxparams, - VIR_DOMAIN_TUNABLE_BLKDEV_GROUP_NAME, groupname) < 0) - goto endjob; - if (qemuDomainSetBlockIoTuneFields(&info, params, nparams, -- 2.52.0
On 1/19/26 10:12, Peter Krempa via Devel wrote:
Peter Krempa (4): qemuDomainSetThrottleGroup: Enforce non-zero 'groupname' string length qemuDomainSetBlockIoTuneField: Move setting of 'group_name' out of the loop qemuDomainSetThrottleGroup: Always honour thottle group name passed as argument qemuDomainSetThrottleGroup: Don't put group name into the 'tunable' event twice
src/qemu/qemu_driver.c | 37 +++++++++++++++++++++++++------------ 1 file changed, 25 insertions(+), 12 deletions(-)
Reviewed-by: Michal Privoznik <mprivozn@redhat.com> Michal
participants (2)
-
Michal Prívozník -
Peter Krempa