Re: [PATCH 01/11] tools/libs/store: add get- and set-quota related functions

["Anthony PERARD" <anthony.perard@vates.tech>]

On Thu, Mar 05, 2026 at 02:51:58PM +0100, Juergen Gross wrote:
> diff --git a/tools/include/xenstore.h b/tools/include/xenstore.h
> index 423422dc50..6b661e5895 100644
> --- a/tools/include/xenstore.h
> +++ b/tools/include/xenstore.h
> @@ -277,6 +277,25 @@ bool xs_get_features_domain(struct xs_handle *h, unsigned int domid,
>  bool xs_set_features_domain(struct xs_handle *h, unsigned int domid,
>  			    unsigned int features);
>  
> +/* Get names of supported quota. */
> +char **xs_get_quota_names(struct xs_handle *h, unsigned int *num);
> +
> +/* Get the value of one global quota. */
> +bool xs_get_global_quota(struct xs_handle *h, char *quota,
> +			 unsigned int *value);
> +
> +/* Set the value of one global quota. */
> +bool xs_set_global_quota(struct xs_handle *h, char *quota,
> +			 unsigned int value);
> +
> +/* Get the value of one domain quota. */
> +bool xs_get_domain_quota(struct xs_handle *h, unsigned int domid,
> +			 char *quota, unsigned int *value);
> +
> +/* Set the value of one domain quota. */
> +bool xs_set_domain_quota(struct xs_handle *h, unsigned int domid,
> +			 char *quota, unsigned int value);
> +

Do you think all those new prototype could get a bit more descriptions?
Which parameter are actually output (and not input), what does it mean
to return false, do they set errno, is there something to do with the
return value of xs_get_quota_names?

For output arguments, libxl have a convention (well at least a mention
in the coding style) to suffix argument names with `_r` or `_out`.

For the strings, could we use `const char *` instead of non-const one?

> diff --git a/tools/libs/store/xs.c b/tools/libs/store/xs.c
> index 8f4b90a3cf..dda37f7526 100644
> --- a/tools/libs/store/xs.c
> +++ b/tools/libs/store/xs.c
> @@ -1456,6 +1456,117 @@ bool xs_set_features_domain(struct xs_handle *h, unsigned int domid,
>  	return xs_bool(xs_talkv(h, iov, ARRAY_SIZE(iov), NULL));
>  }
>  
> +char **xs_get_quota_names(struct xs_handle *h, unsigned int *num)
> +{
> +	struct xsd_sockmsg msg = { .type = XS_GET_QUOTA };
> +	struct iovec iov[1];
> +	char **quota;
> +	char *reply;
> +	char *c;
> +	unsigned int i;
> +
> +	iov[0].iov_base = &msg;
> +	iov[0].iov_len  = sizeof(msg);
> +
> +	reply = xs_talkv(h, iov, ARRAY_SIZE(iov), NULL);
> +	if (!reply)
> +		return NULL;
> +
> +	*num = 1;
> +	for (c = reply; *c; c++)
> +		if (*c == ' ')
> +			(*num)++;
> +
> +	quota = malloc(*num * sizeof(char *) + strlen(reply) + 1);
> +	c = (char *)(quota + *num);
> +	strcpy(c, reply);
> +	for (i = 0; i < *num; i++) {
> +		quota[i] = c;
> +		c = strchr(c, ' ');
> +		if (c) {

If `c` is NULL, it's likely that this is the last iteration of the `for`
loop. But just in case, should we prevent the code from doing another
round and prevent `strchr(NULL, ' ')`? (Or just check that `c` is !NULL,
and let the loop finish set NULL for the remaining slot in `quota`)

> +			*c = 0;
> +			c++;
> +		}
> +	}
> +
> +	return quota;
> +}
> +
> +bool xs_get_global_quota(struct xs_handle *h, char *quota,
> +			 unsigned int *value)
> +{
> +	struct xsd_sockmsg msg = { .type = XS_GET_QUOTA };
> +	struct iovec iov[2];
> +
> +	iov[0].iov_base = &msg;
> +	iov[0].iov_len  = sizeof(msg);
> +	iov[1].iov_base = quota;
> +	iov[1].iov_len  = strlen(quota) + 1;
> +
> +	return xs_uint(xs_talkv(h, iov, ARRAY_SIZE(iov), NULL), value);
> +}
> +
> +bool xs_set_global_quota(struct xs_handle *h, char *quota,
> +			 unsigned int value)
> +{
> +	struct xsd_sockmsg msg = { .type = XS_SET_QUOTA };
> +	char val_str[MAX_STRLEN(value)];

MAX_STRLEN doesn't have a great name, I wounder what is was :-). And
it's not about a maximum size of payload that could go on xs wire or
something, it's actually the maximum string size that can take a
numerical value, when converted to charaters.


The rest looks fine to me.

Thanks,


--
Anthony Perard | Vates XCP-ng Developer

XCP-ng & Xen Orchestra - Vates solutions

web: https://vates.tech