[PATCH v3 00/25] Setup iothread polling attributes in the XML

Previous version: https://listman.redhat.com/archives/libvir-list/2023-March/239164.html Changes: - patches 1 - 23 are new and do the following - refactors to typed param handling 1 - 15, 18 - addition of new typed param APIs needed for changing the type we use to store the poll parameters currently, 16, 17, 19 - conversion of the existing parameter handling for iothread poll attributes to unsigned long long 20 - 23 - patches 24 and 25 are patches 1 and 2 of the previous posting: - use unsigned long long to store the values and adjust the code Peter Krempa (25): virTypedParameterAssignValue: Drop 'copystr' parameter util: virtypedparam: Use proper enum type for all switch() statements virTypedParamsDeserialize: Remove unnecessary line breaks virTypedParameterAssignValueVArgs: Ensure proper typed param type in caller util: virtypedparam: Simplify error handling in virTypedParamListAdd* virtypedparam.h: Consistently use contemporary header style util: virtypedparam: Introduce virTypedParamListNew() util: typedparam: Introduce 'virTypedParamListConcat' qemuDomainGetStatsBlock: Don't directly access virTypedParamList util: virtypedparam: Introduce 'virTypedParamListFetch' Use 'virTypedParamListFetch' for extracting identity parameters list util: virtypedparam: Privatize definition of struct _virTypedParamList util: virtypedparam: Refactor return value of virTypedParamListStealParams util: virtypedparam: Store errors inside virTypedParamList util: virtypedparam: Remove return values from virTypedParamListAdd* APIs util: typedparam: Introduce virTypedParamListAddUnsigned util: virtypedparam: Introduce virTypedParamsGetUnsigned virTypedParamsValidate: Refactor variable declaration and cleanup virTypedParamsValidate: Allow typed params to be both _UINT and _ULLONG virsh: cmdIOThreadSet: Refactor to use virTypedParamList qemu: Remove iothread 'poll-' value validation qemu: Store all iothread's 'poll*' attributes as unsigned long long virsh: cmdIOThreadSet: Use bigger types for --poll-grow and --poll-shrink conf: Store the iothread 'poll' settings in the XML qemu: Use configured iothread poll parameters on startup docs/formatdomain.rst | 11 +- include/libvirt/libvirt-domain.h | 4 +- src/admin/admin_server.c | 118 ++--- src/conf/domain_conf.c | 41 +- src/conf/domain_conf.h | 7 + src/conf/schemas/domaincommon.rng | 19 + src/driver.c | 7 +- src/libvirt-domain.c | 14 +- src/libvirt_private.syms | 7 +- src/qemu/qemu_command.c | 18 + src/qemu/qemu_domainjob.c | 49 +-- src/qemu/qemu_driver.c | 409 ++++++------------ src/qemu/qemu_monitor.h | 4 +- src/qemu/qemu_monitor_json.c | 48 +- src/remote/remote_daemon_dispatch.c | 8 +- src/test/test_driver.c | 34 +- src/util/virtypedparam.c | 401 +++++++++++------ src/util/virtypedparam.h | 187 +++++--- ...othreads-ids-pool-sizes.x86_64-latest.args | 6 +- .../iothreads-ids-pool-sizes.xml | 12 +- tools/virsh-domain.c | 83 ++-- 21 files changed, 781 insertions(+), 706 deletions(-) -- 2.39.2

