Couple notes before doing a proper review:
On Tue, Mar 04, 2025 at 14:03:56 +0000, Daniel P. Berrangé wrote:
> Contrary to most APIs returning typed parameters, there are no constants
> defined for the guest info data keys. This is was because many of the
> keys needs to be dynamically constructed using one or more array index
> values.
>
> It is possible to define constants while still supporting dynamic
> array indexes by simply defining the prefixes and suffixes as constants.
> The consuming code can then combine the constants with array index
> value.
>
> With this approach, it is practical to add constants for the guest info
> API keys.
>
> Signed-off-by: Daniel P. Berrangé <berrange@xxxxxxxxxx>
> ---
> include/libvirt/libvirt-domain.h | 49 ++++++++++++++++++++++++++++++++
> src/libvirt-domain.c | 14 +++------
> src/qemu/qemu_agent.c | 12 +++++---
> 3 files changed, 61 insertions(+), 14 deletions(-)
>
> diff --git a/include/libvirt/libvirt-domain.h b/include/libvirt/libvirt-domain.h
> index f5420bca6e..cef8bd4dbe 100644
> --- a/include/libvirt/libvirt-domain.h
> +++ b/include/libvirt/libvirt-domain.h
> @@ -6442,6 +6442,55 @@ int virDomainSetLaunchSecurityState(virDomainPtr domain,
> int nparams,
> unsigned int flags);
>
> +/**
> + * VIR_DOMAIN_GUEST_INFO_USER_COUNT:
> + *
> + * The number of active users on this domain as an unsigned int.
> + *
> + * Since: 11.2.0
This makes it sound as if the field was available since the version
which is not exactly true. This is especially true as ...
> + */
> +#define VIR_DOMAIN_GUEST_INFO_USER_COUNT "user.count"
> +
> +/**
> + * VIR_DOMAIN_GUEST_INFO_USER_PREFIX:
> + *
> + * The parameter name prefix to access each user entry. Concatenate the
> + * prefix, the entry number formatted as an unsigned integer and one of the
> + * user suffix parameters to form a complete parameter name.
> + *
> + * Since: 11.2.0
> + */
> +#define VIR_DOMAIN_GUEST_INFO_USER_PREFIX "user."
> +
> +/**
> + * VIR_DOMAIN_GUEST_INFO_USER_SUFFIX_NAME:
> + *
> + * Username of the user as a string.
> + *
> + * Since: 11.2.0
> + */
> +#define VIR_DOMAIN_GUEST_INFO_USER_SUFFIX_NAME ".name"
> +
> +/**
> + * VIR_DOMAIN_GUEST_INFO_USER_SUFFIX_DOMAIN:
> + *
> + * Domain of the user as a string (may only be present on certain guest
> + * types).
> + *
> + * Since: 11.2.0
> + */
> +#define VIR_DOMAIN_GUEST_INFO_USER_SUFFIX_DOMAIN ".domain"
> +
> +/**
> + * VIR_DOMAIN_GUEST_INFO_USER_SUFFIX_LOGIN_TIME:
> + *
> + * The login time of a user in milliseconds since the epoch as unsigned long
> + * long.
> + *
> + * Since: 11.2.0
> + */
> +#define VIR_DOMAIN_GUEST_INFO_USER_SUFFIX_LOGIN_TIME ".login-time"
> +
> /**
> * virDomainGuestInfoTypes:
> *
> diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c
> index 072cc32255..8485dad668 100644
> --- a/src/libvirt-domain.c
> +++ b/src/libvirt-domain.c
> @@ -13199,16 +13199,10 @@ virDomainSetVcpu(virDomainPtr domain,
> * (although not necessarily implemented for each hypervisor):
> *
> * VIR_DOMAIN_GUEST_INFO_USERS:
> - * returns information about users that are currently logged in within the
> - * guest domain. The typed parameter keys are in this format:
> - *
> - * "user.count" - the number of active users on this domain as an
> - * unsigned int
> - * "user.<num>.name" - username of the user as a string
> - * "user.<num>.domain" - domain of the user as a string (may only be
> - * present on certain guest types)
> - * "user.<num>.login-time" - the login time of a user in milliseconds
> - * since the epoch as unsigned long long
... you delete this ...
> + * Return information about users that are currently logged in within the
> + * guest domain.
> + * The VIR_DOMAIN_GUEST_INFO_USER_* constants define the known typed parameter
> + * keys.
... and replace it by the reference here.
I also understand the reason but honestly for human consumption the
existing format was more understandable. At least we keep that in virsh.
> *
> * VIR_DOMAIN_GUEST_INFO_OS:
> * Return information about the operating system running within the guest. The
> diff --git a/src/qemu/qemu_agent.c b/src/qemu/qemu_agent.c
> index 43fca86f10..cb9cce7f6a 100644
> --- a/src/qemu/qemu_agent.c
> +++ b/src/qemu/qemu_agent.c
> @@ -2200,7 +2200,7 @@ qemuAgentGetUsers(qemuAgent *agent,
> ndata = virJSONValueArraySize(data);
>
> if (virTypedParamsAddUInt(params, nparams, maxparams,
> - "user.count", ndata) < 0)
> + VIR_DOMAIN_GUEST_INFO_USER_COUNT, ndata) < 0)
> return -1;
>
> for (i = 0; i < ndata; i++) {
> @@ -2221,7 +2221,9 @@ qemuAgentGetUsers(qemuAgent *agent,
> return -1;
> }
>
> - g_snprintf(param_name, VIR_TYPED_PARAM_FIELD_LENGTH, "user.%zu.name", i);
> + g_snprintf(param_name, VIR_TYPED_PARAM_FIELD_LENGTH,
> + VIR_DOMAIN_GUEST_INFO_USER_PREFIX "%zu"
> + VIR_DOMAIN_GUEST_INFO_USER_SUFFIX_NAME, i);
> if (virTypedParamsAddString(params, nparams, maxparams,
> param_name, strvalue) < 0)
All of the above code was refactored recently to use
'virTypedParamListAdd*' so all of the patches dealing with guest agent
info will need to be rebased.
