Re: [PATCH 01/19] src: add constants for guest info 'user.' parameters

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(a)redhat.com&gt; > --- > 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.

> > - 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.