All callers pass 'true'. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/util/virtypedparam.c | 17 ++++++++--------- 1 file changed, 8 insertions(+), 9 deletions(-) diff --git a/src/util/virtypedparam.c b/src/util/virtypedparam.c index e6ad9ec7a9..0cca16053d 100644 --- a/src/util/virtypedparam.c +++ b/src/util/virtypedparam.c @@ -251,7 +251,6 @@ virTypedParameterAssignValueVArgs(virTypedParameterPtr param, static int virTypedParameterAssignValue(virTypedParameterPtr param, - bool copystr, int type, ...) { @@ -259,7 +258,7 @@ virTypedParameterAssignValue(virTypedParameterPtr param, va_list ap; va_start(ap, type); - ret = virTypedParameterAssignValueVArgs(param, type, ap, copystr); + ret = virTypedParameterAssignValueVArgs(param, type, ap, true); va_end(ap); return ret; @@ -785,7 +784,7 @@ virTypedParamListAddInt(virTypedParamList *list, int ret; if (!(par = virTypedParamListExtend(list)) || - virTypedParameterAssignValue(par, true, VIR_TYPED_PARAM_INT, value) < 0) + virTypedParameterAssignValue(par, VIR_TYPED_PARAM_INT, value) < 0) return -1; va_start(ap, namefmt); @@ -807,7 +806,7 @@ virTypedParamListAddUInt(virTypedParamList *list, int ret; if (!(par = virTypedParamListExtend(list)) || - virTypedParameterAssignValue(par, true, VIR_TYPED_PARAM_UINT, value) < 0) + virTypedParameterAssignValue(par, VIR_TYPED_PARAM_UINT, value) < 0) return -1; va_start(ap, namefmt); @@ -829,7 +828,7 @@ virTypedParamListAddLLong(virTypedParamList *list, int ret; if (!(par = virTypedParamListExtend(list)) || - virTypedParameterAssignValue(par, true, VIR_TYPED_PARAM_LLONG, value) < 0) + virTypedParameterAssignValue(par, VIR_TYPED_PARAM_LLONG, value) < 0) return -1; va_start(ap, namefmt); @@ -851,7 +850,7 @@ virTypedParamListAddULLong(virTypedParamList *list, int ret; if (!(par = virTypedParamListExtend(list)) || - virTypedParameterAssignValue(par, true, VIR_TYPED_PARAM_ULLONG, value) < 0) + virTypedParameterAssignValue(par, VIR_TYPED_PARAM_ULLONG, value) < 0) return -1; va_start(ap, namefmt); @@ -873,7 +872,7 @@ virTypedParamListAddString(virTypedParamList *list, int ret; if (!(par = virTypedParamListExtend(list)) || - virTypedParameterAssignValue(par, true, VIR_TYPED_PARAM_STRING, value) < 0) + virTypedParameterAssignValue(par, VIR_TYPED_PARAM_STRING, value) < 0) return -1; va_start(ap, namefmt); @@ -895,7 +894,7 @@ virTypedParamListAddBoolean(virTypedParamList *list, int ret; if (!(par = virTypedParamListExtend(list)) || - virTypedParameterAssignValue(par, true, VIR_TYPED_PARAM_BOOLEAN, value) < 0) + virTypedParameterAssignValue(par, VIR_TYPED_PARAM_BOOLEAN, value) < 0) return -1; va_start(ap, namefmt); @@ -917,7 +916,7 @@ virTypedParamListAddDouble(virTypedParamList *list, int ret; if (!(par = virTypedParamListExtend(list)) || - virTypedParameterAssignValue(par, true, VIR_TYPED_PARAM_DOUBLE, value) < 0) + virTypedParameterAssignValue(par, VIR_TYPED_PARAM_DOUBLE, value) < 0) return -1; va_start(ap, namefmt); -- 2.39.2

Ensure that all switch statements in this module use the proper type in switch() statements to ensure complier protections. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/util/virtypedparam.c | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/src/util/virtypedparam.c b/src/util/virtypedparam.c index 0cca16053d..974ec51a79 100644 --- a/src/util/virtypedparam.c +++ b/src/util/virtypedparam.c @@ -170,7 +170,7 @@ virTypedParameterToString(virTypedParameterPtr param) { char *value = NULL; - switch (param->type) { + switch ((virTypedParameterType) param->type) { case VIR_TYPED_PARAM_INT: value = g_strdup_printf("%d", param->value.i); break; @@ -192,6 +192,7 @@ virTypedParameterToString(virTypedParameterPtr param) case VIR_TYPED_PARAM_STRING: value = g_strdup(param->value.s); break; + case VIR_TYPED_PARAM_LAST: default: virReportError(VIR_ERR_INTERNAL_ERROR, _("unexpected type %1$d for field %2$s"), @@ -204,7 +205,7 @@ virTypedParameterToString(virTypedParameterPtr param) static int virTypedParameterAssignValueVArgs(virTypedParameterPtr param, - int type, + virTypedParameterType type, va_list ap, bool copystr) { @@ -238,6 +239,7 @@ virTypedParameterAssignValueVArgs(virTypedParameterPtr param, if (!param->value.s) param->value.s = g_strdup(""); break; + case VIR_TYPED_PARAM_LAST: default: virReportError(VIR_ERR_INTERNAL_ERROR, _("unexpected type %1$d for field %2$s"), type, @@ -559,7 +561,7 @@ virTypedParamsDeserialize(struct _virTypedParameterRemote *remote_params, } param->type = remote_param->value.type; - switch (param->type) { + switch ((virTypedParameterType) param->type) { case VIR_TYPED_PARAM_INT: param->value.i = remote_param->value.remote_typed_param_value.i; @@ -587,6 +589,7 @@ virTypedParamsDeserialize(struct _virTypedParameterRemote *remote_params, case VIR_TYPED_PARAM_STRING: param->value.s = g_strdup(remote_param->value.remote_typed_param_value.s); break; + case VIR_TYPED_PARAM_LAST: default: virReportError(VIR_ERR_RPC, _("unknown parameter type: %1$d"), param->type); @@ -670,7 +673,7 @@ virTypedParamsSerialize(virTypedParameterPtr params, * depending on the calling side, i.e. server or client */ val->field = g_strdup(param->field); val->value.type = param->type; - switch (param->type) { + switch ((virTypedParameterType) param->type) { case VIR_TYPED_PARAM_INT: val->value.remote_typed_param_value.i = param->value.i; break; @@ -692,6 +695,7 @@ virTypedParamsSerialize(virTypedParameterPtr params, case VIR_TYPED_PARAM_STRING: val->value.remote_typed_param_value.s = g_strdup(param->value.s); break; + case VIR_TYPED_PARAM_LAST: default: virReportError(VIR_ERR_RPC, _("unknown parameter type: %1$d"), param->type); -- 2.39.2

On Wed, Apr 19, 2023 at 02:04:19PM +0200, Peter Krempa wrote:
Ensure that all switch statements in this module use the proper type in switch() statements to ensure complier protections.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/util/virtypedparam.c | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-)
diff --git a/src/util/virtypedparam.c b/src/util/virtypedparam.c index 0cca16053d..974ec51a79 100644 --- a/src/util/virtypedparam.c +++ b/src/util/virtypedparam.c @@ -170,7 +170,7 @@ virTypedParameterToString(virTypedParameterPtr param) { char *value = NULL;
- switch (param->type) { + switch ((virTypedParameterType) param->type) { case VIR_TYPED_PARAM_INT: value = g_strdup_printf("%d", param->value.i); break; @@ -192,6 +192,7 @@ virTypedParameterToString(virTypedParameterPtr param) case VIR_TYPED_PARAM_STRING: value = g_strdup(param->value.s); break; + case VIR_TYPED_PARAM_LAST: default:
To ensure compiler protection in switch() statements you should also remove the default case from the switch statements. With that changed: Reviewed-by: Martin Kletzander <mkletzan@redhat.com>

On Wed, Apr 19, 2023 at 15:02:34 +0200, Martin Kletzander wrote:
On Wed, Apr 19, 2023 at 02:04:19PM +0200, Peter Krempa wrote:
Ensure that all switch statements in this module use the proper type in switch() statements to ensure complier protections.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/util/virtypedparam.c | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-)
diff --git a/src/util/virtypedparam.c b/src/util/virtypedparam.c index 0cca16053d..974ec51a79 100644 --- a/src/util/virtypedparam.c +++ b/src/util/virtypedparam.c @@ -170,7 +170,7 @@ virTypedParameterToString(virTypedParameterPtr param) { char *value = NULL;
- switch (param->type) { + switch ((virTypedParameterType) param->type) { case VIR_TYPED_PARAM_INT: value = g_strdup_printf("%d", param->value.i); break; @@ -192,6 +192,7 @@ virTypedParameterToString(virTypedParameterPtr param) case VIR_TYPED_PARAM_STRING: value = g_strdup(param->value.s); break; + case VIR_TYPED_PARAM_LAST: default:
To ensure compiler protection in switch() statements you should also remove the default case from the switch statements.
It actually works properly even with the 'default' case being present. Additionally in case a caller passes in something which is not actually a proper enum value (e.g. by typecasting) the 'default' case will be applied. In many new places we already do it like this.
With that changed:
Reviewed-by: Martin Kletzander <mkletzan@redhat.com>

On Wed, Apr 19, 2023 at 03:08:03PM +0200, Peter Krempa wrote:
On Wed, Apr 19, 2023 at 15:02:34 +0200, Martin Kletzander wrote:
On Wed, Apr 19, 2023 at 02:04:19PM +0200, Peter Krempa wrote:
Ensure that all switch statements in this module use the proper type in switch() statements to ensure complier protections.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/util/virtypedparam.c | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-)
diff --git a/src/util/virtypedparam.c b/src/util/virtypedparam.c index 0cca16053d..974ec51a79 100644 --- a/src/util/virtypedparam.c +++ b/src/util/virtypedparam.c @@ -170,7 +170,7 @@ virTypedParameterToString(virTypedParameterPtr param) { char *value = NULL;
- switch (param->type) { + switch ((virTypedParameterType) param->type) { case VIR_TYPED_PARAM_INT: value = g_strdup_printf("%d", param->value.i); break; @@ -192,6 +192,7 @@ virTypedParameterToString(virTypedParameterPtr param) case VIR_TYPED_PARAM_STRING: value = g_strdup(param->value.s); break; + case VIR_TYPED_PARAM_LAST: default:
To ensure compiler protection in switch() statements you should also remove the default case from the switch statements.
It actually works properly even with the 'default' case being present. Additionally in case a caller passes in something which is not actually a proper enum value (e.g. by typecasting) the 'default' case will be applied.
Oh, good to know, TIL then. In the meantime I replied as well =)
In many new places we already do it like this.
With that changed:
Reviewed-by: Martin Kletzander <mkletzan@redhat.com>

On Wed, Apr 19, 2023 at 03:02:34PM +0200, Martin Kletzander wrote:
On Wed, Apr 19, 2023 at 02:04:19PM +0200, Peter Krempa wrote:
Ensure that all switch statements in this module use the proper type in switch() statements to ensure complier protections.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/util/virtypedparam.c | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-)
diff --git a/src/util/virtypedparam.c b/src/util/virtypedparam.c index 0cca16053d..974ec51a79 100644 --- a/src/util/virtypedparam.c +++ b/src/util/virtypedparam.c @@ -170,7 +170,7 @@ virTypedParameterToString(virTypedParameterPtr param) { char *value = NULL;
- switch (param->type) { + switch ((virTypedParameterType) param->type) { case VIR_TYPED_PARAM_INT: value = g_strdup_printf("%d", param->value.i); break; @@ -192,6 +192,7 @@ virTypedParameterToString(virTypedParameterPtr param) case VIR_TYPED_PARAM_STRING: value = g_strdup(param->value.s); break; + case VIR_TYPED_PARAM_LAST: default:
To ensure compiler protection in switch() statements you should also remove the default case from the switch statements.
With that changed:
Reviewed-by: Martin Kletzander <mkletzan@redhat.com>
I missed that the type is an int which is not checked anywhere else, so the default branch needs to stay there, unfortunately that will not ensure compiler protection, except in that one case which you fix in a follow-up. So feel free to keep this one as is even though the commit message is a bit misleading.

All changed lines even fit into 80 columns. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/util/virtypedparam.c | 18 ++++++------------ 1 file changed, 6 insertions(+), 12 deletions(-) diff --git a/src/util/virtypedparam.c b/src/util/virtypedparam.c index 974ec51a79..fe4c04bcea 100644 --- a/src/util/virtypedparam.c +++ b/src/util/virtypedparam.c @@ -563,28 +563,22 @@ virTypedParamsDeserialize(struct _virTypedParameterRemote *remote_params, param->type = remote_param->value.type; switch ((virTypedParameterType) param->type) { case VIR_TYPED_PARAM_INT: - param->value.i = - remote_param->value.remote_typed_param_value.i; + param->value.i = remote_param->value.remote_typed_param_value.i; break; case VIR_TYPED_PARAM_UINT: - param->value.ui = - remote_param->value.remote_typed_param_value.ui; + param->value.ui = remote_param->value.remote_typed_param_value.ui; break; case VIR_TYPED_PARAM_LLONG: - param->value.l = - remote_param->value.remote_typed_param_value.l; + param->value.l = remote_param->value.remote_typed_param_value.l; break; case VIR_TYPED_PARAM_ULLONG: - param->value.ul = - remote_param->value.remote_typed_param_value.ul; + param->value.ul = remote_param->value.remote_typed_param_value.ul; break; case VIR_TYPED_PARAM_DOUBLE: - param->value.d = - remote_param->value.remote_typed_param_value.d; + param->value.d = remote_param->value.remote_typed_param_value.d; break; case VIR_TYPED_PARAM_BOOLEAN: - param->value.b = - remote_param->value.remote_typed_param_value.b; + param->value.b = remote_param->value.remote_typed_param_value.b; break; case VIR_TYPED_PARAM_STRING: param->value.s = g_strdup(remote_param->value.remote_typed_param_value.s); -- 2.39.2

On Wed, Apr 19, 2023 at 02:04:20PM +0200, Peter Krempa wrote:
All changed lines even fit into 80 columns.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Martin Kletzander <mkletzan@redhat.com>

There are two callers of virTypedParameterAssignValueVArgs. - 'virTypedParameterAssignValue' always uses the correct type, thus doesn't need to be modified. Just use the proper type in the function declaration - 'virTypedParameterAssign' can get improper type, but we can move the validation into it decreasing the scope in which failures need to be propagated. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/util/virtypedparam.c | 29 ++++++++++++++--------------- 1 file changed, 14 insertions(+), 15 deletions(-) diff --git a/src/util/virtypedparam.c b/src/util/virtypedparam.c index fe4c04bcea..f325f3b012 100644 --- a/src/util/virtypedparam.c +++ b/src/util/virtypedparam.c @@ -203,7 +203,7 @@ virTypedParameterToString(virTypedParameterPtr param) } -static int +static void virTypedParameterAssignValueVArgs(virTypedParameterPtr param, virTypedParameterType type, va_list ap, @@ -240,30 +240,23 @@ virTypedParameterAssignValueVArgs(virTypedParameterPtr param, param->value.s = g_strdup(""); break; case VIR_TYPED_PARAM_LAST: - default: - virReportError(VIR_ERR_INTERNAL_ERROR, - _("unexpected type %1$d for field %2$s"), type, - NULLSTR(param->field)); - return -1; + break; } - - return 0; } static int virTypedParameterAssignValue(virTypedParameterPtr param, - int type, + virTypedParameterType type, ...) { - int ret; va_list ap; va_start(ap, type); - ret = virTypedParameterAssignValueVArgs(param, type, ap, true); + virTypedParameterAssignValueVArgs(param, type, ap, true); va_end(ap); - return ret; + return 0; } @@ -276,7 +269,6 @@ virTypedParameterAssign(virTypedParameterPtr param, const char *name, int type, ...) { va_list ap; - int ret = -1; if (virStrcpyStatic(param->field, name) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, _("Field name '%1$s' too long"), @@ -284,11 +276,18 @@ virTypedParameterAssign(virTypedParameterPtr param, const char *name, return -1; } + if (type < VIR_TYPED_PARAM_INT || + type >= VIR_TYPED_PARAM_LAST) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("unexpected type %1$d for field %2$s"), type, name); + return -1; + } + va_start(ap, type); - ret = virTypedParameterAssignValueVArgs(param, type, ap, false); + virTypedParameterAssignValueVArgs(param, type, ap, false); va_end(ap); - return ret; + return 0; } -- 2.39.2

On Wed, Apr 19, 2023 at 02:04:21PM +0200, Peter Krempa wrote:
There are two callers of virTypedParameterAssignValueVArgs.
- 'virTypedParameterAssignValue' always uses the correct type, thus doesn't need to be modified. Just use the proper type in the function declaration
- 'virTypedParameterAssign' can get improper type, but we can move the validation into it decreasing the scope in which failures need to be propagated.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Martin Kletzander <mkletzan@redhat.com>

Don't check the return value of 'virTypedParamListExtend' which will always be a valid pointer and 'virTypedParameterAssignValue' always returns 0. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/util/virtypedparam.c | 46 +++++++++++++--------------------------- 1 file changed, 15 insertions(+), 31 deletions(-) diff --git a/src/util/virtypedparam.c b/src/util/virtypedparam.c index f325f3b012..1a1a34fdb2 100644 --- a/src/util/virtypedparam.c +++ b/src/util/virtypedparam.c @@ -245,7 +245,7 @@ virTypedParameterAssignValueVArgs(virTypedParameterPtr param, } -static int +static void virTypedParameterAssignValue(virTypedParameterPtr param, virTypedParameterType type, ...) @@ -255,8 +255,6 @@ virTypedParameterAssignValue(virTypedParameterPtr param, va_start(ap, type); virTypedParameterAssignValueVArgs(param, type, ap, true); va_end(ap); - - return 0; } @@ -776,13 +774,11 @@ virTypedParamListAddInt(virTypedParamList *list, const char *namefmt, ...) { - virTypedParameterPtr par; + virTypedParameterPtr par = virTypedParamListExtend(list); va_list ap; int ret; - if (!(par = virTypedParamListExtend(list)) || - virTypedParameterAssignValue(par, VIR_TYPED_PARAM_INT, value) < 0) - return -1; + virTypedParameterAssignValue(par, VIR_TYPED_PARAM_INT, value); va_start(ap, namefmt); ret = virTypedParamSetNameVPrintf(par, namefmt, ap); @@ -798,13 +794,11 @@ virTypedParamListAddUInt(virTypedParamList *list, const char *namefmt, ...) { - virTypedParameterPtr par; + virTypedParameterPtr par = virTypedParamListExtend(list); va_list ap; int ret; - if (!(par = virTypedParamListExtend(list)) || - virTypedParameterAssignValue(par, VIR_TYPED_PARAM_UINT, value) < 0) - return -1; + virTypedParameterAssignValue(par, VIR_TYPED_PARAM_UINT, value); va_start(ap, namefmt); ret = virTypedParamSetNameVPrintf(par, namefmt, ap); @@ -820,13 +814,11 @@ virTypedParamListAddLLong(virTypedParamList *list, const char *namefmt, ...) { - virTypedParameterPtr par; + virTypedParameterPtr par = virTypedParamListExtend(list); va_list ap; int ret; - if (!(par = virTypedParamListExtend(list)) || - virTypedParameterAssignValue(par, VIR_TYPED_PARAM_LLONG, value) < 0) - return -1; + virTypedParameterAssignValue(par, VIR_TYPED_PARAM_LLONG, value); va_start(ap, namefmt); ret = virTypedParamSetNameVPrintf(par, namefmt, ap); @@ -842,13 +834,11 @@ virTypedParamListAddULLong(virTypedParamList *list, const char *namefmt, ...) { - virTypedParameterPtr par; + virTypedParameterPtr par = virTypedParamListExtend(list); va_list ap; int ret; - if (!(par = virTypedParamListExtend(list)) || - virTypedParameterAssignValue(par, VIR_TYPED_PARAM_ULLONG, value) < 0) - return -1; + virTypedParameterAssignValue(par, VIR_TYPED_PARAM_ULLONG, value); va_start(ap, namefmt); ret = virTypedParamSetNameVPrintf(par, namefmt, ap); @@ -864,13 +854,11 @@ virTypedParamListAddString(virTypedParamList *list, const char *namefmt, ...) { - virTypedParameterPtr par; + virTypedParameterPtr par = virTypedParamListExtend(list); va_list ap; int ret; - if (!(par = virTypedParamListExtend(list)) || - virTypedParameterAssignValue(par, VIR_TYPED_PARAM_STRING, value) < 0) - return -1; + virTypedParameterAssignValue(par, VIR_TYPED_PARAM_STRING, value); va_start(ap, namefmt); ret = virTypedParamSetNameVPrintf(par, namefmt, ap); @@ -886,13 +874,11 @@ virTypedParamListAddBoolean(virTypedParamList *list, const char *namefmt, ...) { - virTypedParameterPtr par; + virTypedParameterPtr par = virTypedParamListExtend(list); va_list ap; int ret; - if (!(par = virTypedParamListExtend(list)) || - virTypedParameterAssignValue(par, VIR_TYPED_PARAM_BOOLEAN, value) < 0) - return -1; + virTypedParameterAssignValue(par, VIR_TYPED_PARAM_BOOLEAN, value); va_start(ap, namefmt); ret = virTypedParamSetNameVPrintf(par, namefmt, ap); @@ -908,13 +894,11 @@ virTypedParamListAddDouble(virTypedParamList *list, const char *namefmt, ...) { - virTypedParameterPtr par; + virTypedParameterPtr par = virTypedParamListExtend(list); va_list ap; int ret; - if (!(par = virTypedParamListExtend(list)) || - virTypedParameterAssignValue(par, VIR_TYPED_PARAM_DOUBLE, value) < 0) - return -1; + virTypedParameterAssignValue(par, VIR_TYPED_PARAM_DOUBLE, value); va_start(ap, namefmt); ret = virTypedParamSetNameVPrintf(par, namefmt, ap); -- 2.39.2

On Wed, Apr 19, 2023 at 02:04:22PM +0200, Peter Krempa wrote:
Don't check the return value of 'virTypedParamListExtend' which will always be a valid pointer and 'virTypedParameterAssignValue' always returns 0.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Martin Kletzander <mkletzan@redhat.com>

The header uses both styles randomly, switch it to the contemporary style. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/util/virtypedparam.h | 140 ++++++++++++++++++++++----------------- 1 file changed, 80 insertions(+), 60 deletions(-) diff --git a/src/util/virtypedparam.h b/src/util/virtypedparam.h index 936dac24ea..fb9815c661 100644 --- a/src/util/virtypedparam.h +++ b/src/util/virtypedparam.h @@ -55,14 +55,17 @@ struct _virTypedParameterRemote { }; -int virTypedParamsValidate(virTypedParameterPtr params, int nparams, - /* const char *name, int type ... */ ...) +int +virTypedParamsValidate(virTypedParameterPtr params, + int nparams, + /* const char *name, int type ... */ ...) G_GNUC_NULL_TERMINATED G_GNUC_WARN_UNUSED_RESULT; -bool virTypedParamsCheck(virTypedParameterPtr params, - int nparams, - const char **names, - int nnames); +bool +virTypedParamsCheck(virTypedParameterPtr params, + int nparams, + const char **names, + int nnames); int virTypedParamsGetStringList(virTypedParameterPtr params, @@ -77,36 +80,44 @@ virTypedParamsFilter(virTypedParameterPtr params, G_GNUC_WARN_UNUSED_RESULT; -int virTypedParameterAssign(virTypedParameterPtr param, const char *name, - int type, /* TYPE arg */ ...) +int +virTypedParameterAssign(virTypedParameterPtr param, + const char *name, + int type, /* TYPE arg */ ...) G_GNUC_WARN_UNUSED_RESULT; -int virTypedParamsReplaceString(virTypedParameterPtr *params, - int *nparams, - const char *name, - const char *value); +int +virTypedParamsReplaceString(virTypedParameterPtr *params, + int *nparams, + const char *name, + const char *value); -void virTypedParamsCopy(virTypedParameterPtr *dst, - virTypedParameterPtr src, - int nparams); +void +virTypedParamsCopy(virTypedParameterPtr *dst, + virTypedParameterPtr src, + int nparams); -char *virTypedParameterToString(virTypedParameterPtr param); +char * +virTypedParameterToString(virTypedParameterPtr param); -void virTypedParamsRemoteFree(struct _virTypedParameterRemote *remote_params_val, - unsigned int remote_params_len); +void +virTypedParamsRemoteFree(struct _virTypedParameterRemote *remote_params_val, + unsigned int remote_params_len); -int virTypedParamsDeserialize(struct _virTypedParameterRemote *remote_params, - unsigned int remote_params_len, - int limit, - virTypedParameterPtr *params, - int *nparams); +int +virTypedParamsDeserialize(struct _virTypedParameterRemote *remote_params, + unsigned int remote_params_len, + int limit, + virTypedParameterPtr *params, + int *nparams); -int virTypedParamsSerialize(virTypedParameterPtr params, - int nparams, - int limit, - struct _virTypedParameterRemote **remote_params_val, - unsigned int *remote_params_len, - unsigned int flags); +int +virTypedParamsSerialize(virTypedParameterPtr params, + int nparams, + int limit, + struct _virTypedParameterRemote **remote_params_val, + unsigned int *remote_params_len, + unsigned int flags); VIR_ENUM_DECL(virTypedParameter); @@ -132,48 +143,57 @@ struct _virTypedParamList { size_t par_alloc; }; -void virTypedParamListFree(virTypedParamList *list); +void +virTypedParamListFree(virTypedParamList *list); G_DEFINE_AUTOPTR_CLEANUP_FUNC(virTypedParamList, virTypedParamListFree); -size_t virTypedParamListStealParams(virTypedParamList *list, - virTypedParameterPtr *params); +size_t +virTypedParamListStealParams(virTypedParamList *list, + virTypedParameterPtr *params); virTypedParamList * virTypedParamListFromParams(virTypedParameterPtr *params, size_t nparams); -int virTypedParamListAddInt(virTypedParamList *list, - int value, - const char *namefmt, - ...) +int +virTypedParamListAddInt(virTypedParamList *list, + int value, + const char *namefmt, + ...) G_GNUC_PRINTF(3, 4) G_GNUC_WARN_UNUSED_RESULT; -int virTypedParamListAddUInt(virTypedParamList *list, - unsigned int value, - const char *namefmt, - ...) +int +virTypedParamListAddUInt(virTypedParamList *list, + unsigned int value, + const char *namefmt, + ...) G_GNUC_PRINTF(3, 4) G_GNUC_WARN_UNUSED_RESULT; -int virTypedParamListAddLLong(virTypedParamList *list, - long long value, - const char *namefmt, - ...) +int +virTypedParamListAddLLong(virTypedParamList *list, + long long value, + const char *namefmt, + ...) G_GNUC_PRINTF(3, 4) G_GNUC_WARN_UNUSED_RESULT; -int virTypedParamListAddULLong(virTypedParamList *list, - unsigned long long value, - const char *namefmt, - ...) +int +virTypedParamListAddULLong(virTypedParamList *list, + unsigned long long value, + const char *namefmt, + ...) G_GNUC_PRINTF(3, 4) G_GNUC_WARN_UNUSED_RESULT; -int virTypedParamListAddString(virTypedParamList *list, - const char *value, - const char *namefmt, - ...) +int +virTypedParamListAddString(virTypedParamList *list, + const char *value, + const char *namefmt, + ...) G_GNUC_PRINTF(3, 4) G_GNUC_WARN_UNUSED_RESULT; -int virTypedParamListAddBoolean(virTypedParamList *list, - bool value, - const char *namefmt, - ...) +int +virTypedParamListAddBoolean(virTypedParamList *list, + bool value, + const char *namefmt, + ...) G_GNUC_PRINTF(3, 4) G_GNUC_WARN_UNUSED_RESULT; -int virTypedParamListAddDouble(virTypedParamList *list, - double value, - const char *namefmt, - ...) +int +virTypedParamListAddDouble(virTypedParamList *list, + double value, + const char *namefmt, + ...) G_GNUC_PRINTF(3, 4) G_GNUC_WARN_UNUSED_RESULT; -- 2.39.2

On Wed, Apr 19, 2023 at 02:04:23PM +0200, Peter Krempa wrote:
The header uses both styles randomly, switch it to the contemporary style.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Martin Kletzander <mkletzan@redhat.com>

Add an allocator function and refactor all allocations to use it. In upcoming patches 'struct _virTypedParamList' will be made private. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/admin/admin_server.c | 6 +++--- src/libvirt_private.syms | 1 + src/qemu/qemu_domainjob.c | 2 +- src/qemu/qemu_driver.c | 2 +- src/test/test_driver.c | 2 +- src/util/virtypedparam.c | 9 ++++++++- src/util/virtypedparam.h | 1 + 7 files changed, 16 insertions(+), 7 deletions(-) diff --git a/src/admin/admin_server.c b/src/admin/admin_server.c index 7ed2645f57..394de0dfab 100644 --- a/src/admin/admin_server.c +++ b/src/admin/admin_server.c @@ -76,7 +76,7 @@ adminServerGetThreadPoolParameters(virNetServer *srv, size_t freeWorkers; size_t nPrioWorkers; size_t jobQueueDepth; - g_autoptr(virTypedParamList) paramlist = g_new0(virTypedParamList, 1); + g_autoptr(virTypedParamList) paramlist = virTypedParamListNew(); virCheckFlags(0, -1); @@ -200,7 +200,7 @@ adminClientGetInfo(virNetServerClient *client, bool readonly; g_autofree char *sock_addr = NULL; const char *attr = NULL; - g_autoptr(virTypedParamList) paramlist = g_new0(virTypedParamList, 1); + g_autoptr(virTypedParamList) paramlist = virTypedParamListNew(); g_autoptr(virIdentity) identity = NULL; int rc; @@ -298,7 +298,7 @@ adminServerGetClientLimits(virNetServer *srv, int *nparams, unsigned int flags) { - g_autoptr(virTypedParamList) paramlist = g_new0(virTypedParamList, 1); + g_autoptr(virTypedParamList) paramlist = virTypedParamListNew(); virCheckFlags(0, -1); diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 1247b67a39..c596ef0f87 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -3557,6 +3557,7 @@ virTypedParamListAddUInt; virTypedParamListAddULLong; virTypedParamListFree; virTypedParamListFromParams; +virTypedParamListNew; virTypedParamListStealParams; virTypedParamsCheck; virTypedParamsCopy; diff --git a/src/qemu/qemu_domainjob.c b/src/qemu/qemu_domainjob.c index 6376f928d5..4df1af0ce7 100644 --- a/src/qemu/qemu_domainjob.c +++ b/src/qemu/qemu_domainjob.c @@ -478,7 +478,7 @@ qemuDomainBackupJobDataToParams(virDomainJobData *jobData, { qemuDomainJobDataPrivate *priv = jobData->privateData; qemuDomainBackupStats *stats = &priv->stats.backup; - g_autoptr(virTypedParamList) par = g_new0(virTypedParamList, 1); + g_autoptr(virTypedParamList) par = virTypedParamListNew(); if (virTypedParamListAddInt(par, jobData->operation, VIR_DOMAIN_JOB_OPERATION) < 0) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 28e470e4a2..4cf5e8a512 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -18447,7 +18447,7 @@ qemuDomainGetStats(virConnectPtr conn, g_autoptr(virTypedParamList) params = NULL; size_t i; - params = g_new0(virTypedParamList, 1); + params = virTypedParamListNew(); for (i = 0; qemuDomainGetStatsWorkers[i].func; i++) { if (stats & qemuDomainGetStatsWorkers[i].stats) { diff --git a/src/test/test_driver.c b/src/test/test_driver.c index 7b06896d44..47c74c420c 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -9858,7 +9858,7 @@ testDomainGetStats(virConnectPtr conn, g_autoptr(virTypedParamList) params = NULL; size_t i; - params = g_new0(virTypedParamList, 1); + params = virTypedParamListNew(); for (i = 0; testDomainGetStatsWorkers[i].func; i++) { if (stats & testDomainGetStatsWorkers[i].stats) { diff --git a/src/util/virtypedparam.c b/src/util/virtypedparam.c index 1a1a34fdb2..de3a4e76b4 100644 --- a/src/util/virtypedparam.c +++ b/src/util/virtypedparam.c @@ -705,6 +705,13 @@ virTypedParamsSerialize(virTypedParameterPtr params, } +virTypedParamList * +virTypedParamListNew(void) +{ + return g_new0(virTypedParamList, 1); +} + + void virTypedParamListFree(virTypedParamList *list) { @@ -733,7 +740,7 @@ virTypedParamList * virTypedParamListFromParams(virTypedParameterPtr *params, size_t nparams) { - virTypedParamList *l = g_new0(virTypedParamList, 1); + virTypedParamList *l = virTypedParamListNew(); l->par = g_steal_pointer(params); l->npar = nparams; diff --git a/src/util/virtypedparam.h b/src/util/virtypedparam.h index fb9815c661..4aa597bc81 100644 --- a/src/util/virtypedparam.h +++ b/src/util/virtypedparam.h @@ -146,6 +146,7 @@ struct _virTypedParamList { void virTypedParamListFree(virTypedParamList *list); G_DEFINE_AUTOPTR_CLEANUP_FUNC(virTypedParamList, virTypedParamListFree); +virTypedParamList *virTypedParamListNew(void); size_t virTypedParamListStealParams(virTypedParamList *list, -- 2.39.2

On Wed, Apr 19, 2023 at 02:04:24PM +0200, Peter Krempa wrote:
Add an allocator function and refactor all allocations to use it. In upcoming patches 'struct _virTypedParamList' will be made private.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Martin Kletzander <mkletzan@redhat.com>

Introduce a helper function to concatenate two virTypedParamLists. This will allow us to refactor qemuDomainGetStatsBlock to not access the list directly. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/libvirt_private.syms | 1 + src/util/virtypedparam.c | 24 ++++++++++++++++++++++++ src/util/virtypedparam.h | 4 ++++ 3 files changed, 29 insertions(+) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index c596ef0f87..0f42c2de0b 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -3555,6 +3555,7 @@ virTypedParamListAddLLong; virTypedParamListAddString; virTypedParamListAddUInt; virTypedParamListAddULLong; +virTypedParamListConcat; virTypedParamListFree; virTypedParamListFromParams; virTypedParamListNew; diff --git a/src/util/virtypedparam.c b/src/util/virtypedparam.c index de3a4e76b4..f8dce9ed98 100644 --- a/src/util/virtypedparam.c +++ b/src/util/virtypedparam.c @@ -712,6 +712,30 @@ virTypedParamListNew(void) } +/** + * virTypedParamListConcat: + * + * @to: typed param list to concatenate into + * @fromptr: pointer to pointer to a typed param list to concatenate into @from + * + * Concatenates all params from the virTypedParamList pointed to by @fromptr + * into @to and deallocates the list pointed to by @fromptr and clears the + * variable. + */ +void +virTypedParamListConcat(virTypedParamList *to, + virTypedParamList **fromptr) +{ + g_autoptr(virTypedParamList) from = g_steal_pointer(fromptr); + + VIR_RESIZE_N(to->par, to->par_alloc, to->npar, from->npar); + + memcpy(to->par + to->npar, from->par, sizeof(*(to->par)) * from->npar); + to->npar += from->npar; + from->npar = 0; +} + + void virTypedParamListFree(virTypedParamList *list) { diff --git a/src/util/virtypedparam.h b/src/util/virtypedparam.h index 4aa597bc81..45422c2673 100644 --- a/src/util/virtypedparam.h +++ b/src/util/virtypedparam.h @@ -156,6 +156,10 @@ virTypedParamList * virTypedParamListFromParams(virTypedParameterPtr *params, size_t nparams); +void +virTypedParamListConcat(virTypedParamList *to, + virTypedParamList **fromptr); + int virTypedParamListAddInt(virTypedParamList *list, int value, -- 2.39.2

On a Wednesday in 2023, Peter Krempa wrote:
Introduce a helper function to concatenate two virTypedParamLists. This will allow us to refactor qemuDomainGetStatsBlock to not access the list directly.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/libvirt_private.syms | 1 + src/util/virtypedparam.c | 24 ++++++++++++++++++++++++ src/util/virtypedparam.h | 4 ++++ 3 files changed, 29 insertions(+)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index c596ef0f87..0f42c2de0b 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -3555,6 +3555,7 @@ virTypedParamListAddLLong; virTypedParamListAddString; virTypedParamListAddUInt; virTypedParamListAddULLong; +virTypedParamListConcat; virTypedParamListFree; virTypedParamListFromParams; virTypedParamListNew; diff --git a/src/util/virtypedparam.c b/src/util/virtypedparam.c index de3a4e76b4..f8dce9ed98 100644 --- a/src/util/virtypedparam.c +++ b/src/util/virtypedparam.c @@ -712,6 +712,30 @@ virTypedParamListNew(void) }
+/** + * virTypedParamListConcat: + * + * @to: typed param list to concatenate into + * @fromptr: pointer to pointer to a typed param list to concatenate into @from
*to concatenate into @to Jano

On Wed, Apr 19, 2023 at 02:04:25PM +0200, Peter Krempa wrote:
Introduce a helper function to concatenate two virTypedParamLists. This will allow us to refactor qemuDomainGetStatsBlock to not access the list directly.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/libvirt_private.syms | 1 + src/util/virtypedparam.c | 24 ++++++++++++++++++++++++ src/util/virtypedparam.h | 4 ++++ 3 files changed, 29 insertions(+)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index c596ef0f87..0f42c2de0b 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -3555,6 +3555,7 @@ virTypedParamListAddLLong; virTypedParamListAddString; virTypedParamListAddUInt; virTypedParamListAddULLong; +virTypedParamListConcat; virTypedParamListFree; virTypedParamListFromParams; virTypedParamListNew; diff --git a/src/util/virtypedparam.c b/src/util/virtypedparam.c index de3a4e76b4..f8dce9ed98 100644 --- a/src/util/virtypedparam.c +++ b/src/util/virtypedparam.c @@ -712,6 +712,30 @@ virTypedParamListNew(void) }
+/** + * virTypedParamListConcat: + * + * @to: typed param list to concatenate into + * @fromptr: pointer to pointer to a typed param list to concatenate into @from + * ^ | These two lines are terrible to read, but I think you meant @to here --------´
Reviewed-by: Martin Kletzander <mkletzan@redhat.com>

The struct will be made private in upcoming patches. Construct the list of block entries into a separate list and append them rather than remember the index of the count element. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_driver.c | 16 ++++++---------- 1 file changed, 6 insertions(+), 10 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 4cf5e8a512..b89bf176bb 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -18107,9 +18107,9 @@ qemuDomainGetStatsBlock(virQEMUDriver *driver, g_autoptr(GHashTable) stats = NULL; qemuDomainObjPrivate *priv = dom->privateData; g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver); - int count_index = -1; size_t visited = 0; bool visitBacking = !!(privflags & QEMU_DOMAIN_STATS_BACKING); + g_autoptr(virTypedParamList) blockparams = virTypedParamListNew(); if (HAVE_JOB(privflags) && virDomainObjIsActive(dom)) { qemuDomainObjEnterMonitor(dom); @@ -18126,21 +18126,17 @@ qemuDomainGetStatsBlock(virQEMUDriver *driver, virResetLastError(); } - /* When listing backing chains, it's easier to fix up the count - * after the iteration than it is to iterate twice; but we still - * want count listed first. */ - count_index = params->npar; - if (virTypedParamListAddUInt(params, 0, "block.count") < 0) - return -1; - for (i = 0; i < dom->def->ndisks; i++) { if (qemuDomainGetStatsBlockExportDisk(dom->def->disks[i], stats, - params, &visited, + blockparams, &visited, visitBacking, driver, cfg, dom) < 0) return -1; } - params->par[count_index].value.ui = visited; + if (virTypedParamListAddUInt(params, 0, "block.count") < 0) + return -1; + + virTypedParamListConcat(params, &blockparams); return 0; } -- 2.39.2

On a Wednesday in 2023, Peter Krempa wrote:
The struct will be made private in upcoming patches. Construct the list of block entries into a separate list and append them rather than remember the index of the count element.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_driver.c | 16 ++++++---------- 1 file changed, 6 insertions(+), 10 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 4cf5e8a512..b89bf176bb 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -18107,9 +18107,9 @@ qemuDomainGetStatsBlock(virQEMUDriver *driver, g_autoptr(GHashTable) stats = NULL; qemuDomainObjPrivate *priv = dom->privateData; g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver); - int count_index = -1; size_t visited = 0; bool visitBacking = !!(privflags & QEMU_DOMAIN_STATS_BACKING); + g_autoptr(virTypedParamList) blockparams = virTypedParamListNew();
if (HAVE_JOB(privflags) && virDomainObjIsActive(dom)) { qemuDomainObjEnterMonitor(dom); @@ -18126,21 +18126,17 @@ qemuDomainGetStatsBlock(virQEMUDriver *driver, virResetLastError(); }
- /* When listing backing chains, it's easier to fix up the count - * after the iteration than it is to iterate twice; but we still - * want count listed first. */ - count_index = params->npar; - if (virTypedParamListAddUInt(params, 0, "block.count") < 0) - return -1; - for (i = 0; i < dom->def->ndisks; i++) { if (qemuDomainGetStatsBlockExportDisk(dom->def->disks[i], stats, - params, &visited, + blockparams, &visited, visitBacking, driver, cfg, dom) < 0) return -1; }
- params->par[count_index].value.ui = visited; + if (virTypedParamListAddUInt(params, 0, "block.count") < 0)
s/0/visited/ Jano
+ return -1; + + virTypedParamListConcat(params, &blockparams);
return 0; } -- 2.39.2

On Wed, Apr 19, 2023 at 02:04:26PM +0200, Peter Krempa wrote:
The struct will be made private in upcoming patches. Construct the list of block entries into a separate list and append them rather than remember the index of the count element.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_driver.c | 16 ++++++---------- 1 file changed, 6 insertions(+), 10 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 4cf5e8a512..b89bf176bb 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -18107,9 +18107,9 @@ qemuDomainGetStatsBlock(virQEMUDriver *driver, g_autoptr(GHashTable) stats = NULL; qemuDomainObjPrivate *priv = dom->privateData; g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver); - int count_index = -1; size_t visited = 0; bool visitBacking = !!(privflags & QEMU_DOMAIN_STATS_BACKING); + g_autoptr(virTypedParamList) blockparams = virTypedParamListNew();
if (HAVE_JOB(privflags) && virDomainObjIsActive(dom)) { qemuDomainObjEnterMonitor(dom); @@ -18126,21 +18126,17 @@ qemuDomainGetStatsBlock(virQEMUDriver *driver, virResetLastError(); }
- /* When listing backing chains, it's easier to fix up the count - * after the iteration than it is to iterate twice; but we still - * want count listed first. */ - count_index = params->npar; - if (virTypedParamListAddUInt(params, 0, "block.count") < 0) - return -1; - for (i = 0; i < dom->def->ndisks; i++) { if (qemuDomainGetStatsBlockExportDisk(dom->def->disks[i], stats, - params, &visited, + blockparams, &visited, visitBacking, driver, cfg, dom) < 0) return -1; }
- params->par[count_index].value.ui = visited; + if (virTypedParamListAddUInt(params, 0, "block.count") < 0)
Shouldn't this be: if (virTypedParamListAddUInt(params, visited, "block.count") < 0) instead? With that changed: Reviewed-by: Martin Kletzander <mkletzan@redhat.com>
+ return -1; + + virTypedParamListConcat(params, &blockparams);
return 0; } -- 2.39.2

Introduce a helper that fetches the typed parameters from the list while still preserving ownership of the pointer by the list. In the future this will be also able to report errors stored in the list. Signed-off-by: Peter Krempa <pkrempa@redhat.com --- src/libvirt_private.syms | 1 + src/util/virtypedparam.c | 26 ++++++++++++++++++++++++++ src/util/virtypedparam.h | 6 ++++++ 3 files changed, 33 insertions(+) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 0f42c2de0b..f1999d0e99 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -3556,6 +3556,7 @@ virTypedParamListAddString; virTypedParamListAddUInt; virTypedParamListAddULLong; virTypedParamListConcat; +virTypedParamListFetch; virTypedParamListFree; virTypedParamListFromParams; virTypedParamListNew; diff --git a/src/util/virtypedparam.c b/src/util/virtypedparam.c index f8dce9ed98..6e837c65bc 100644 --- a/src/util/virtypedparam.c +++ b/src/util/virtypedparam.c @@ -747,6 +747,32 @@ virTypedParamListFree(virTypedParamList *list) } +/** + * virTypedParamListFetch: + * + * @list: virTypedParamList object + * @par: if not NULL filled with the typed parameters stored in @list + * @npar: if not NULL filled with the number of typed parameters stored in @list + * + * Checks that @list has no errors stored and optionally fills @par and @npar + * with a valid list of typed parametes. The typed parameters still belong to + * @list and will be freed together. + */ +int +virTypedParamListFetch(virTypedParamList *list, + virTypedParameterPtr *par, + size_t *npar) +{ + if (par) + *par = list->par; + + if (npar) + *npar = list->npar; + + return 0; +} + + size_t virTypedParamListStealParams(virTypedParamList *list, virTypedParameterPtr *params) diff --git a/src/util/virtypedparam.h b/src/util/virtypedparam.h index 45422c2673..628c951432 100644 --- a/src/util/virtypedparam.h +++ b/src/util/virtypedparam.h @@ -152,6 +152,12 @@ size_t virTypedParamListStealParams(virTypedParamList *list, virTypedParameterPtr *params); +int +virTypedParamListFetch(virTypedParamList *list, + virTypedParameterPtr *par, + size_t *npar) + G_GNUC_WARN_UNUSED_RESULT; + virTypedParamList * virTypedParamListFromParams(virTypedParameterPtr *params, size_t nparams); -- 2.39.2

On a Wednesday in 2023, Peter Krempa wrote:
Introduce a helper that fetches the typed parameters from the list while still preserving ownership of the pointer by the list.
In the future this will be also able to report errors stored in the list.
Signed-off-by: Peter Krempa <pkrempa@redhat.com --- src/libvirt_private.syms | 1 + src/util/virtypedparam.c | 26 ++++++++++++++++++++++++++ src/util/virtypedparam.h | 6 ++++++ 3 files changed, 33 insertions(+)
diff --git a/src/util/virtypedparam.c b/src/util/virtypedparam.c index f8dce9ed98..6e837c65bc 100644 --- a/src/util/virtypedparam.c +++ b/src/util/virtypedparam.c @@ -747,6 +747,32 @@ virTypedParamListFree(virTypedParamList *list) }
+/** + * virTypedParamListFetch: + * + * @list: virTypedParamList object + * @par: if not NULL filled with the typed parameters stored in @list + * @npar: if not NULL filled with the number of typed parameters stored in @list + * + * Checks that @list has no errors stored and optionally fills @par and @npar + * with a valid list of typed parametes. The typed parameters still belong to
*parameters
+ * @list and will be freed together. + */
Jano

Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/driver.c | 7 ++++++- src/remote/remote_daemon_dispatch.c | 8 +++++++- 2 files changed, 13 insertions(+), 2 deletions(-) diff --git a/src/driver.c b/src/driver.c index c7a9c2659f..35f8156605 100644 --- a/src/driver.c +++ b/src/driver.c @@ -165,8 +165,13 @@ virGetConnectGeneric(virThreadLocal *threadPtr, const char *name) ident = virIdentityGetCurrent(); if (ident) { g_autoptr(virTypedParamList) tmp = virIdentityGetParameters(ident); + virTypedParameterPtr par; + size_t npar; - if (virConnectSetIdentity(conn, tmp->par, tmp->npar, 0) < 0) + if (virTypedParamListFetch(tmp, &par, &npar) < 0) + goto error; + + if (virConnectSetIdentity(conn, par, npar, 0) < 0) goto error; } } diff --git a/src/remote/remote_daemon_dispatch.c b/src/remote/remote_daemon_dispatch.c index 4fc83dbd90..7144e9e7ca 100644 --- a/src/remote/remote_daemon_dispatch.c +++ b/src/remote/remote_daemon_dispatch.c @@ -1824,7 +1824,13 @@ remoteOpenConn(const char *uri, VIR_DEBUG("Opened driver %p", newconn); if (preserveIdentity) { - if (virConnectSetIdentity(newconn, identparams->par, identparams->npar, 0) < 0) + virTypedParameterPtr par; + size_t npar; + + if (virTypedParamListFetch(identparams, &par, &npar) < 0) + return -1; + + if (virConnectSetIdentity(newconn, par, npar, 0) < 0) return -1; VIR_DEBUG("Forwarded current identity to secondary driver"); -- 2.39.2

Ensure that all callers access it via the APIs. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/util/virtypedparam.c | 7 +++++++ src/util/virtypedparam.h | 5 ----- 2 files changed, 7 insertions(+), 5 deletions(-) diff --git a/src/util/virtypedparam.c b/src/util/virtypedparam.c index 6e837c65bc..e03e112d1e 100644 --- a/src/util/virtypedparam.c +++ b/src/util/virtypedparam.c @@ -705,6 +705,13 @@ virTypedParamsSerialize(virTypedParameterPtr params, } +struct _virTypedParamList { + virTypedParameterPtr par; + size_t npar; + size_t par_alloc; +}; + + virTypedParamList * virTypedParamListNew(void) { diff --git a/src/util/virtypedparam.h b/src/util/virtypedparam.h index 628c951432..b6ad209e25 100644 --- a/src/util/virtypedparam.h +++ b/src/util/virtypedparam.h @@ -137,11 +137,6 @@ VIR_ENUM_DECL(virTypedParameter); } while (0) typedef struct _virTypedParamList virTypedParamList; -struct _virTypedParamList { - virTypedParameterPtr par; - size_t npar; - size_t par_alloc; -}; void virTypedParamListFree(virTypedParamList *list); -- 2.39.2

Return the number of parameters via pointer passed as argument to free up possibility to report errors. Strangely all callers actually use 'int' as type for storing the count of elements, thus this function will use the same. The function is also renamed to virTypedParamListSteal. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/admin/admin_server.c | 10 +++++++--- src/libvirt_private.syms | 2 +- src/qemu/qemu_domainjob.c | 4 +++- src/qemu/qemu_driver.c | 4 +++- src/test/test_driver.c | 4 +++- src/util/virtypedparam.c | 20 ++++++++++++++------ src/util/virtypedparam.h | 7 ++++--- 7 files changed, 35 insertions(+), 16 deletions(-) diff --git a/src/admin/admin_server.c b/src/admin/admin_server.c index 394de0dfab..1d1ae97f2f 100644 --- a/src/admin/admin_server.c +++ b/src/admin/admin_server.c @@ -113,7 +113,8 @@ adminServerGetThreadPoolParameters(virNetServer *srv, "%s", VIR_THREADPOOL_JOB_QUEUE_DEPTH) < 0) return -1; - *nparams = virTypedParamListStealParams(paramlist, params); + if (virTypedParamListSteal(paramlist, params, nparams) < 0) + return -1; return 0; } @@ -279,7 +280,9 @@ adminClientGetInfo(virNetServerClient *client, "%s", VIR_CLIENT_INFO_SELINUX_CONTEXT) < 0) return -1; - *nparams = virTypedParamListStealParams(paramlist, params); + if (virTypedParamListSteal(paramlist, params, nparams) < 0) + return -1; + return 0; } @@ -322,7 +325,8 @@ adminServerGetClientLimits(virNetServer *srv, "%s", VIR_SERVER_CLIENTS_UNAUTH_CURRENT) < 0) return -1; - *nparams = virTypedParamListStealParams(paramlist, params); + if (virTypedParamListSteal(paramlist, params, nparams) < 0) + return -1; return 0; } diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index f1999d0e99..b58be6aa33 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -3560,7 +3560,7 @@ virTypedParamListFetch; virTypedParamListFree; virTypedParamListFromParams; virTypedParamListNew; -virTypedParamListStealParams; +virTypedParamListSteal; virTypedParamsCheck; virTypedParamsCopy; virTypedParamsDeserialize; diff --git a/src/qemu/qemu_domainjob.c b/src/qemu/qemu_domainjob.c index 4df1af0ce7..6be48118b9 100644 --- a/src/qemu/qemu_domainjob.c +++ b/src/qemu/qemu_domainjob.c @@ -522,7 +522,9 @@ qemuDomainBackupJobDataToParams(virDomainJobData *jobData, virTypedParamListAddString(par, jobData->errmsg, VIR_DOMAIN_JOB_ERRMSG) < 0) return -1; - *nparams = virTypedParamListStealParams(par, params); + if (virTypedParamListSteal(par, params, nparams) < 0) + return -1; + *type = virDomainJobStatusToType(jobData->status); return 0; } diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index b89bf176bb..9c409948f0 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -18459,7 +18459,9 @@ qemuDomainGetStats(virConnectPtr conn, dom->def->uuid, dom->def->id))) return -1; - tmp->nparams = virTypedParamListStealParams(params, &tmp->params); + if (virTypedParamListSteal(params, &tmp->params, &tmp->nparams) < 0) + return -1; + *record = g_steal_pointer(&tmp); return 0; } diff --git a/src/test/test_driver.c b/src/test/test_driver.c index 47c74c420c..ac12488de9 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -9873,7 +9873,9 @@ testDomainGetStats(virConnectPtr conn, dom->def->uuid, dom->def->id))) return -1; - tmp->nparams = virTypedParamListStealParams(params, &tmp->params); + if (virTypedParamListSteal(params, &tmp->params, &tmp->nparams) < 0) + return -1; + *record = g_steal_pointer(&tmp); return 0; } diff --git a/src/util/virtypedparam.c b/src/util/virtypedparam.c index e03e112d1e..68698fe583 100644 --- a/src/util/virtypedparam.c +++ b/src/util/virtypedparam.c @@ -780,19 +780,27 @@ virTypedParamListFetch(virTypedParamList *list, } -size_t -virTypedParamListStealParams(virTypedParamList *list, - virTypedParameterPtr *params) +int +virTypedParamListSteal(virTypedParamList *list, + virTypedParameterPtr *par, + int *npar) { - size_t ret = list->npar; + size_t nparams; + + if (virTypedParamListFetch(list, par, &nparams) < 0) + return -1; - *params = g_steal_pointer(&list->par); + /* most callers expect 'int', so help them out */ + *npar = nparams; + + list->par = NULL; list->npar = 0; list->par_alloc = 0; - return ret; + return 0; } + virTypedParamList * virTypedParamListFromParams(virTypedParameterPtr *params, size_t nparams) diff --git a/src/util/virtypedparam.h b/src/util/virtypedparam.h index b6ad209e25..b2869173e1 100644 --- a/src/util/virtypedparam.h +++ b/src/util/virtypedparam.h @@ -143,9 +143,10 @@ virTypedParamListFree(virTypedParamList *list); G_DEFINE_AUTOPTR_CLEANUP_FUNC(virTypedParamList, virTypedParamListFree); virTypedParamList *virTypedParamListNew(void); -size_t -virTypedParamListStealParams(virTypedParamList *list, - virTypedParameterPtr *params); +int +virTypedParamListSteal(virTypedParamList *list, + virTypedParameterPtr *par, + int *npar); int virTypedParamListFetch(virTypedParamList *list, -- 2.39.2

The only non-abort()-ing error which can happen is if the field name is too long. Store the overly long name in the virTypedParamList container so that in upcoming patches the helpers adding to the list can be refactored to not have a return value. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/util/virtypedparam.c | 62 ++++++++++++++++++++++------------------ 1 file changed, 34 insertions(+), 28 deletions(-) diff --git a/src/util/virtypedparam.c b/src/util/virtypedparam.c index 68698fe583..046164d36c 100644 --- a/src/util/virtypedparam.c +++ b/src/util/virtypedparam.c @@ -709,6 +709,8 @@ struct _virTypedParamList { virTypedParameterPtr par; size_t npar; size_t par_alloc; + + char *err_name; /* overly long field name for error message */ }; @@ -740,6 +742,9 @@ virTypedParamListConcat(virTypedParamList *to, memcpy(to->par + to->npar, from->par, sizeof(*(to->par)) * from->npar); to->npar += from->npar; from->npar = 0; + + if (!to->err_name) + to->err_name = g_steal_pointer(&from->err_name); } @@ -750,6 +755,7 @@ virTypedParamListFree(virTypedParamList *list) return; virTypedParamsFree(list->par, list->npar); + g_free(list->err_name); g_free(list); } @@ -770,6 +776,12 @@ virTypedParamListFetch(virTypedParamList *list, virTypedParameterPtr *par, size_t *npar) { + if (list->err_name) { + virReportError(VIR_ERR_INTERNAL_ERROR, _("Field name '%1$s' too long"), + list->err_name); + return -1; + } + if (par) *par = list->par; @@ -815,17 +827,18 @@ virTypedParamListFromParams(virTypedParameterPtr *params, } -static int G_GNUC_PRINTF(2, 0) -virTypedParamSetNameVPrintf(virTypedParameterPtr par, +static void G_GNUC_PRINTF(3, 0) +virTypedParamSetNameVPrintf(virTypedParamList *list, + virTypedParameterPtr par, const char *fmt, va_list ap) { - if (g_vsnprintf(par->field, VIR_TYPED_PARAM_FIELD_LENGTH, fmt, ap) > VIR_TYPED_PARAM_FIELD_LENGTH) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Field name too long")); - return -1; - } + g_autofree char *name = g_strdup_vprintf(fmt, ap); - return 0; + if (virStrcpyStatic(par->field, name) < 0) { + if (!list->err_name) + list->err_name = g_steal_pointer(&name); + } } @@ -848,15 +861,14 @@ virTypedParamListAddInt(virTypedParamList *list, { virTypedParameterPtr par = virTypedParamListExtend(list); va_list ap; - int ret; virTypedParameterAssignValue(par, VIR_TYPED_PARAM_INT, value); va_start(ap, namefmt); - ret = virTypedParamSetNameVPrintf(par, namefmt, ap); + virTypedParamSetNameVPrintf(list, par, namefmt, ap); va_end(ap); - return ret; + return 0; } @@ -868,15 +880,14 @@ virTypedParamListAddUInt(virTypedParamList *list, { virTypedParameterPtr par = virTypedParamListExtend(list); va_list ap; - int ret; virTypedParameterAssignValue(par, VIR_TYPED_PARAM_UINT, value); va_start(ap, namefmt); - ret = virTypedParamSetNameVPrintf(par, namefmt, ap); + virTypedParamSetNameVPrintf(list, par, namefmt, ap); va_end(ap); - return ret; + return 0; } @@ -888,15 +899,14 @@ virTypedParamListAddLLong(virTypedParamList *list, { virTypedParameterPtr par = virTypedParamListExtend(list); va_list ap; - int ret; virTypedParameterAssignValue(par, VIR_TYPED_PARAM_LLONG, value); va_start(ap, namefmt); - ret = virTypedParamSetNameVPrintf(par, namefmt, ap); + virTypedParamSetNameVPrintf(list, par, namefmt, ap); va_end(ap); - return ret; + return 0; } @@ -908,15 +918,14 @@ virTypedParamListAddULLong(virTypedParamList *list, { virTypedParameterPtr par = virTypedParamListExtend(list); va_list ap; - int ret; virTypedParameterAssignValue(par, VIR_TYPED_PARAM_ULLONG, value); va_start(ap, namefmt); - ret = virTypedParamSetNameVPrintf(par, namefmt, ap); + virTypedParamSetNameVPrintf(list, par, namefmt, ap); va_end(ap); - return ret; + return 0; } @@ -928,15 +937,14 @@ virTypedParamListAddString(virTypedParamList *list, { virTypedParameterPtr par = virTypedParamListExtend(list); va_list ap; - int ret; virTypedParameterAssignValue(par, VIR_TYPED_PARAM_STRING, value); va_start(ap, namefmt); - ret = virTypedParamSetNameVPrintf(par, namefmt, ap); + virTypedParamSetNameVPrintf(list, par, namefmt, ap); va_end(ap); - return ret; + return 0; } @@ -948,15 +956,14 @@ virTypedParamListAddBoolean(virTypedParamList *list, { virTypedParameterPtr par = virTypedParamListExtend(list); va_list ap; - int ret; virTypedParameterAssignValue(par, VIR_TYPED_PARAM_BOOLEAN, value); va_start(ap, namefmt); - ret = virTypedParamSetNameVPrintf(par, namefmt, ap); + virTypedParamSetNameVPrintf(list, par, namefmt, ap); va_end(ap); - return ret; + return 0; } @@ -968,13 +975,12 @@ virTypedParamListAddDouble(virTypedParamList *list, { virTypedParameterPtr par = virTypedParamListExtend(list); va_list ap; - int ret; virTypedParameterAssignValue(par, VIR_TYPED_PARAM_DOUBLE, value); va_start(ap, namefmt); - ret = virTypedParamSetNameVPrintf(par, namefmt, ap); + virTypedParamSetNameVPrintf(list, par, namefmt, ap); va_end(ap); - return ret; + return 0; } -- 2.39.2

The function now return always 0. Refactor the code and remove return values. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/admin/admin_server.c | 108 ++++--------- src/qemu/qemu_domainjob.c | 45 ++---- src/qemu/qemu_driver.c | 330 ++++++++++++-------------------------- src/test/test_driver.c | 28 ++-- src/util/virtypedparam.c | 28 +--- src/util/virtypedparam.h | 28 ++-- 6 files changed, 173 insertions(+), 394 deletions(-) diff --git a/src/admin/admin_server.c b/src/admin/admin_server.c index 1d1ae97f2f..2498a1309a 100644 --- a/src/admin/admin_server.c +++ b/src/admin/admin_server.c @@ -89,29 +89,12 @@ adminServerGetThreadPoolParameters(virNetServer *srv, return -1; } - if (virTypedParamListAddUInt(paramlist, minWorkers, - "%s", VIR_THREADPOOL_WORKERS_MIN) < 0) - return -1; - - if (virTypedParamListAddUInt(paramlist, maxWorkers, - "%s", VIR_THREADPOOL_WORKERS_MAX) < 0) - return -1; - - if (virTypedParamListAddUInt(paramlist, nWorkers, - "%s", VIR_THREADPOOL_WORKERS_CURRENT) < 0) - return -1; - - if (virTypedParamListAddUInt(paramlist, freeWorkers, - "%s", VIR_THREADPOOL_WORKERS_FREE) < 0) - return -1; - - if (virTypedParamListAddUInt(paramlist, nPrioWorkers, - "%s", VIR_THREADPOOL_WORKERS_PRIORITY) < 0) - return -1; - - if (virTypedParamListAddUInt(paramlist, jobQueueDepth, - "%s", VIR_THREADPOOL_JOB_QUEUE_DEPTH) < 0) - return -1; + virTypedParamListAddUInt(paramlist, minWorkers, VIR_THREADPOOL_WORKERS_MIN); + virTypedParamListAddUInt(paramlist, maxWorkers, VIR_THREADPOOL_WORKERS_MAX); + virTypedParamListAddUInt(paramlist, nWorkers, VIR_THREADPOOL_WORKERS_CURRENT); + virTypedParamListAddUInt(paramlist, freeWorkers, VIR_THREADPOOL_WORKERS_FREE); + virTypedParamListAddUInt(paramlist, nPrioWorkers, VIR_THREADPOOL_WORKERS_PRIORITY); + virTypedParamListAddUInt(paramlist, jobQueueDepth, VIR_THREADPOOL_JOB_QUEUE_DEPTH); if (virTypedParamListSteal(paramlist, params, nparams) < 0) return -1; @@ -211,74 +194,54 @@ adminClientGetInfo(virNetServerClient *client, &sock_addr, &identity) < 0) return -1; - if (virTypedParamListAddBoolean(paramlist, readonly, - "%s", VIR_CLIENT_INFO_READONLY) < 0) - return -1; + virTypedParamListAddBoolean(paramlist, readonly, VIR_CLIENT_INFO_READONLY); if ((rc = virIdentityGetSASLUserName(identity, &attr)) < 0) return -1; - if (rc == 1 && - virTypedParamListAddString(paramlist, attr, - "%s", VIR_CLIENT_INFO_SASL_USER_NAME) < 0) - return -1; + if (rc == 1) + virTypedParamListAddString(paramlist, attr, VIR_CLIENT_INFO_SASL_USER_NAME); if (!virNetServerClientIsLocal(client)) { - if (virTypedParamListAddString(paramlist, sock_addr, - "%s", VIR_CLIENT_INFO_SOCKET_ADDR) < 0) - return -1; + virTypedParamListAddString(paramlist, sock_addr, VIR_CLIENT_INFO_SOCKET_ADDR); if ((rc = virIdentityGetX509DName(identity, &attr)) < 0) return -1; - if (rc == 1 && - virTypedParamListAddString(paramlist, attr, - "%s", VIR_CLIENT_INFO_X509_DISTINGUISHED_NAME) < 0) - return -1; + if (rc == 1) + virTypedParamListAddString(paramlist, attr, VIR_CLIENT_INFO_X509_DISTINGUISHED_NAME); } else { pid_t pid; uid_t uid; gid_t gid; if ((rc = virIdentityGetUNIXUserID(identity, &uid)) < 0) return -1; - if (rc == 1 && - virTypedParamListAddInt(paramlist, uid, - "%s", VIR_CLIENT_INFO_UNIX_USER_ID) < 0) - return -1; + if (rc == 1) + virTypedParamListAddInt(paramlist, uid, VIR_CLIENT_INFO_UNIX_USER_ID); if ((rc = virIdentityGetUserName(identity, &attr)) < 0) return -1; - if (rc == 1 && - virTypedParamListAddString(paramlist, attr, - "%s", VIR_CLIENT_INFO_UNIX_USER_NAME) < 0) - return -1; + if (rc == 1) + virTypedParamListAddString(paramlist, attr, VIR_CLIENT_INFO_UNIX_USER_NAME); if ((rc = virIdentityGetUNIXGroupID(identity, &gid)) < 0) return -1; - if (rc == 1 && - virTypedParamListAddInt(paramlist, gid, - "%s", VIR_CLIENT_INFO_UNIX_GROUP_ID) < 0) - return -1; + if (rc == 1) + virTypedParamListAddInt(paramlist, gid, VIR_CLIENT_INFO_UNIX_GROUP_ID); if ((rc = virIdentityGetGroupName(identity, &attr)) < 0) return -1; - if (rc == 1 && - virTypedParamListAddString(paramlist, attr, - "%s", VIR_CLIENT_INFO_UNIX_GROUP_NAME) < 0) - return -1; + if (rc == 1) + virTypedParamListAddString(paramlist, attr, VIR_CLIENT_INFO_UNIX_GROUP_NAME); if ((rc = virIdentityGetProcessID(identity, &pid)) < 0) return -1; - if (rc == 1 && - virTypedParamListAddInt(paramlist, pid, - "%s", VIR_CLIENT_INFO_UNIX_PROCESS_ID) < 0) - return -1; + if (rc == 1) + virTypedParamListAddInt(paramlist, pid, VIR_CLIENT_INFO_UNIX_PROCESS_ID); } if ((rc = virIdentityGetSELinuxContext(identity, &attr)) < 0) return -1; - if (rc == 1 && - virTypedParamListAddString(paramlist, attr, - "%s", VIR_CLIENT_INFO_SELINUX_CONTEXT) < 0) - return -1; + if (rc == 1) + virTypedParamListAddString(paramlist, attr, VIR_CLIENT_INFO_SELINUX_CONTEXT); if (virTypedParamListSteal(paramlist, params, nparams) < 0) return -1; @@ -305,25 +268,10 @@ adminServerGetClientLimits(virNetServer *srv, virCheckFlags(0, -1); - if (virTypedParamListAddUInt(paramlist, - virNetServerGetMaxClients(srv), - "%s", VIR_SERVER_CLIENTS_MAX) < 0) - return -1; - - if (virTypedParamListAddUInt(paramlist, - virNetServerGetCurrentClients(srv), - "%s", VIR_SERVER_CLIENTS_CURRENT) < 0) - return -1; - - if (virTypedParamListAddUInt(paramlist, - virNetServerGetMaxUnauthClients(srv), - "%s", VIR_SERVER_CLIENTS_UNAUTH_MAX) < 0) - return -1; - - if (virTypedParamListAddUInt(paramlist, - virNetServerGetCurrentUnauthClients(srv), - "%s", VIR_SERVER_CLIENTS_UNAUTH_CURRENT) < 0) - return -1; + virTypedParamListAddUInt(paramlist, virNetServerGetMaxClients(srv), VIR_SERVER_CLIENTS_MAX); + virTypedParamListAddUInt(paramlist, virNetServerGetCurrentClients(srv), VIR_SERVER_CLIENTS_CURRENT); + virTypedParamListAddUInt(paramlist, virNetServerGetMaxUnauthClients(srv), VIR_SERVER_CLIENTS_UNAUTH_MAX); + virTypedParamListAddUInt(paramlist, virNetServerGetCurrentUnauthClients(srv), VIR_SERVER_CLIENTS_UNAUTH_CURRENT); if (virTypedParamListSteal(paramlist, params, nparams) < 0) return -1; diff --git a/src/qemu/qemu_domainjob.c b/src/qemu/qemu_domainjob.c index 6be48118b9..245e51f14b 100644 --- a/src/qemu/qemu_domainjob.c +++ b/src/qemu/qemu_domainjob.c @@ -480,47 +480,26 @@ qemuDomainBackupJobDataToParams(virDomainJobData *jobData, qemuDomainBackupStats *stats = &priv->stats.backup; g_autoptr(virTypedParamList) par = virTypedParamListNew(); - if (virTypedParamListAddInt(par, jobData->operation, - VIR_DOMAIN_JOB_OPERATION) < 0) - return -1; - - if (virTypedParamListAddULLong(par, jobData->timeElapsed, - VIR_DOMAIN_JOB_TIME_ELAPSED) < 0) - return -1; + virTypedParamListAddInt(par, jobData->operation, VIR_DOMAIN_JOB_OPERATION); + virTypedParamListAddULLong(par, jobData->timeElapsed, VIR_DOMAIN_JOB_TIME_ELAPSED); if (stats->transferred > 0 || stats->total > 0) { - if (virTypedParamListAddULLong(par, stats->total, - VIR_DOMAIN_JOB_DISK_TOTAL) < 0) - return -1; - - if (virTypedParamListAddULLong(par, stats->transferred, - VIR_DOMAIN_JOB_DISK_PROCESSED) < 0) - return -1; - - if (virTypedParamListAddULLong(par, stats->total - stats->transferred, - VIR_DOMAIN_JOB_DISK_REMAINING) < 0) - return -1; + virTypedParamListAddULLong(par, stats->total, VIR_DOMAIN_JOB_DISK_TOTAL); + virTypedParamListAddULLong(par, stats->transferred, VIR_DOMAIN_JOB_DISK_PROCESSED); + virTypedParamListAddULLong(par, stats->total - stats->transferred, VIR_DOMAIN_JOB_DISK_REMAINING); } if (stats->tmp_used > 0 || stats->tmp_total > 0) { - if (virTypedParamListAddULLong(par, stats->tmp_used, - VIR_DOMAIN_JOB_DISK_TEMP_USED) < 0) - return -1; - - if (virTypedParamListAddULLong(par, stats->tmp_total, - VIR_DOMAIN_JOB_DISK_TEMP_TOTAL) < 0) - return -1; + virTypedParamListAddULLong(par, stats->tmp_used, VIR_DOMAIN_JOB_DISK_TEMP_USED); + virTypedParamListAddULLong(par, stats->tmp_total, VIR_DOMAIN_JOB_DISK_TEMP_TOTAL); } - if (jobData->status != VIR_DOMAIN_JOB_STATUS_ACTIVE && - virTypedParamListAddBoolean(par, - jobData->status == VIR_DOMAIN_JOB_STATUS_COMPLETED, - VIR_DOMAIN_JOB_SUCCESS) < 0) - return -1; + if (jobData->status != VIR_DOMAIN_JOB_STATUS_ACTIVE) + virTypedParamListAddBoolean(par, jobData->status == VIR_DOMAIN_JOB_STATUS_COMPLETED, + VIR_DOMAIN_JOB_SUCCESS); - if (jobData->errmsg && - virTypedParamListAddString(par, jobData->errmsg, VIR_DOMAIN_JOB_ERRMSG) < 0) - return -1; + if (jobData->errmsg) + virTypedParamListAddString(par, jobData->errmsg, VIR_DOMAIN_JOB_ERRMSG); if (virTypedParamListSteal(par, params, nparams) < 0) return -1; diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 9c409948f0..d47c1883f0 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -17020,11 +17020,8 @@ qemuDomainGetStatsState(virQEMUDriver *driver G_GNUC_UNUSED, virTypedParamList *params, unsigned int privflags G_GNUC_UNUSED) { - if (virTypedParamListAddInt(params, dom->state.state, "state.state") < 0) - return -1; - - if (virTypedParamListAddInt(params, dom->state.reason, "state.reason") < 0) - return -1; + virTypedParamListAddInt(params, dom->state.state, "state.state"); + virTypedParamListAddInt(params, dom->state.reason, "state.reason"); return 0; } @@ -17183,33 +17180,16 @@ qemuDomainGetStatsMemoryBandwidth(virQEMUDriver *driver, if (nresdata == 0) return 0; - if (virTypedParamListAddUInt(params, nresdata, - "memory.bandwidth.monitor.count") < 0) - goto cleanup; + virTypedParamListAddUInt(params, nresdata, "memory.bandwidth.monitor.count"); for (i = 0; i < nresdata; i++) { - if (virTypedParamListAddString(params, resdata[i]->name, - "memory.bandwidth.monitor.%zu.name", - i) < 0) - goto cleanup; - - if (virTypedParamListAddString(params, resdata[i]->vcpus, - "memory.bandwidth.monitor.%zu.vcpus", - i) < 0) - goto cleanup; - - if (virTypedParamListAddUInt(params, resdata[i]->nstats, - "memory.bandwidth.monitor.%zu.node.count", - i) < 0) - goto cleanup; - + virTypedParamListAddString(params, resdata[i]->name, "memory.bandwidth.monitor.%zu.name", i); + virTypedParamListAddString(params, resdata[i]->vcpus, "memory.bandwidth.monitor.%zu.vcpus", i); + virTypedParamListAddUInt(params, resdata[i]->nstats, "memory.bandwidth.monitor.%zu.node.count", i); for (j = 0; j < resdata[i]->nstats; j++) { - if (virTypedParamListAddUInt(params, resdata[i]->stats[j]->id, - "memory.bandwidth.monitor.%zu." - "node.%zu.id", - i, j) < 0) - goto cleanup; + virTypedParamListAddUInt(params, resdata[i]->stats[j]->id, + "memory.bandwidth.monitor.%zu.node.%zu.id", i, j); features = resdata[i]->stats[j]->features; @@ -17217,23 +17197,15 @@ qemuDomainGetStatsMemoryBandwidth(virQEMUDriver *driver, if (STREQ(features[k], "mbm_local_bytes")) { /* The accumulative data passing through local memory * controller is recorded with 64 bit counter. */ - if (virTypedParamListAddULLong(params, - resdata[i]->stats[j]->vals[k], - "memory.bandwidth.monitor." - "%zu.node.%zu.bytes.local", - i, j) < 0) - goto cleanup; + virTypedParamListAddULLong(params, resdata[i]->stats[j]->vals[k], + "memory.bandwidth.monitor.%zu.node.%zu.bytes.local", i, j); } if (STREQ(features[k], "mbm_total_bytes")) { /* The accumulative data passing through local and remote * memory controller is recorded with 64 bit counter. */ - if (virTypedParamListAddULLong(params, - resdata[i]->stats[j]->vals[k], - "memory.bandwidth.monitor." - "%zu.node.%zu.bytes.total", - i, j) < 0) - goto cleanup; + virTypedParamListAddULLong(params, resdata[i]->stats[j]->vals[k], + "memory.bandwidth.monitor.%zu.node.%zu.bytes.total", i, j); } } } @@ -17266,26 +17238,16 @@ qemuDomainGetStatsCpuCache(virQEMUDriver *driver, VIR_RESCTRL_MONITOR_TYPE_CACHE) < 0) goto cleanup; - if (virTypedParamListAddUInt(params, nresdata, "cpu.cache.monitor.count") < 0) - goto cleanup; + virTypedParamListAddUInt(params, nresdata, "cpu.cache.monitor.count"); for (i = 0; i < nresdata; i++) { - if (virTypedParamListAddString(params, resdata[i]->name, - "cpu.cache.monitor.%zu.name", i) < 0) - goto cleanup; - - if (virTypedParamListAddString(params, resdata[i]->vcpus, - "cpu.cache.monitor.%zu.vcpus", i) < 0) - goto cleanup; - - if (virTypedParamListAddUInt(params, resdata[i]->nstats, - "cpu.cache.monitor.%zu.bank.count", i) < 0) - goto cleanup; + virTypedParamListAddString(params, resdata[i]->name, "cpu.cache.monitor.%zu.name", i); + virTypedParamListAddString(params, resdata[i]->vcpus, "cpu.cache.monitor.%zu.vcpus", i); + virTypedParamListAddUInt(params, resdata[i]->nstats, "cpu.cache.monitor.%zu.bank.count", i); for (j = 0; j < resdata[i]->nstats; j++) { - if (virTypedParamListAddUInt(params, resdata[i]->stats[j]->id, - "cpu.cache.monitor.%zu.bank.%zu.id", i, j) < 0) - goto cleanup; + virTypedParamListAddUInt(params, resdata[i]->stats[j]->id, + "cpu.cache.monitor.%zu.bank.%zu.id", i, j); /* 'resdata[i]->stats[j]->vals[0]' keeps the value of how many last * level cache in bank j currently occupied by the vcpus listed in @@ -17296,11 +17258,8 @@ qemuDomainGetStatsCpuCache(virQEMUDriver *driver, * than 4G bytes in size, to keep the 'domstats' interface * historically consistent, it is safe to report the value with a * truncated 'UInt' data type here. */ - if (virTypedParamListAddUInt(params, - (unsigned int)resdata[i]->stats[j]->vals[0], - "cpu.cache.monitor.%zu.bank.%zu.bytes", - i, j) < 0) - goto cleanup; + virTypedParamListAddUInt(params, (unsigned int)resdata[i]->stats[j]->vals[0], + "cpu.cache.monitor.%zu.bank.%zu.bytes", i, j); } } @@ -17321,20 +17280,17 @@ qemuDomainGetStatsCpuCgroup(virDomainObj *dom, unsigned long long cpu_time = 0; unsigned long long user_time = 0; unsigned long long sys_time = 0; - int err = 0; if (!priv->cgroup) return 0; - err = virCgroupGetCpuacctUsage(priv->cgroup, &cpu_time); - if (!err && virTypedParamListAddULLong(params, cpu_time, "cpu.time") < 0) - return -1; + if (virCgroupGetCpuacctUsage(priv->cgroup, &cpu_time) == 0) + virTypedParamListAddULLong(params, cpu_time, "cpu.time"); - err = virCgroupGetCpuacctStat(priv->cgroup, &user_time, &sys_time); - if (!err && virTypedParamListAddULLong(params, user_time, "cpu.user") < 0) - return -1; - if (!err && virTypedParamListAddULLong(params, sys_time, "cpu.system") < 0) - return -1; + if (virCgroupGetCpuacctStat(priv->cgroup, &user_time, &sys_time) == 0) { + virTypedParamListAddULLong(params, user_time, "cpu.user"); + virTypedParamListAddULLong(params, sys_time, "cpu.system"); + } return 0; } @@ -17354,10 +17310,9 @@ qemuDomainGetStatsCpuProc(virDomainObj *vm, return 0; } - if (virTypedParamListAddULLong(params, cpuTime, "cpu.time") < 0 || - virTypedParamListAddULLong(params, userTime, "cpu.user") < 0 || - virTypedParamListAddULLong(params, sysTime, "cpu.system") < 0) - return -1; + virTypedParamListAddULLong(params, cpuTime, "cpu.time"); + virTypedParamListAddULLong(params, userTime, "cpu.user"); + virTypedParamListAddULLong(params, sysTime, "cpu.system"); return 0; } @@ -17445,9 +17400,8 @@ qemuDomainGetStatsCpuHaltPollTime(virDomainObj *dom, virHostCPUGetHaltPollTime(dom->pid, &haltPollSuccess, &haltPollFail) < 0) return 0; - if (virTypedParamListAddULLong(params, haltPollSuccess, "cpu.haltpoll.success.time") < 0 || - virTypedParamListAddULLong(params, haltPollFail, "cpu.haltpoll.fail.time") < 0) - return -1; + virTypedParamListAddULLong(params, haltPollSuccess, "cpu.haltpoll.success.time"); + virTypedParamListAddULLong(params, haltPollFail, "cpu.haltpoll.fail.time"); return 0; } @@ -17506,12 +17460,8 @@ qemuDomainGetStatsBalloon(virQEMUDriver *driver G_GNUC_UNUSED, cur_balloon = dom->def->mem.cur_balloon; } - if (virTypedParamListAddULLong(params, cur_balloon, "balloon.current") < 0) - return -1; - - if (virTypedParamListAddULLong(params, virDomainDefGetMemoryTotal(dom->def), - "balloon.maximum") < 0) - return -1; + virTypedParamListAddULLong(params, cur_balloon, "balloon.current"); + virTypedParamListAddULLong(params, virDomainDefGetMemoryTotal(dom->def), "balloon.maximum"); if (!HAVE_JOB(privflags) || !virDomainObjIsActive(dom)) return 0; @@ -17523,8 +17473,7 @@ qemuDomainGetStatsBalloon(virQEMUDriver *driver G_GNUC_UNUSED, #define STORE_MEM_RECORD(TAG, NAME) \ if (stats[i].tag == VIR_DOMAIN_MEMORY_STAT_ ##TAG) \ - if (virTypedParamListAddULLong(params, stats[i].val, "balloon." NAME) < 0) \ - return -1; + virTypedParamListAddULLong(params, stats[i].val, "balloon." NAME); for (i = 0; i < nr_stats; i++) { STORE_MEM_RECORD(SWAP_IN, "swap_in") @@ -17593,16 +17542,14 @@ qemuDomainAddStatsFromHashTable(GHashTable *stats, if (virJSONValueGetBoolean(value, &stat) < 0) continue; - ignore_value(virTypedParamListAddBoolean(params, stat, "%s.%s.%s", - prefix, key, type)); + virTypedParamListAddBoolean(params, stat, "%s.%s.%s", prefix, key, type); } else { unsigned long long stat; if (virJSONValueGetNumberUlong(value, &stat) < 0) continue; - ignore_value(virTypedParamListAddULLong(params, stat, "%s.%s.%s", - prefix, key, type)); + virTypedParamListAddULLong(params, stat, "%s.%s.%s", prefix, key, type); } } } @@ -17623,13 +17570,8 @@ qemuDomainGetStatsVcpu(virQEMUDriver *driver G_GNUC_UNUSED, qemuDomainObjPrivate *priv = dom->privateData; g_autoptr(virJSONValue) queried_stats = NULL; - if (virTypedParamListAddUInt(params, virDomainDefGetVcpus(dom->def), - "vcpu.current") < 0) - return -1; - - if (virTypedParamListAddUInt(params, virDomainDefGetVcpusMax(dom->def), - "vcpu.maximum") < 0) - return -1; + virTypedParamListAddUInt(params, virDomainDefGetVcpus(dom->def), "vcpu.current"); + virTypedParamListAddUInt(params, virDomainDefGetVcpusMax(dom->def), "vcpu.maximum"); cpuinfo = g_new0(virVcpuInfo, virDomainDefGetVcpus(dom->def)); cpuwait = g_new0(unsigned long long, virDomainDefGetVcpus(dom->def)); @@ -17662,25 +17604,15 @@ qemuDomainGetStatsVcpu(virQEMUDriver *driver G_GNUC_UNUSED, g_autoptr(GHashTable) stats = NULL; g_autofree char *prefix = g_strdup_printf("vcpu.%u", cpuinfo[i].number); - if (virTypedParamListAddInt(params, cpuinfo[i].state, - "vcpu.%u.state", cpuinfo[i].number) < 0) - return -1; + virTypedParamListAddInt(params, cpuinfo[i].state, "vcpu.%u.state", cpuinfo[i].number); /* stats below are available only if the VM is alive */ if (!virDomainObjIsActive(dom)) continue; - if (virTypedParamListAddULLong(params, cpuinfo[i].cpuTime, - "vcpu.%u.time", cpuinfo[i].number) < 0) - return -1; - - if (virTypedParamListAddULLong(params, cpuwait[i], - "vcpu.%u.wait", cpuinfo[i].number) < 0) - return -1; - - if (virTypedParamListAddULLong(params, cpudelay[i], - "vcpu.%u.delay", cpuinfo[i].number) < 0) - return -1; + virTypedParamListAddULLong(params, cpuinfo[i].cpuTime, "vcpu.%u.time", cpuinfo[i].number); + virTypedParamListAddULLong(params, cpuwait[i], "vcpu.%u.wait", cpuinfo[i].number); + virTypedParamListAddULLong(params, cpudelay[i], "vcpu.%u.delay", cpuinfo[i].number); /* state below is extracted from the individual vcpu structs */ if (!(vcpu = virDomainDefGetVcpu(dom->def, cpuinfo[i].number))) @@ -17689,11 +17621,8 @@ qemuDomainGetStatsVcpu(virQEMUDriver *driver G_GNUC_UNUSED, vcpupriv = QEMU_DOMAIN_VCPU_PRIVATE(vcpu); if (vcpupriv->halted != VIR_TRISTATE_BOOL_ABSENT) { - if (virTypedParamListAddBoolean(params, - vcpupriv->halted == VIR_TRISTATE_BOOL_YES, - "vcpu.%u.halted", - cpuinfo[i].number) < 0) - return -1; + virTypedParamListAddBoolean(params, vcpupriv->halted == VIR_TRISTATE_BOOL_YES, + "vcpu.%u.halted", cpuinfo[i].number); } if (!queried_stats) @@ -17709,9 +17638,8 @@ qemuDomainGetStatsVcpu(virQEMUDriver *driver G_GNUC_UNUSED, } #define QEMU_ADD_NET_PARAM(params, num, name, value) \ - if (value >= 0 && \ - virTypedParamListAddULLong((params), (value), "net.%zu.%s", (num), (name)) < 0) \ - return -1; + if (value >= 0)\ + virTypedParamListAddULLong((params), (value), "net.%zu.%s", (num), (name)); static int qemuDomainGetStatsInterface(virQEMUDriver *driver G_GNUC_UNUSED, @@ -17725,8 +17653,7 @@ qemuDomainGetStatsInterface(virQEMUDriver *driver G_GNUC_UNUSED, if (!virDomainObjIsActive(dom)) return 0; - if (virTypedParamListAddUInt(params, dom->def->nnets, "net.count") < 0) - return -1; + virTypedParamListAddUInt(params, dom->def->nnets, "net.count"); /* Check the path is one of the domain's network interfaces. */ for (i = 0; i < dom->def->nnets; i++) { @@ -17740,8 +17667,7 @@ qemuDomainGetStatsInterface(virQEMUDriver *driver G_GNUC_UNUSED, actualType = virDomainNetGetActualType(net); - if (virTypedParamListAddString(params, net->ifname, "net.%zu.name", i) < 0) - return -1; + virTypedParamListAddString(params, net->ifname, "net.%zu.name", i); if (actualType == VIR_DOMAIN_NET_TYPE_VHOSTUSER) { if (virNetDevOpenvswitchInterfaceStats(net->ifname, &tmp) < 0) { @@ -17756,22 +17682,14 @@ qemuDomainGetStatsInterface(virQEMUDriver *driver G_GNUC_UNUSED, } } - QEMU_ADD_NET_PARAM(params, i, - "rx.bytes", tmp.rx_bytes); - QEMU_ADD_NET_PARAM(params, i, - "rx.pkts", tmp.rx_packets); - QEMU_ADD_NET_PARAM(params, i, - "rx.errs", tmp.rx_errs); - QEMU_ADD_NET_PARAM(params, i, - "rx.drop", tmp.rx_drop); - QEMU_ADD_NET_PARAM(params, i, - "tx.bytes", tmp.tx_bytes); - QEMU_ADD_NET_PARAM(params, i, - "tx.pkts", tmp.tx_packets); - QEMU_ADD_NET_PARAM(params, i, - "tx.errs", tmp.tx_errs); - QEMU_ADD_NET_PARAM(params, i, - "tx.drop", tmp.tx_drop); + QEMU_ADD_NET_PARAM(params, i, "rx.bytes", tmp.rx_bytes); + QEMU_ADD_NET_PARAM(params, i, "rx.pkts", tmp.rx_packets); + QEMU_ADD_NET_PARAM(params, i, "rx.errs", tmp.rx_errs); + QEMU_ADD_NET_PARAM(params, i, "rx.drop", tmp.rx_drop); + QEMU_ADD_NET_PARAM(params, i, "tx.bytes", tmp.tx_bytes); + QEMU_ADD_NET_PARAM(params, i, "tx.pkts", tmp.tx_packets); + QEMU_ADD_NET_PARAM(params, i, "tx.errs", tmp.tx_errs); + QEMU_ADD_NET_PARAM(params, i, "tx.drop", tmp.tx_drop); } return 0; @@ -17799,20 +17717,14 @@ qemuDomainGetStatsOneBlockFallback(virQEMUDriver *driver, return 0; } - if (src->allocation && - virTypedParamListAddULLong(params, src->allocation, - "block.%zu.allocation", block_idx) < 0) - return -1; + if (src->allocation) + virTypedParamListAddULLong(params, src->allocation, "block.%zu.allocation", block_idx); - if (src->capacity && - virTypedParamListAddULLong(params, src->capacity, - "block.%zu.capacity", block_idx) < 0) - return -1; + if (src->capacity) + virTypedParamListAddULLong(params, src->capacity, "block.%zu.capacity", block_idx); - if (src->physical && - virTypedParamListAddULLong(params, src->physical, - "block.%zu.physical", block_idx) < 0) - return -1; + if (src->physical) + virTypedParamListAddULLong(params, src->physical, "block.%zu.physical", block_idx); return 0; } @@ -17843,24 +17755,16 @@ qemuDomainGetStatsOneBlock(virQEMUDriver *driver, if (!stats || !entryname || !(entry = virHashLookup(stats, entryname))) return 0; - if (virTypedParamListAddULLong(params, entry->wr_highest_offset, - "block.%zu.allocation", block_idx) < 0) - return -1; + virTypedParamListAddULLong(params, entry->wr_highest_offset, "block.%zu.allocation", block_idx); - if (entry->capacity && - virTypedParamListAddULLong(params, entry->capacity, - "block.%zu.capacity", block_idx) < 0) - return -1; + if (entry->capacity) + virTypedParamListAddULLong(params, entry->capacity, "block.%zu.capacity", block_idx); if (entry->physical) { - if (virTypedParamListAddULLong(params, entry->physical, - "block.%zu.physical", block_idx) < 0) - return -1; + virTypedParamListAddULLong(params, entry->physical, "block.%zu.physical", block_idx); } else { if (qemuDomainStorageUpdatePhysical(driver, cfg, dom, src) == 0) { - if (virTypedParamListAddULLong(params, src->physical, - "block.%zu.physical", block_idx) < 0) - return -1; + virTypedParamListAddULLong(params, src->physical, "block.%zu.physical", block_idx); } } @@ -17879,10 +17783,8 @@ qemuDomainGetStatsBlockExportBackendStorage(const char *entryname, if (!stats || !entryname || !(entry = virHashLookup(stats, entryname))) return 0; - if (entry->write_threshold && - virTypedParamListAddULLong(params, entry->write_threshold, - "block.%zu.threshold", recordnr) < 0) - return -1; + if (entry->write_threshold) + virTypedParamListAddULLong(params, entry->write_threshold, "block.%zu.threshold", recordnr); return 0; } @@ -17902,15 +17804,14 @@ qemuDomainGetStatsBlockExportFrontend(const char *frontendname, if (!stats || !frontendname || !(en = virHashLookup(stats, frontendname))) return 0; - if (virTypedParamListAddULLong(par, en->rd_req, "block.%zu.rd.reqs", idx) < 0 || - virTypedParamListAddULLong(par, en->rd_bytes, "block.%zu.rd.bytes", idx) < 0 || - virTypedParamListAddULLong(par, en->rd_total_times, "block.%zu.rd.times", idx) < 0 || - virTypedParamListAddULLong(par, en->wr_req, "block.%zu.wr.reqs", idx) < 0 || - virTypedParamListAddULLong(par, en->wr_bytes, "block.%zu.wr.bytes", idx) < 0 || - virTypedParamListAddULLong(par, en->wr_total_times, "block.%zu.wr.times", idx) < 0 || - virTypedParamListAddULLong(par, en->flush_req, "block.%zu.fl.reqs", idx) < 0 || - virTypedParamListAddULLong(par, en->flush_total_times, "block.%zu.fl.times", idx) < 0) - return -1; + virTypedParamListAddULLong(par, en->rd_req, "block.%zu.rd.reqs", idx); + virTypedParamListAddULLong(par, en->rd_bytes, "block.%zu.rd.bytes", idx); + virTypedParamListAddULLong(par, en->rd_total_times, "block.%zu.rd.times", idx); + virTypedParamListAddULLong(par, en->wr_req, "block.%zu.wr.reqs", idx); + virTypedParamListAddULLong(par, en->wr_bytes, "block.%zu.wr.bytes", idx); + virTypedParamListAddULLong(par, en->wr_total_times, "block.%zu.wr.times", idx); + virTypedParamListAddULLong(par, en->flush_req, "block.%zu.fl.reqs", idx); + virTypedParamListAddULLong(par, en->flush_total_times, "block.%zu.fl.times", idx); return 0; } @@ -17922,16 +17823,13 @@ qemuDomainGetStatsBlockExportHeader(virDomainDiskDef *disk, size_t recordnr, virTypedParamList *params) { - if (virTypedParamListAddString(params, disk->dst, "block.%zu.name", recordnr) < 0) - return -1; + virTypedParamListAddString(params, disk->dst, "block.%zu.name", recordnr); - if (virStorageSourceIsLocalStorage(src) && src->path && - virTypedParamListAddString(params, src->path, "block.%zu.path", recordnr) < 0) - return -1; + if (virStorageSourceIsLocalStorage(src) && src->path) + virTypedParamListAddString(params, src->path, "block.%zu.path", recordnr); - if (src->id && - virTypedParamListAddUInt(params, src->id, "block.%zu.backingIndex", recordnr) < 0) - return -1; + if (src->id) + virTypedParamListAddUInt(params, src->id, "block.%zu.backingIndex", recordnr); return 0; } @@ -18133,9 +18031,7 @@ qemuDomainGetStatsBlock(virQEMUDriver *driver, return -1; } - if (virTypedParamListAddUInt(params, 0, "block.count") < 0) - return -1; - + virTypedParamListAddUInt(params, 0, "block.count"); virTypedParamListConcat(params, &blockparams); return 0; @@ -18166,23 +18062,19 @@ qemuDomainGetStatsIOThread(virQEMUDriver *driver G_GNUC_UNUSED, goto cleanup; } - if (virTypedParamListAddUInt(params, niothreads, "iothread.count") < 0) - goto cleanup; + virTypedParamListAddUInt(params, niothreads, "iothread.count"); for (i = 0; i < niothreads; i++) { if (iothreads[i]->poll_valid) { - if (virTypedParamListAddULLong(params, iothreads[i]->poll_max_ns, - "iothread.%u.poll-max-ns", - iothreads[i]->iothread_id) < 0) - goto cleanup; - if (virTypedParamListAddUInt(params, iothreads[i]->poll_grow, - "iothread.%u.poll-grow", - iothreads[i]->iothread_id) < 0) - goto cleanup; - if (virTypedParamListAddUInt(params, iothreads[i]->poll_shrink, - "iothread.%u.poll-shrink", - iothreads[i]->iothread_id) < 0) - goto cleanup; + virTypedParamListAddULLong(params, iothreads[i]->poll_max_ns, + "iothread.%u.poll-max-ns", + iothreads[i]->iothread_id); + virTypedParamListAddUInt(params, iothreads[i]->poll_grow, + "iothread.%u.poll-grow", + iothreads[i]->iothread_id); + virTypedParamListAddUInt(params, iothreads[i]->poll_shrink, + "iothread.%u.poll-shrink", + iothreads[i]->iothread_id); } } @@ -18207,9 +18099,7 @@ qemuDomainGetStatsPerfOneEvent(virPerf *perf, if (virPerfReadEvent(perf, type, &value) < 0) return -1; - if (virTypedParamListAddULLong(params, value, "perf.%s", - virPerfEventTypeToString(type)) < 0) - return -1; + virTypedParamListAddULLong(params, value, "perf.%s", virPerfEventTypeToString(type)); return 0; } @@ -18262,35 +18152,21 @@ qemuDomainGetStatsDirtyRate(virQEMUDriver *driver G_GNUC_UNUSED, if (qemuDomainGetStatsDirtyRateMon(dom, &info) < 0) return -1; - if (virTypedParamListAddInt(params, info.status, - "dirtyrate.calc_status") < 0) - return -1; - - if (virTypedParamListAddLLong(params, info.startTime, - "dirtyrate.calc_start_time") < 0) - return -1; - - if (virTypedParamListAddInt(params, info.calcTime, - "dirtyrate.calc_period") < 0) - return -1; - - if (virTypedParamListAddString(params, - qemuMonitorDirtyRateCalcModeTypeToString(info.mode), - "dirtyrate.calc_mode") < 0) - return -1; + virTypedParamListAddInt(params, info.status, "dirtyrate.calc_status"); + virTypedParamListAddLLong(params, info.startTime, "dirtyrate.calc_start_time"); + virTypedParamListAddInt(params, info.calcTime, "dirtyrate.calc_period"); + virTypedParamListAddString(params, qemuMonitorDirtyRateCalcModeTypeToString(info.mode), + "dirtyrate.calc_mode"); if (info.status == VIR_DOMAIN_DIRTYRATE_MEASURED) { - if (virTypedParamListAddLLong(params, info.dirtyRate, - "dirtyrate.megabytes_per_second") < 0) - return -1; + virTypedParamListAddLLong(params, info.dirtyRate, "dirtyrate.megabytes_per_second"); if (info.mode == QEMU_MONITOR_DIRTYRATE_CALC_MODE_DIRTY_RING) { size_t i; for (i = 0; i < info.nvcpus; i++) { - if (virTypedParamListAddULLong(params, info.rates[i].value, - "dirtyrate.vcpu.%d.megabytes_per_second", - info.rates[i].idx) < 0) - return -1; + virTypedParamListAddULLong(params, info.rates[i].value, + "dirtyrate.vcpu.%d.megabytes_per_second", + info.rates[i].idx); } } } diff --git a/src/test/test_driver.c b/src/test/test_driver.c index ac12488de9..e7fce053b4 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -9784,11 +9784,8 @@ static int testDomainGetStatsState(virDomainObj *dom, virTypedParamList *params) { - if (virTypedParamListAddInt(params, dom->state.state, "state.state") < 0) - return -1; - - if (virTypedParamListAddInt(params, dom->state.reason, "state.reason") < 0) - return -1; + virTypedParamListAddInt(params, dom->state.state, "state.state"); + virTypedParamListAddInt(params, dom->state.reason, "state.reason"); return 0; } @@ -9810,24 +9807,17 @@ testDomainGetStatsIOThread(virDomainObj *dom, return 0; } - if (virTypedParamListAddUInt(params, niothreads, "iothread.count") < 0) - return -1; + virTypedParamListAddUInt(params, niothreads, "iothread.count"); for (i = 0; i < niothreads; i++) { testIOThreadInfo iothread = g_array_index(priv->iothreads, testIOThreadInfo, i); - if (virTypedParamListAddULLong(params, iothread.poll_max_ns, - "iothread.%u.poll-max-ns", - iothread.iothread_id) < 0) - return -1; - if (virTypedParamListAddUInt(params, iothread.poll_grow, - "iothread.%u.poll-grow", - iothread.iothread_id) < 0) - return -1; - if (virTypedParamListAddUInt(params, iothread.poll_shrink, - "iothread.%u.poll-shrink", - iothread.iothread_id) < 0) - return -1; + virTypedParamListAddULLong(params, iothread.poll_max_ns, + "iothread.%u.poll-max-ns", iothread.iothread_id); + virTypedParamListAddUInt(params, iothread.poll_grow, + "iothread.%u.poll-grow", iothread.iothread_id); + virTypedParamListAddUInt(params, iothread.poll_shrink, + "iothread.%u.poll-shrink", iothread.iothread_id); } return 0; diff --git a/src/util/virtypedparam.c b/src/util/virtypedparam.c index 046164d36c..ee9c6d1c45 100644 --- a/src/util/virtypedparam.c +++ b/src/util/virtypedparam.c @@ -853,7 +853,7 @@ virTypedParamListExtend(virTypedParamList *list) } -int +void virTypedParamListAddInt(virTypedParamList *list, int value, const char *namefmt, @@ -867,12 +867,10 @@ virTypedParamListAddInt(virTypedParamList *list, va_start(ap, namefmt); virTypedParamSetNameVPrintf(list, par, namefmt, ap); va_end(ap); - - return 0; } -int +void virTypedParamListAddUInt(virTypedParamList *list, unsigned int value, const char *namefmt, @@ -886,12 +884,10 @@ virTypedParamListAddUInt(virTypedParamList *list, va_start(ap, namefmt); virTypedParamSetNameVPrintf(list, par, namefmt, ap); va_end(ap); - - return 0; } -int +void virTypedParamListAddLLong(virTypedParamList *list, long long value, const char *namefmt, @@ -905,12 +901,10 @@ virTypedParamListAddLLong(virTypedParamList *list, va_start(ap, namefmt); virTypedParamSetNameVPrintf(list, par, namefmt, ap); va_end(ap); - - return 0; } -int +void virTypedParamListAddULLong(virTypedParamList *list, unsigned long long value, const char *namefmt, @@ -924,12 +918,10 @@ virTypedParamListAddULLong(virTypedParamList *list, va_start(ap, namefmt); virTypedParamSetNameVPrintf(list, par, namefmt, ap); va_end(ap); - - return 0; } -int +void virTypedParamListAddString(virTypedParamList *list, const char *value, const char *namefmt, @@ -943,12 +935,10 @@ virTypedParamListAddString(virTypedParamList *list, va_start(ap, namefmt); virTypedParamSetNameVPrintf(list, par, namefmt, ap); va_end(ap); - - return 0; } -int +void virTypedParamListAddBoolean(virTypedParamList *list, bool value, const char *namefmt, @@ -962,12 +952,10 @@ virTypedParamListAddBoolean(virTypedParamList *list, va_start(ap, namefmt); virTypedParamSetNameVPrintf(list, par, namefmt, ap); va_end(ap); - - return 0; } -int +void virTypedParamListAddDouble(virTypedParamList *list, double value, const char *namefmt, @@ -981,6 +969,4 @@ virTypedParamListAddDouble(virTypedParamList *list, va_start(ap, namefmt); virTypedParamSetNameVPrintf(list, par, namefmt, ap); va_end(ap); - - return 0; } diff --git a/src/util/virtypedparam.h b/src/util/virtypedparam.h index b2869173e1..7065683297 100644 --- a/src/util/virtypedparam.h +++ b/src/util/virtypedparam.h @@ -162,45 +162,45 @@ void virTypedParamListConcat(virTypedParamList *to, virTypedParamList **fromptr); -int +void virTypedParamListAddInt(virTypedParamList *list, int value, const char *namefmt, ...) - G_GNUC_PRINTF(3, 4) G_GNUC_WARN_UNUSED_RESULT; -int + G_GNUC_PRINTF(3, 4); +void virTypedParamListAddUInt(virTypedParamList *list, unsigned int value, const char *namefmt, ...) - G_GNUC_PRINTF(3, 4) G_GNUC_WARN_UNUSED_RESULT; -int + G_GNUC_PRINTF(3, 4); +void virTypedParamListAddLLong(virTypedParamList *list, long long value, const char *namefmt, ...) - G_GNUC_PRINTF(3, 4) G_GNUC_WARN_UNUSED_RESULT; -int + G_GNUC_PRINTF(3, 4); +void virTypedParamListAddULLong(virTypedParamList *list, unsigned long long value, const char *namefmt, ...) - G_GNUC_PRINTF(3, 4) G_GNUC_WARN_UNUSED_RESULT; -int + G_GNUC_PRINTF(3, 4); +void virTypedParamListAddString(virTypedParamList *list, const char *value, const char *namefmt, ...) - G_GNUC_PRINTF(3, 4) G_GNUC_WARN_UNUSED_RESULT; -int + G_GNUC_PRINTF(3, 4); +void virTypedParamListAddBoolean(virTypedParamList *list, bool value, const char *namefmt, ...) - G_GNUC_PRINTF(3, 4) G_GNUC_WARN_UNUSED_RESULT; -int + G_GNUC_PRINTF(3, 4); +void virTypedParamListAddDouble(virTypedParamList *list, double value, const char *namefmt, ...) - G_GNUC_PRINTF(3, 4) G_GNUC_WARN_UNUSED_RESULT; + G_GNUC_PRINTF(3, 4); -- 2.39.2

The new helper adds a unsigned value, stored as _UINT if it fits into the type and stored as _ULLONG otherwise. This is useful for the statistics code which is quite tolerant to changes in type in cases when we'll need more range for the value. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/libvirt_private.syms | 1 + src/util/virtypedparam.c | 36 ++++++++++++++++++++++++++++++++++++ src/util/virtypedparam.h | 6 ++++++ 3 files changed, 43 insertions(+) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index b58be6aa33..4e21851c53 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -3555,6 +3555,7 @@ virTypedParamListAddLLong; virTypedParamListAddString; virTypedParamListAddUInt; virTypedParamListAddULLong; +virTypedParamListAddUnsigned; virTypedParamListConcat; virTypedParamListFetch; virTypedParamListFree; diff --git a/src/util/virtypedparam.c b/src/util/virtypedparam.c index ee9c6d1c45..52eea0fc54 100644 --- a/src/util/virtypedparam.c +++ b/src/util/virtypedparam.c @@ -921,6 +921,42 @@ virTypedParamListAddULLong(virTypedParamList *list, } +/** + * virTypedParamListAddUnsigned: + * @list: typed parameter list + * @value: value to add (see below on details) + * @namefmt: formatting string for constructing the name of the added value + * @...: additional parameters to format the name + * + * Adds a new typed parameter to @list. The name of the parameter is formatted + * from @fmt. + * + * @value is added as VIR_TYPED_PARAM_UINT, unless it doesn't fit into the data + * type in which case it's added as VIR_TYPED_PARAM_ULLONG. + */ +void +virTypedParamListAddUnsigned(virTypedParamList *list, + unsigned long long value, + const char *namefmt, + ...) +{ + virTypedParameterPtr par = virTypedParamListExtend(list); + va_list ap; + + if (value > UINT_MAX) { + virTypedParameterAssignValue(par, VIR_TYPED_PARAM_ULLONG, value); + } else { + unsigned int ival = value; + + virTypedParameterAssignValue(par, VIR_TYPED_PARAM_UINT, ival); + } + + va_start(ap, namefmt); + virTypedParamSetNameVPrintf(list, par, namefmt, ap); + va_end(ap); +} + + void virTypedParamListAddString(virTypedParamList *list, const char *value, diff --git a/src/util/virtypedparam.h b/src/util/virtypedparam.h index 7065683297..88f810bf78 100644 --- a/src/util/virtypedparam.h +++ b/src/util/virtypedparam.h @@ -187,6 +187,12 @@ virTypedParamListAddULLong(virTypedParamList *list, ...) G_GNUC_PRINTF(3, 4); void +virTypedParamListAddUnsigned(virTypedParamList *list, + unsigned long long value, + const char *namefmt, + ...) + G_GNUC_PRINTF(3, 4); +void virTypedParamListAddString(virTypedParamList *list, const char *value, const char *namefmt, -- 2.39.2

Add an internal helper for fetching a typed parameter which can be either of the '_UINT' or '_ULONG' type and store it in a unsigned long long variable. Since this is an internal helper it offers less protections against invalid use compared to those we expose as public API. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/libvirt_private.syms | 1 + src/util/virtypedparam.c | 51 ++++++++++++++++++++++++++++++++++++++++ src/util/virtypedparam.h | 5 ++++ 3 files changed, 57 insertions(+) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 4e21851c53..62c296bf5f 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -3567,6 +3567,7 @@ virTypedParamsCopy; virTypedParamsDeserialize; virTypedParamsFilter; virTypedParamsGetStringList; +virTypedParamsGetUnsigned; virTypedParamsRemoteFree; virTypedParamsReplaceString; virTypedParamsSerialize; diff --git a/src/util/virtypedparam.c b/src/util/virtypedparam.c index 52eea0fc54..d0e6cf6d9b 100644 --- a/src/util/virtypedparam.c +++ b/src/util/virtypedparam.c @@ -456,6 +456,57 @@ virTypedParamsGetStringList(virTypedParameterPtr params, } +/** + * virTypedParamsGetUnsigned: + * @params: array of typed parameters + * @nparams: number of parameters in the @params array + * @name: name of the parameter to find + * @value: where to store the parameter's value + * + * Finds typed parameter called @name and store its 'unsigned long long' or + * 'unsigned int' value in @value. + * + * This is an internal variand which expects that the typed parameters were + * already validated by calling virTypedParamsValidate and the appropriate + * parameter has the expected type. + * + * Returns 1 on success, 0 when the parameter does not exist in @params, or + * -1 on invalid usage. + */ +int +virTypedParamsGetUnsigned(virTypedParameterPtr params, + int nparams, + const char *name, + unsigned long long *value) +{ + virTypedParameterPtr param; + + if (!(param = virTypedParamsGet(params, nparams, name))) + return 0; + + switch ((virTypedParameterType) param->type) { + case VIR_TYPED_PARAM_UINT: + *value = param->value.ui; + break; + + case VIR_TYPED_PARAM_ULLONG: + *value = param->value.ul; + break; + + case VIR_TYPED_PARAM_INT: + case VIR_TYPED_PARAM_LLONG: + case VIR_TYPED_PARAM_DOUBLE: + case VIR_TYPED_PARAM_BOOLEAN: + case VIR_TYPED_PARAM_STRING: + case VIR_TYPED_PARAM_LAST: + default: + return -1; + } + + return 1; +} + + /** * virTypedParamsRemoteFree: * @remote_params_val: array of typed parameters as specified by diff --git a/src/util/virtypedparam.h b/src/util/virtypedparam.h index 88f810bf78..c2f58e994c 100644 --- a/src/util/virtypedparam.h +++ b/src/util/virtypedparam.h @@ -79,6 +79,11 @@ virTypedParamsFilter(virTypedParameterPtr params, virTypedParameterPtr **ret) G_GNUC_WARN_UNUSED_RESULT; +int +virTypedParamsGetUnsigned(virTypedParameterPtr params, + int nparams, + const char *name, + unsigned long long *value); int virTypedParameterAssign(virTypedParameterPtr param, -- 2.39.2

Use automatic memory cleanup for the 'keys' and 'sorted' helpers and remove the 'cleanup' label. Since this patch is modifying variable declarations ensure that all declarations conform with our coding style. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/util/virtypedparam.c | 31 ++++++++++++++++--------------- 1 file changed, 16 insertions(+), 15 deletions(-) diff --git a/src/util/virtypedparam.c b/src/util/virtypedparam.c index d0e6cf6d9b..c71d4415a1 100644 --- a/src/util/virtypedparam.c +++ b/src/util/virtypedparam.c @@ -59,12 +59,15 @@ int virTypedParamsValidate(virTypedParameterPtr params, int nparams, ...) { va_list ap; - int ret = -1; - size_t i, j; - const char *name, *last_name = NULL; + size_t i; + size_t j; + const char *name; + const char *last_name = NULL; int type; - size_t nkeys = 0, nkeysalloc = 0; - virTypedParameterPtr sorted = NULL, keys = NULL; + size_t nkeys = 0; + size_t nkeysalloc = 0; + g_autofree virTypedParameterPtr sorted = NULL; + g_autofree virTypedParameterPtr keys = NULL; va_start(ap, nparams); @@ -82,7 +85,8 @@ virTypedParamsValidate(virTypedParameterPtr params, int nparams, ...) if (virStrcpyStatic(keys[nkeys].field, name) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, _("Field name '%1$s' too long"), name); - goto cleanup; + va_end(ap); + return -1; } keys[nkeys].type = type & ~VIR_TYPED_PARAM_MULTIPLE; @@ -93,6 +97,8 @@ virTypedParamsValidate(virTypedParameterPtr params, int nparams, ...) name = va_arg(ap, const char *); } + va_end(ap); + qsort(keys, nkeys, sizeof(*keys), virTypedParamsSortName); for (i = 0, j = 0; i < nparams && j < nkeys;) { @@ -104,7 +110,7 @@ virTypedParamsValidate(virTypedParameterPtr params, int nparams, ...) virReportError(VIR_ERR_INVALID_ARG, _("parameter '%1$s' occurs multiple times"), sorted[i].field); - goto cleanup; + return -1; } if (sorted[i].type != keys[j].type) { const char *badtype; @@ -116,7 +122,7 @@ virTypedParamsValidate(virTypedParameterPtr params, int nparams, ...) _("invalid type '%1$s' for parameter '%2$s', expected '%3$s'"), badtype, sorted[i].field, virTypedParameterTypeToString(keys[j].type)); - goto cleanup; + return -1; } last_name = sorted[i].field; i++; @@ -127,15 +133,10 @@ virTypedParamsValidate(virTypedParameterPtr params, int nparams, ...) virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, _("parameter '%1$s' not supported"), sorted[i].field); - goto cleanup; + return -1; } - ret = 0; - cleanup: - va_end(ap); - VIR_FREE(sorted); - VIR_FREE(keys); - return ret; + return 0; } -- 2.39.2

For certain typed parameters we want to extend the supproted range by switching to VIR_TYPED_PARAM_ULLONG. To preserve compatibility we've added APIs such as 'virTypedParamsGetUnsigned' and 'virTypedParamListAddUnsigned' which automatically select the bigger type if necessary. This patch adds a new internal macro VIR_TYPED_PARAM_UNSIGNED which is used with virTypedParamsValidate to allow both types and adjusts the code to handle it properly. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/util/virtypedparam.c | 19 ++++++++++++++----- src/util/virtypedparam.h | 9 +++++++++ 2 files changed, 23 insertions(+), 5 deletions(-) diff --git a/src/util/virtypedparam.c b/src/util/virtypedparam.c index c71d4415a1..c3bc1237c1 100644 --- a/src/util/virtypedparam.c +++ b/src/util/virtypedparam.c @@ -63,7 +63,6 @@ virTypedParamsValidate(virTypedParameterPtr params, int nparams, ...) size_t j; const char *name; const char *last_name = NULL; - int type; size_t nkeys = 0; size_t nkeysalloc = 0; g_autofree virTypedParameterPtr sorted = NULL; @@ -79,7 +78,7 @@ virTypedParamsValidate(virTypedParameterPtr params, int nparams, ...) name = va_arg(ap, const char *); while (name) { - type = va_arg(ap, int); + int type = va_arg(ap, int); VIR_RESIZE_N(keys, nkeysalloc, nkeys, 1); if (virStrcpyStatic(keys[nkeys].field, name) < 0) { @@ -105,6 +104,9 @@ virTypedParamsValidate(virTypedParameterPtr params, int nparams, ...) if (STRNEQ(sorted[i].field, keys[j].field)) { j++; } else { + const char *expecttype = virTypedParameterTypeToString(keys[j].type); + int type = sorted[i].type; + if (STREQ_NULLABLE(last_name, sorted[i].field) && !(keys[j].value.i & VIR_TYPED_PARAM_MULTIPLE)) { virReportError(VIR_ERR_INVALID_ARG, @@ -112,7 +114,15 @@ virTypedParamsValidate(virTypedParameterPtr params, int nparams, ...) sorted[i].field); return -1; } - if (sorted[i].type != keys[j].type) { + + if (keys[j].type == VIR_TYPED_PARAM_UNSIGNED && + (type == VIR_TYPED_PARAM_UINT || + type == VIR_TYPED_PARAM_ULLONG)) { + type = VIR_TYPED_PARAM_UNSIGNED; + expecttype = "uint, ullong"; + } + + if (type != keys[j].type) { const char *badtype; badtype = virTypedParameterTypeToString(sorted[i].type); @@ -120,8 +130,7 @@ virTypedParamsValidate(virTypedParameterPtr params, int nparams, ...) badtype = virTypedParameterTypeToString(0); virReportError(VIR_ERR_INVALID_ARG, _("invalid type '%1$s' for parameter '%2$s', expected '%3$s'"), - badtype, sorted[i].field, - virTypedParameterTypeToString(keys[j].type)); + badtype, sorted[i].field, expecttype); return -1; } last_name = sorted[i].field; diff --git a/src/util/virtypedparam.h b/src/util/virtypedparam.h index c2f58e994c..7454ef3ce0 100644 --- a/src/util/virtypedparam.h +++ b/src/util/virtypedparam.h @@ -24,6 +24,15 @@ #include "internal.h" #include "virenum.h" + +/** + * VIR_TYPED_PARAM_UNSIGNED: + * + * Special typed parameter type only used with virTypedParamsValidate to + * indicate that both VIR_TYPED_PARAM_UINT and VIR_TYPED_PARAM_ULLONG types + * are acceptable for given value. + */ +#define VIR_TYPED_PARAM_UNSIGNED (VIR_TYPED_PARAM_LAST + 1) /** * VIR_TYPED_PARAM_MULTIPLE: * -- 2.39.2

Refactor to use the new data type so that we can use the APIs of it in upcoming patches. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- tools/virsh-domain.c | 78 ++++++++++++++++++-------------------------- 1 file changed, 31 insertions(+), 47 deletions(-) diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index 6850843a25..d37e27000b 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -7846,14 +7846,13 @@ cmdIOThreadSet(vshControl *ctl, const vshCmd *cmd) { g_autoptr(virshDomain) dom = NULL; int id = 0; - bool ret = false; bool current = vshCommandOptBool(cmd, "current"); bool config = vshCommandOptBool(cmd, "config"); bool live = vshCommandOptBool(cmd, "live"); unsigned int flags = VIR_DOMAIN_AFFECT_CURRENT; - virTypedParameterPtr params = NULL; - int nparams = 0; - int maxparams = 0; + g_autoptr(virTypedParamList) params = virTypedParamListNew(); + virTypedParameterPtr par; + size_t npar = 0; unsigned long long poll_max; unsigned int poll_val; int thread_val; @@ -7871,64 +7870,49 @@ cmdIOThreadSet(vshControl *ctl, const vshCmd *cmd) return false; if (vshCommandOptInt(ctl, cmd, "id", &id) < 0) - goto cleanup; + return false; if (id <= 0) { vshError(ctl, _("Invalid IOThread id value: '%1$d'"), id); - goto cleanup; + return false; } - poll_val = 0; if ((rc = vshCommandOptULongLong(ctl, cmd, "poll-max-ns", &poll_max)) < 0) - goto cleanup; - if (rc > 0 && virTypedParamsAddULLong(¶ms, &nparams, &maxparams, - VIR_DOMAIN_IOTHREAD_POLL_MAX_NS, - poll_max) < 0) - goto save_error; + return false; + if (rc > 0) + virTypedParamListAddULLong(params, poll_max, VIR_DOMAIN_IOTHREAD_POLL_MAX_NS); -#define VSH_IOTHREAD_SET_UINT_PARAMS(opt, param) \ - poll_val = 0; \ - if ((rc = vshCommandOptUInt(ctl, cmd, opt, &poll_val)) < 0) \ - goto cleanup; \ - if (rc > 0 && \ - virTypedParamsAddUInt(¶ms, &nparams, &maxparams, \ - param, poll_val) < 0) \ - goto save_error; + if ((rc = vshCommandOptUInt(ctl, cmd, "poll-grow", &poll_val)) < 0) + return false; + if (rc > 0) + virTypedParamListAddUInt(params, poll_val, VIR_DOMAIN_IOTHREAD_POLL_GROW); - VSH_IOTHREAD_SET_UINT_PARAMS("poll-grow", VIR_DOMAIN_IOTHREAD_POLL_GROW) - VSH_IOTHREAD_SET_UINT_PARAMS("poll-shrink", VIR_DOMAIN_IOTHREAD_POLL_SHRINK) + if ((rc = vshCommandOptUInt(ctl, cmd, "poll-shrink", &poll_val)) < 0) + return false; + if (rc > 0) + virTypedParamListAddUInt(params, poll_val, VIR_DOMAIN_IOTHREAD_POLL_SHRINK); -#undef VSH_IOTHREAD_SET_UINT_PARAMS + if ((rc = vshCommandOptInt(ctl, cmd, "thread-pool-min", &thread_val)) < 0) + return false; + if (rc > 0) + virTypedParamListAddInt(params, thread_val, VIR_DOMAIN_IOTHREAD_THREAD_POOL_MIN); -#define VSH_IOTHREAD_SET_INT_PARAMS(opt, param) \ - thread_val = -1; \ - if ((rc = vshCommandOptInt(ctl, cmd, opt, &thread_val)) < 0) \ - goto cleanup; \ - if (rc > 0 && \ - virTypedParamsAddInt(¶ms, &nparams, &maxparams, \ - param, thread_val) < 0) \ - goto save_error; + if ((rc = vshCommandOptInt(ctl, cmd, "thread-pool-max", &thread_val)) < 0) + return false; + if (rc > 0) + virTypedParamListAddInt(params, thread_val, VIR_DOMAIN_IOTHREAD_THREAD_POOL_MAX); - VSH_IOTHREAD_SET_INT_PARAMS("thread-pool-min", VIR_DOMAIN_IOTHREAD_THREAD_POOL_MIN) - VSH_IOTHREAD_SET_INT_PARAMS("thread-pool-max", VIR_DOMAIN_IOTHREAD_THREAD_POOL_MAX) -#undef VSH_IOTHREAD_SET_INT_PARAMS + if (virTypedParamListFetch(params, &par, &npar) < 0) + return false; - if (nparams == 0) { + if (npar == 0) { vshError(ctl, _("Not enough arguments passed, nothing to set")); - goto cleanup; + return false; } - if (virDomainSetIOThreadParams(dom, id, params, nparams, flags) < 0) - goto cleanup; - - ret = true; - - cleanup: - virTypedParamsFree(params, nparams); - return ret; + if (virDomainSetIOThreadParams(dom, id, par, npar, flags) < 0) + return false; - save_error: - vshSaveLibvirtError(); - goto cleanup; + return true; } -- 2.39.2

QEMU accepts even values bigger than INT_MAX. The reasoning for these checks was that the QAPI definition declares them as 'int', but in QAPI terms that's any number as it's JSON. Remove the validation as well as the comment missintepreting the QAPI definiton. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_driver.c | 21 --------------------- src/qemu/qemu_monitor_json.c | 7 ------- 2 files changed, 28 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index d47c1883f0..23646b7289 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -5234,27 +5234,6 @@ qemuDomainIOThreadParseParams(virTypedParameterPtr params, if (rc == 1) iothread->set_thread_pool_max = true; - if (iothread->set_poll_max_ns && iothread->poll_max_ns > INT_MAX) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("poll-max-ns (%1$llu) must be less than or equal to %2$d"), - iothread->poll_max_ns, INT_MAX); - return -1; - } - - if (iothread->set_poll_grow && iothread->poll_grow > INT_MAX) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("poll-grow (%1$u) must be less than or equal to %2$d"), - iothread->poll_grow, INT_MAX); - return -1; - } - - if (iothread->set_poll_shrink && iothread->poll_shrink > INT_MAX) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("poll-shrink (%1$u) must be less than or equal to %2$d"), - iothread->poll_shrink, INT_MAX); - return -1; - } - if (iothread->set_thread_pool_min && iothread->thread_pool_min < -1) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("thread_pool_min (%1$d) must be equal to or greater than -1"), diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index b2c0b20a11..3454e85e43 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -7128,13 +7128,6 @@ qemuMonitorJSONGetIOThreads(qemuMonitor *mon, goto cleanup; } - /* Fetch poll values (since QEMU 2.9 ) if available. QEMU - * stores these values as int64_t's; however, the qapi type - * is an int. The qapi/misc.json also mis-describes the grow - * and shrink values as pure add/remove values. The source - * util/aio-posix.c function aio_poll uses them as a factor - * or divisor in it's calculation. We will fetch and store - * them as defined in our structures. */ if (virJSONValueObjectGetNumberUlong(child, "poll-max-ns", &info->poll_max_ns) == 0 && virJSONValueObjectGetNumberUint(child, "poll-grow", -- 2.39.2

On a Wednesday in 2023, Peter Krempa wrote:
QEMU accepts even values bigger than INT_MAX. The reasoning for these checks was that the QAPI definition declares them as 'int', but in QAPI terms that's any number as it's JSON.
Remove the validation as well as the comment missintepreting the QAPI
*misinterpreting
definiton.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_driver.c | 21 --------------------- src/qemu/qemu_monitor_json.c | 7 ------- 2 files changed, 28 deletions(-)
Jano

Convert the internal types to unsigned long long. Luckily we can also covert the external types too: - 'qemuDomainSetIOThreadParams' can accept both _UINT and _ULLONG by converting to 'virTypedParamsGetUnsigned' - querying is handled via the bulk stats API which is flexible: - we use virTypedParamListAddUnsigned to use the bigger type only if necessary - most users don't even notice because the bindings abstract the data types Apart from the code modifications we also improve the documenataion which was missing for the setters. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- include/libvirt/libvirt-domain.h | 4 +++- src/libvirt-domain.c | 14 +++++++---- src/qemu/qemu_driver.c | 28 +++++++++++----------- src/qemu/qemu_monitor.h | 4 ++-- src/qemu/qemu_monitor_json.c | 41 ++++++++++++++++++++------------ 5 files changed, 54 insertions(+), 37 deletions(-) diff --git a/include/libvirt/libvirt-domain.h b/include/libvirt/libvirt-domain.h index 3ebb2c6642..20862a69f2 100644 --- a/include/libvirt/libvirt-domain.h +++ b/include/libvirt/libvirt-domain.h @@ -2482,7 +2482,7 @@ int virDomainDelIOThread(virDomainPtr domain, * poll_grow and poll_shrink parameters provided. A value set too large * will cause more CPU time to be allocated the guest. A value set too * small will not provide enough cycles for the guest to process data. - * The polling interval is not available for statistical purposes. + * Accepted type is VIR_TYPED_PARAM_ULLONG. * * Since: 4.10.0 */ @@ -2495,6 +2495,7 @@ int virDomainDelIOThread(virDomainPtr domain, * use to grow its polling interval up to the poll_max_ns value. A value * of 0 (zero) allows the hypervisor to choose its own value. The algorithm * to use for adjustment is hypervisor specific. + * Accepted type is VIR_TYPED_PARAM_UINT or since 9.3.0 VIR_TYPED_PARAM_ULLONG. * * Since: 4.10.0 */ @@ -2508,6 +2509,7 @@ int virDomainDelIOThread(virDomainPtr domain, * the poll_max_ns value. A value of 0 (zero) allows the hypervisor to * choose its own value. The algorithm to use for adjustment is hypervisor * specific. + * Accepted type is VIR_TYPED_PARAM_UINT or since 9.3.0 VIR_TYPED_PARAM_ULLONG. * * Since: 4.10.0 */ diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c index 63829bb028..ec42bb9a53 100644 --- a/src/libvirt-domain.c +++ b/src/libvirt-domain.c @@ -12449,14 +12449,18 @@ virConnectGetDomainCapabilities(virConnectPtr conn, * "iothread.<id>.poll-max-ns" - maximum polling time in ns as an unsigned * long long. A 0 (zero) means polling is * disabled. - * "iothread.<id>.poll-grow" - polling time factor as an unsigned int. + * "iothread.<id>.poll-grow" - polling time factor as an unsigned int or + * unsigned long long if exceeding range of + * unsigned int. * A 0 (zero) indicates to allow the underlying * hypervisor to choose how to grow the * polling time. - * "iothread.<id>.poll-shrink" - polling time divisor as an unsigned int. - * A 0 (zero) indicates to allow the underlying - * hypervisor to choose how to shrink the - * polling time. + * "iothread.<id>.poll-shrink" - polling time divisor as an unsigned int or + * unsigned long long if exceeding range of + * unsigned int. + * A 0 (zero) indicates to allow the underlying + * hypervisor to choose how to shrink the + * polling time. * * VIR_DOMAIN_STATS_MEMORY: * Return memory bandwidth statistics and the usage information. The typed diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 23646b7289..dafe8af8a6 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -5189,9 +5189,9 @@ qemuDomainIOThreadParseParams(virTypedParameterPtr params, VIR_DOMAIN_IOTHREAD_POLL_MAX_NS, VIR_TYPED_PARAM_ULLONG, VIR_DOMAIN_IOTHREAD_POLL_GROW, - VIR_TYPED_PARAM_UINT, + VIR_TYPED_PARAM_UNSIGNED, VIR_DOMAIN_IOTHREAD_POLL_SHRINK, - VIR_TYPED_PARAM_UINT, + VIR_TYPED_PARAM_UNSIGNED, VIR_DOMAIN_IOTHREAD_THREAD_POOL_MIN, VIR_TYPED_PARAM_INT, VIR_DOMAIN_IOTHREAD_THREAD_POOL_MAX, @@ -5206,16 +5206,16 @@ qemuDomainIOThreadParseParams(virTypedParameterPtr params, if (rc == 1) iothread->set_poll_max_ns = true; - if ((rc = virTypedParamsGetUInt(params, nparams, - VIR_DOMAIN_IOTHREAD_POLL_GROW, - &iothread->poll_grow)) < 0) + if ((rc = virTypedParamsGetUnsigned(params, nparams, + VIR_DOMAIN_IOTHREAD_POLL_GROW, + &iothread->poll_grow)) < 0) return -1; if (rc == 1) iothread->set_poll_grow = true; - if ((rc = virTypedParamsGetUInt(params, nparams, - VIR_DOMAIN_IOTHREAD_POLL_SHRINK, - &iothread->poll_shrink)) < 0) + if ((rc = virTypedParamsGetUnsigned(params, nparams, + VIR_DOMAIN_IOTHREAD_POLL_SHRINK, + &iothread->poll_shrink)) < 0) return -1; if (rc == 1) iothread->set_poll_shrink = true; @@ -18048,12 +18048,12 @@ qemuDomainGetStatsIOThread(virQEMUDriver *driver G_GNUC_UNUSED, virTypedParamListAddULLong(params, iothreads[i]->poll_max_ns, "iothread.%u.poll-max-ns", iothreads[i]->iothread_id); - virTypedParamListAddUInt(params, iothreads[i]->poll_grow, - "iothread.%u.poll-grow", - iothreads[i]->iothread_id); - virTypedParamListAddUInt(params, iothreads[i]->poll_shrink, - "iothread.%u.poll-shrink", - iothreads[i]->iothread_id); + virTypedParamListAddUnsigned(params, iothreads[i]->poll_grow, + "iothread.%u.poll-grow", + iothreads[i]->iothread_id); + virTypedParamListAddUnsigned(params, iothreads[i]->poll_shrink, + "iothread.%u.poll-shrink", + iothreads[i]->iothread_id); } } diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h index 72db0c0838..09f22f2328 100644 --- a/src/qemu/qemu_monitor.h +++ b/src/qemu/qemu_monitor.h @@ -1254,8 +1254,8 @@ struct _qemuMonitorIOThreadInfo { int thread_id; bool poll_valid; unsigned long long poll_max_ns; - unsigned int poll_grow; - unsigned int poll_shrink; + unsigned long long poll_grow; + unsigned long long poll_shrink; int thread_pool_min; int thread_pool_max; bool set_poll_max_ns; diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index 3454e85e43..745d83e2b6 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -7130,10 +7130,10 @@ qemuMonitorJSONGetIOThreads(qemuMonitor *mon, if (virJSONValueObjectGetNumberUlong(child, "poll-max-ns", &info->poll_max_ns) == 0 && - virJSONValueObjectGetNumberUint(child, "poll-grow", - &info->poll_grow) == 0 && - virJSONValueObjectGetNumberUint(child, "poll-shrink", - &info->poll_shrink) == 0) + virJSONValueObjectGetNumberUlong(child, "poll-grow", + &info->poll_grow) == 0 && + virJSONValueObjectGetNumberUlong(child, "poll-shrink", + &info->poll_shrink) == 0) info->poll_valid = true; } @@ -7161,18 +7161,20 @@ qemuMonitorJSONSetIOThread(qemuMonitor *mon, path = g_strdup_printf("/objects/iothread%u", iothreadInfo->iothread_id); -#define VIR_IOTHREAD_SET_PROP(propName, propVal) \ +#define VIR_IOTHREAD_SET_PROP_UL(propName, propVal) \ if (iothreadInfo->set_##propVal) { \ memset(&prop, 0, sizeof(qemuMonitorJSONObjectProperty)); \ - prop.type = QEMU_MONITOR_OBJECT_PROPERTY_INT; \ - prop.val.iv = iothreadInfo->propVal; \ + prop.type = QEMU_MONITOR_OBJECT_PROPERTY_ULONG; \ + prop.val.ul = iothreadInfo->propVal; \ if (qemuMonitorJSONSetObjectProperty(mon, path, propName, &prop) < 0) \ return -1; \ } - VIR_IOTHREAD_SET_PROP("poll-max-ns", poll_max_ns); - VIR_IOTHREAD_SET_PROP("poll-grow", poll_grow); - VIR_IOTHREAD_SET_PROP("poll-shrink", poll_shrink); + VIR_IOTHREAD_SET_PROP_UL("poll-max-ns", poll_max_ns); + VIR_IOTHREAD_SET_PROP_UL("poll-grow", poll_grow); + VIR_IOTHREAD_SET_PROP_UL("poll-shrink", poll_shrink); + +#undef VIR_IOTHREAD_SET_PROP_UL if (iothreadInfo->set_thread_pool_min && iothreadInfo->set_thread_pool_max) { @@ -7192,15 +7194,24 @@ qemuMonitorJSONSetIOThread(qemuMonitor *mon, setMaxFirst = true; } +#define VIR_IOTHREAD_SET_PROP_INT(propName, propVal) \ + if (iothreadInfo->set_##propVal) { \ + memset(&prop, 0, sizeof(qemuMonitorJSONObjectProperty)); \ + prop.type = QEMU_MONITOR_OBJECT_PROPERTY_INT; \ + prop.val.iv = iothreadInfo->propVal; \ + if (qemuMonitorJSONSetObjectProperty(mon, path, propName, &prop) < 0) \ + return -1; \ + } + if (setMaxFirst) { - VIR_IOTHREAD_SET_PROP("thread-pool-max", thread_pool_max); - VIR_IOTHREAD_SET_PROP("thread-pool-min", thread_pool_min); + VIR_IOTHREAD_SET_PROP_INT("thread-pool-max", thread_pool_max); + VIR_IOTHREAD_SET_PROP_INT("thread-pool-min", thread_pool_min); } else { - VIR_IOTHREAD_SET_PROP("thread-pool-min", thread_pool_min); - VIR_IOTHREAD_SET_PROP("thread-pool-max", thread_pool_max); + VIR_IOTHREAD_SET_PROP_INT("thread-pool-min", thread_pool_min); + VIR_IOTHREAD_SET_PROP_INT("thread-pool-max", thread_pool_max); } -#undef VIR_IOTHREAD_SET_PROP +#undef VIR_IOTHREAD_SET_PROP_INT return 0; } -- 2.39.2

On a Wednesday in 2023, Peter Krempa wrote:
Convert the internal types to unsigned long long. Luckily we can also covert the external types too:
- 'qemuDomainSetIOThreadParams' can accept both _UINT and _ULLONG by converting to 'virTypedParamsGetUnsigned'
- querying is handled via the bulk stats API which is flexible: - we use virTypedParamListAddUnsigned to use the bigger type only if necessary - most users don't even notice because the bindings abstract the data types
Apart from the code modifications we also improve the documenataion
*documentation
which was missing for the setters.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- include/libvirt/libvirt-domain.h | 4 +++- src/libvirt-domain.c | 14 +++++++---- src/qemu/qemu_driver.c | 28 +++++++++++----------- src/qemu/qemu_monitor.h | 4 ++-- src/qemu/qemu_monitor_json.c | 41 ++++++++++++++++++++------------ 5 files changed, 54 insertions(+), 37 deletions(-)
Jano

The qemu driver now accepts also _ULLONG as type for bigger numbers. Use the 'virTypedParamListAddUnsigned' helper to use the bigger typed parameter type if necessary to allow full range of the values while preserving compatibility. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- tools/virsh-domain.c | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-) diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index d37e27000b..db47d63d85 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -7853,8 +7853,7 @@ cmdIOThreadSet(vshControl *ctl, const vshCmd *cmd) g_autoptr(virTypedParamList) params = virTypedParamListNew(); virTypedParameterPtr par; size_t npar = 0; - unsigned long long poll_max; - unsigned int poll_val; + unsigned long long poll_val; int thread_val; int rc; @@ -7876,20 +7875,20 @@ cmdIOThreadSet(vshControl *ctl, const vshCmd *cmd) return false; } - if ((rc = vshCommandOptULongLong(ctl, cmd, "poll-max-ns", &poll_max)) < 0) + if ((rc = vshCommandOptULongLong(ctl, cmd, "poll-max-ns", &poll_val)) < 0) return false; if (rc > 0) - virTypedParamListAddULLong(params, poll_max, VIR_DOMAIN_IOTHREAD_POLL_MAX_NS); + virTypedParamListAddULLong(params, poll_val, VIR_DOMAIN_IOTHREAD_POLL_MAX_NS); - if ((rc = vshCommandOptUInt(ctl, cmd, "poll-grow", &poll_val)) < 0) + if ((rc = vshCommandOptULongLong(ctl, cmd, "poll-grow", &poll_val)) < 0) return false; if (rc > 0) - virTypedParamListAddUInt(params, poll_val, VIR_DOMAIN_IOTHREAD_POLL_GROW); + virTypedParamListAddUnsigned(params, poll_val, VIR_DOMAIN_IOTHREAD_POLL_GROW); - if ((rc = vshCommandOptUInt(ctl, cmd, "poll-shrink", &poll_val)) < 0) + if ((rc = vshCommandOptULongLong(ctl, cmd, "poll-shrink", &poll_val)) < 0) return false; if (rc > 0) - virTypedParamListAddUInt(params, poll_val, VIR_DOMAIN_IOTHREAD_POLL_SHRINK); + virTypedParamListAddUnsigned(params, poll_val, VIR_DOMAIN_IOTHREAD_POLL_SHRINK); if ((rc = vshCommandOptInt(ctl, cmd, "thread-pool-min", &thread_val)) < 0) return false; -- 2.39.2

Currently we allow configuring the 'poll-max-ns', 'poll-grow', and 'poll-shrink' parameters of qemu iothreads only during runtime and they are not persisted. Add XML machinery to persist them. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- docs/formatdomain.rst | 11 ++++- src/conf/domain_conf.c | 41 ++++++++++++++++++- src/conf/domain_conf.h | 7 ++++ src/conf/schemas/domaincommon.rng | 19 +++++++++ .../iothreads-ids-pool-sizes.xml | 12 ++++-- 5 files changed, 85 insertions(+), 5 deletions(-) diff --git a/docs/formatdomain.rst b/docs/formatdomain.rst index c31c2fe35c..bfef1c63e6 100644 --- a/docs/formatdomain.rst +++ b/docs/formatdomain.rst @@ -740,7 +740,9 @@ host/guest with many LUNs. :since:`Since 1.2.8 (QEMU only)` <iothread id="2"/> <iothread id="4"/> <iothread id="6"/> - <iothread id="8" thread_pool_min="2" thread_pool_max="32"/> + <iothread id="8" thread_pool_min="2" thread_pool_max="32"> + <poll max='123' grow='456' shrink='789'/> + </iothread> </iothreadids> <defaultiothread thread_pool_min="8" thread_pool_max="16"/> ... @@ -766,6 +768,13 @@ host/guest with many LUNs. :since:`Since 1.2.8 (QEMU only)` ``thread_pool_max`` which allow setting lower and upper boundary for number of worker threads for given IOThread. While the former can be value of zero, the latter can't. :since:`Since 8.5.0` + :since:`Since 9.1.0` an optional sub-element ``poll`` with can be used to + override the hypervisor-default interval of polling for the iothread before + it switches back to events. The optional attribute ``max`` sets the maximum + time polling should be used in nanoseconds. Setting ``max`` to ``0`` disables + polling. Attributes ``grow`` and ``shrink`` override (or disable when set to + ``0`` the default steps for increasing/decreasing the polling interval if + the set interval is deemed insufficient or extensive. ``defaultiothread`` This element represents the default event loop within hypervisor, where I/O requests from devices not assigned to a specific IOThread are processed. diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index b03a3ff011..eccabe07bc 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -15742,6 +15742,7 @@ static virDomainIOThreadIDDef * virDomainIOThreadIDDefParseXML(xmlNodePtr node) { g_autoptr(virDomainIOThreadIDDef) iothrid = virDomainIOThreadIDDefNew(); + xmlNodePtr pollNode; if (virXMLPropUInt(node, "id", 10, VIR_XML_PROP_REQUIRED | VIR_XML_PROP_NONZERO, @@ -15758,6 +15759,28 @@ virDomainIOThreadIDDefParseXML(xmlNodePtr node) &iothrid->thread_pool_max, -1) < 0) return NULL; + if ((pollNode = virXMLNodeGetSubelement(node, "poll"))) { + int rc; + + if ((rc = virXMLPropULongLong(pollNode, "max", 10, VIR_XML_PROP_NONE, + &iothrid->poll_max_ns)) < 0) + return NULL; + + iothrid->set_poll_max_ns = rc == 1; + + if ((rc = virXMLPropULongLong(pollNode, "grow", 10, VIR_XML_PROP_NONE, + &iothrid->poll_grow)) < 0) + return NULL; + + iothrid->set_poll_grow = rc == 1; + + if ((rc = virXMLPropULongLong(pollNode, "shrink", 10, VIR_XML_PROP_NONE, + &iothrid->poll_shrink)) < 0) + return NULL; + + iothrid->set_poll_shrink = rc == 1; + } + return g_steal_pointer(&iothrid); } @@ -26652,6 +26675,9 @@ virDomainDefIothreadShouldFormat(const virDomainDef *def) for (i = 0; i < def->niothreadids; i++) { if (!def->iothreadids[i]->autofill || + def->iothreadids[i]->set_poll_max_ns || + def->iothreadids[i]->set_poll_grow || + def->iothreadids[i]->set_poll_shrink || def->iothreadids[i]->thread_pool_min >= 0 || def->iothreadids[i]->thread_pool_max >= 0) return true; @@ -26700,6 +26726,8 @@ virDomainDefIOThreadsFormat(virBuffer *buf, for (i = 0; i < def->niothreadids; i++) { virDomainIOThreadIDDef *iothread = def->iothreadids[i]; g_auto(virBuffer) attrBuf = VIR_BUFFER_INITIALIZER; + g_auto(virBuffer) iothreadChildBuf = VIR_BUFFER_INIT_CHILD(&childrenBuf); + g_auto(virBuffer) pollAttrBuf = VIR_BUFFER_INITIALIZER; virBufferAsprintf(&attrBuf, " id='%u'", iothread->iothread_id); @@ -26714,7 +26742,18 @@ virDomainDefIOThreadsFormat(virBuffer *buf, iothread->thread_pool_max); } - virXMLFormatElement(&childrenBuf, "iothread", &attrBuf, NULL); + if (iothread->set_poll_max_ns) + virBufferAsprintf(&pollAttrBuf, " max='%llu'", iothread->poll_max_ns); + + if (iothread->set_poll_grow) + virBufferAsprintf(&pollAttrBuf, " grow='%llu'", iothread->poll_grow); + + if (iothread->set_poll_shrink) + virBufferAsprintf(&pollAttrBuf, " shrink='%llu'", iothread->poll_shrink); + + virXMLFormatElement(&iothreadChildBuf, "poll", &pollAttrBuf, NULL); + + virXMLFormatElement(&childrenBuf, "iothread", &attrBuf, &iothreadChildBuf); } virXMLFormatElement(buf, "iothreadids", NULL, &childrenBuf); diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 511067a050..83d3da45d7 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -2714,6 +2714,13 @@ struct _virDomainIOThreadIDDef { virDomainThreadSchedParam sched; + unsigned long long poll_max_ns; + bool set_poll_max_ns; + unsigned long long poll_grow; + bool set_poll_grow; + unsigned long long poll_shrink; + bool set_poll_shrink; + int thread_pool_min; int thread_pool_max; }; diff --git a/src/conf/schemas/domaincommon.rng b/src/conf/schemas/domaincommon.rng index 6158ed79ac..57fb4a5e33 100644 --- a/src/conf/schemas/domaincommon.rng +++ b/src/conf/schemas/domaincommon.rng @@ -885,6 +885,25 @@ <ref name="unsignedInt"/> </attribute> </optional> + <optional> + <element name="poll"> + <optional> + <attribute name="max"> + <ref name="unsignedInt"/> + </attribute> + </optional> + <optional> + <attribute name="grow"> + <ref name="unsignedInt"/> + </attribute> + </optional> + <optional> + <attribute name="shrink"> + <ref name="unsignedInt"/> + </attribute> + </optional> + </element> + </optional> </element> </zeroOrMore> </element> diff --git a/tests/qemuxml2argvdata/iothreads-ids-pool-sizes.xml b/tests/qemuxml2argvdata/iothreads-ids-pool-sizes.xml index df4b291a7a..63fb4a52f6 100644 --- a/tests/qemuxml2argvdata/iothreads-ids-pool-sizes.xml +++ b/tests/qemuxml2argvdata/iothreads-ids-pool-sizes.xml @@ -7,9 +7,15 @@ <iothreads>5</iothreads> <iothreadids> <iothread id='2' thread_pool_min='0' thread_pool_max='60'/> - <iothread id='4' thread_pool_min='1' thread_pool_max='1'/> - <iothread id='1'/> - <iothread id='3'/> + <iothread id='4' thread_pool_min='1' thread_pool_max='1'> + <poll max='123'/> + </iothread> + <iothread id='1'> + <poll grow='456' shrink='789'/> + </iothread> + <iothread id='3'> + <poll max='123000' grow='456' shrink='789'/> + </iothread> <iothread id='5'/> </iothreadids> <defaultiothread thread_pool_min='8' thread_pool_max='16'/> -- 2.39.2

On a Wednesday in 2023, Peter Krempa wrote:
Currently we allow configuring the 'poll-max-ns', 'poll-grow', and 'poll-shrink' parameters of qemu iothreads only during runtime and they are not persisted. Add XML machinery to persist them.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- docs/formatdomain.rst | 11 ++++- src/conf/domain_conf.c | 41 ++++++++++++++++++- src/conf/domain_conf.h | 7 ++++ src/conf/schemas/domaincommon.rng | 19 +++++++++ .../iothreads-ids-pool-sizes.xml | 12 ++++-- 5 files changed, 85 insertions(+), 5 deletions(-)
diff --git a/docs/formatdomain.rst b/docs/formatdomain.rst index c31c2fe35c..bfef1c63e6 100644 --- a/docs/formatdomain.rst +++ b/docs/formatdomain.rst @@ -740,7 +740,9 @@ host/guest with many LUNs. :since:`Since 1.2.8 (QEMU only)` <iothread id="2"/> <iothread id="4"/> <iothread id="6"/> - <iothread id="8" thread_pool_min="2" thread_pool_max="32"/> + <iothread id="8" thread_pool_min="2" thread_pool_max="32"> + <poll max='123' grow='456' shrink='789'/> + </iothread> </iothreadids> <defaultiothread thread_pool_min="8" thread_pool_max="16"/> ... @@ -766,6 +768,13 @@ host/guest with many LUNs. :since:`Since 1.2.8 (QEMU only)` ``thread_pool_max`` which allow setting lower and upper boundary for number of worker threads for given IOThread. While the former can be value of zero, the latter can't. :since:`Since 8.5.0` + :since:`Since 9.1.0` an optional sub-element ``poll`` with can be used to
9.3.0
+ override the hypervisor-default interval of polling for the iothread before + it switches back to events. The optional attribute ``max`` sets the maximum + time polling should be used in nanoseconds. Setting ``max`` to ``0`` disables + polling. Attributes ``grow`` and ``shrink`` override (or disable when set to + ``0`` the default steps for increasing/decreasing the polling interval if + the set interval is deemed insufficient or extensive.
Jano

Implement the support for the persisted poll parameters and remove restrictions on saving config when modifying them during runtime. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_command.c | 18 +++++++++++ src/qemu/qemu_driver.c | 30 ++++++++++--------- ...othreads-ids-pool-sizes.x86_64-latest.args | 6 ++-- 3 files changed, 37 insertions(+), 17 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 4ca93bf3dc..f5b1971f07 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -7298,6 +7298,24 @@ qemuBuildIOThreadCommandLine(virCommand *cmd, NULL) < 0) return -1; + if (iothread->set_poll_max_ns && + virJSONValueObjectAdd(&props, + "U:poll-max-ns", iothread->poll_max_ns, + NULL) < 0) + return -1; + + if (iothread->set_poll_grow && + virJSONValueObjectAdd(&props, + "U:poll-grow", iothread->poll_grow, + NULL) < 0) + return -1; + + if (iothread->set_poll_shrink && + virJSONValueObjectAdd(&props, + "U:poll-shrink", iothread->poll_shrink, + NULL) < 0) + return -1; + if (qemuBuildObjectCommandlineFromJSON(cmd, props, qemuCaps) < 0) return -1; } diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index dafe8af8a6..1917e7e079 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -5067,23 +5067,30 @@ qemuDomainHotplugModIOThread(virDomainObj *vm, } -static int +static void qemuDomainHotplugModIOThreadIDDef(virDomainIOThreadIDDef *def, qemuMonitorIOThreadInfo mondef) { - /* These have no representation in domain XML */ - if (mondef.set_poll_grow || - mondef.set_poll_max_ns || - mondef.set_poll_shrink) - return -1; + if (mondef.set_poll_max_ns) { + def->poll_max_ns = mondef.poll_max_ns; + def->set_poll_max_ns = true; + } + + if (mondef.set_poll_grow) { + def->poll_grow = mondef.poll_grow; + def->set_poll_grow = true; + } + + if (mondef.set_poll_shrink) { + def->poll_shrink = mondef.poll_shrink; + def->set_poll_shrink = true; + } if (mondef.set_thread_pool_min) def->thread_pool_min = mondef.thread_pool_min; if (mondef.set_thread_pool_max) def->thread_pool_max = mondef.thread_pool_max; - - return 0; } @@ -5380,12 +5387,7 @@ qemuDomainChgIOThread(virQEMUDriver *driver, if (qemuDomainIOThreadValidate(iothreaddef, iothread, false) < 0) goto endjob; - if (qemuDomainHotplugModIOThreadIDDef(iothreaddef, iothread) < 0) { - virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s", - _("configuring persistent polling values is not supported")); - goto endjob; - } - + qemuDomainHotplugModIOThreadIDDef(iothreaddef, iothread); break; } } diff --git a/tests/qemuxml2argvdata/iothreads-ids-pool-sizes.x86_64-latest.args b/tests/qemuxml2argvdata/iothreads-ids-pool-sizes.x86_64-latest.args index 4f5a11aab1..1422e94e35 100644 --- a/tests/qemuxml2argvdata/iothreads-ids-pool-sizes.x86_64-latest.args +++ b/tests/qemuxml2argvdata/iothreads-ids-pool-sizes.x86_64-latest.args @@ -18,9 +18,9 @@ XDG_CONFIG_HOME=/var/lib/libvirt/qemu/domain--1-QEMUGuest1/.config \ -overcommit mem-lock=off \ -smp 6,sockets=6,cores=1,threads=1 \ -object '{"qom-type":"iothread","id":"iothread2","thread-pool-min":0,"thread-pool-max":60}' \ --object '{"qom-type":"iothread","id":"iothread4","thread-pool-min":1,"thread-pool-max":1}' \ --object '{"qom-type":"iothread","id":"iothread1"}' \ --object '{"qom-type":"iothread","id":"iothread3"}' \ +-object '{"qom-type":"iothread","id":"iothread4","thread-pool-min":1,"thread-pool-max":1,"poll-max-ns":123}' \ +-object '{"qom-type":"iothread","id":"iothread1","poll-grow":456,"poll-shrink":789}' \ +-object '{"qom-type":"iothread","id":"iothread3","poll-max-ns":123000,"poll-grow":456,"poll-shrink":789}' \ -object '{"qom-type":"iothread","id":"iothread5"}' \ -object '{"qom-type":"main-loop","id":"main-loop","thread-pool-min":8,"thread-pool-max":16}' \ -uuid c7a5fdbd-edaf-9455-926a-d65c16db1809 \ -- 2.39.2

On a Wednesday in 2023, Peter Krempa wrote:
Previous version:
https://listman.redhat.com/archives/libvir-list/2023-March/239164.html
Changes: - patches 1 - 23 are new and do the following - refactors to typed param handling 1 - 15, 18 - addition of new typed param APIs needed for changing the type we use to store the poll parameters currently, 16, 17, 19 - conversion of the existing parameter handling for iothread poll attributes to unsigned long long 20 - 23 - patches 24 and 25 are patches 1 and 2 of the previous posting: - use unsigned long long to store the values and adjust the code
Peter Krempa (25): virTypedParameterAssignValue: Drop 'copystr' parameter util: virtypedparam: Use proper enum type for all switch() statements virTypedParamsDeserialize: Remove unnecessary line breaks virTypedParameterAssignValueVArgs: Ensure proper typed param type in caller util: virtypedparam: Simplify error handling in virTypedParamListAdd* virtypedparam.h: Consistently use contemporary header style util: virtypedparam: Introduce virTypedParamListNew() util: typedparam: Introduce 'virTypedParamListConcat' qemuDomainGetStatsBlock: Don't directly access virTypedParamList util: virtypedparam: Introduce 'virTypedParamListFetch' Use 'virTypedParamListFetch' for extracting identity parameters list util: virtypedparam: Privatize definition of struct _virTypedParamList util: virtypedparam: Refactor return value of virTypedParamListStealParams util: virtypedparam: Store errors inside virTypedParamList util: virtypedparam: Remove return values from virTypedParamListAdd* APIs util: typedparam: Introduce virTypedParamListAddUnsigned util: virtypedparam: Introduce virTypedParamsGetUnsigned virTypedParamsValidate: Refactor variable declaration and cleanup virTypedParamsValidate: Allow typed params to be both _UINT and _ULLONG virsh: cmdIOThreadSet: Refactor to use virTypedParamList qemu: Remove iothread 'poll-' value validation qemu: Store all iothread's 'poll*' attributes as unsigned long long virsh: cmdIOThreadSet: Use bigger types for --poll-grow and --poll-shrink conf: Store the iothread 'poll' settings in the XML qemu: Use configured iothread poll parameters on startup
[..]
21 files changed, 781 insertions(+), 706 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
participants (3)
-
Ján Tomko
-
Martin Kletzander
-
Peter Krempa