On Tue, Mar 04, 2025 at 03:20:56PM +0100, Peter Krempa wrote: > 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. Yeah, its a double edged sword, as I really didn't want to duplicate the information, nor encourage users to use the hardcoded strings instead of the constants. I guess we're effectively dumbing down guest info/stats to the same level of docs "quality" as all the other typed parameter APIs. > > > > - 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. Doh, I missed that those changes merged now. With regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
