Re: [PATCH 07/11] tools/xenstored: implement the GET/SET_QUOTA commands

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

On Thu, Mar 05, 2026 at 02:52:04PM +0100, Juergen Gross wrote:
> diff --git a/tools/xenstored/core.c b/tools/xenstored/core.c
> index 8a06b35808..e283d47184 100644
> --- a/tools/xenstored/core.c
> +++ b/tools/xenstored/core.c
> @@ -2034,6 +2034,10 @@ static struct {
>  	    { "GET_FEATURE",   do_get_feature,  XS_FLAG_PRIV },
>  	[XS_SET_FEATURE]       =
>  	    { "SET_FEATURE",   do_set_feature,  XS_FLAG_PRIV },
> +	[XS_GET_QUOTA]         =
> +	    { "GET_QUOTA",     do_get_quota,    XS_FLAG_PRIV },
> +	[XS_SET_QUOTA]         =
> +	    { "SET_QUOTA",     do_set_quota,    XS_FLAG_PRIV },
>  };
>  
>  static const char *sockmsg_string(enum xsd_sockmsg_type type)
> diff --git a/tools/xenstored/domain.c b/tools/xenstored/domain.c
> index 8e52351695..c0bc8a3eb7 100644
> --- a/tools/xenstored/domain.c
> +++ b/tools/xenstored/domain.c
> @@ -1363,6 +1363,112 @@ static bool parse_quota_name(const char *name, unsigned int *qidx,
>  	return true;
>  }
>  
> +int do_get_quota(const void *ctx, struct connection *conn,
> +		 struct buffered_data *in)
> +{
> +	const char *vec[2];
> +	unsigned int n_pars;
> +	unsigned int domid;
> +	unsigned int q;
> +	unsigned int idx;
> +	char *resp;
> +	const char *name;
> +	const struct quota *quota;
> +	const struct domain *domain;
> +
> +	n_pars = get_strings(in, vec, ARRAY_SIZE(vec));
> +
> +	if (n_pars > 2)
> +		return EINVAL;
> +
> +	if (n_pars == 0) {
> +		resp = talloc_asprintf(ctx, "%s", "");

This could be written with talloc_strdup() instead, since there's no
formatting involve.

> +		if (!resp)
> +			return ENOMEM;
> +		for (q = 0; q < ACC_N; q++) {
> +			if (!quota_adm[q].name)
> +				continue;
> +			if (quotas[q].val[Q_IDX_HARD] != Q_VAL_DISABLED) {

Having set internally a value of Q_VAL_DISABLED, does it mean the named
quota is unsupported?

> +				resp = talloc_asprintf_append(resp, "%s%s",
> +					*resp ? " " : "", quota_adm[q].name);
> +				if (!resp)
> +					return ENOMEM;
> +			}
> +			if (quotas[q].val[Q_IDX_SOFT] != Q_VAL_DISABLED) {
> +				resp = talloc_asprintf_append(resp, "%ssoft-%s",
> +					*resp ? " " : "", quota_adm[q].name);
> +				if (!resp)
> +					return ENOMEM;
> +			}
> +		}
> +	} else {
> +		if (n_pars == 1) {
> +			quota = quotas;
> +			name = vec[0];
> +		} else {
> +			domid = atoi(vec[0]);

Shall we check that vec[0] actually contain a plausible domid? (An
integer between 0..65535). Right now, this accept everything, and would
return 0 if there's not a single digit.

> +			domain = find_or_alloc_existing_domain(domid);
> +			if (!domain)
> +				return ENOENT;
> +			quota = domain->acc;
> +			name = vec[1];
> +		}
> +
> +		if (parse_quota_name(name, &q, &idx))
> +			return EINVAL;
> +
> +		resp = talloc_asprintf(ctx, "%u", quota[q].val[idx]);

Why do we return 4294967295 for disabled quota check when the spec say
to return "0" when a quota check is disabled? That is for quota names
that are supposed to be not supported (if we ask "GET_QUOTA" first).

> +		if (!resp)
> +			return ENOMEM;
> +	}
> +
> +	send_reply(conn, XS_GET_QUOTA, resp, strlen(resp) + 1);
> +
> +	return 0;
> +}
> +
> +int do_set_quota(const void *ctx, struct connection *conn,
> +		 struct buffered_data *in)
> +{
> +	const char *vec[3];
> +	unsigned int n_pars;
> +	unsigned int domid;
> +	unsigned int q;
> +	unsigned int idx;
> +	const char *name;
> +	unsigned int val;
> +	struct quota *quota;
> +	struct domain *domain;
> +
> +	n_pars = get_strings(in, vec, ARRAY_SIZE(vec));
> +
> +	if (n_pars < 2 || n_pars > 3)
> +		return EINVAL;
> +
> +	if (n_pars == 2) {
> +		quota = quotas;
> +		name = vec[0];
> +		val = atoi(vec[1]);

We should check that vec[1] is a valid quota value, and also not an
internal value. Otherwise, we can just have "-1" on the wire, and have
unexpected changes for example. Only "0" is documented as a quota been
disabled, "-1" or "4294967295" isn't.

> +	} else {
> +		domid = atoi(vec[0]);
> +		domain = find_or_alloc_existing_domain(domid);
> +		if (!domain)
> +			return ENOENT;
> +		quota = domain->acc;
> +		name = vec[1];
> +		val = atoi(vec[2]);
> +	}
> +
> +	if (parse_quota_name(name, &q, &idx))
> +		return EINVAL;
> +
> +	quota[q].val[idx] = val;
> +
> +	send_ack(conn, XS_SET_QUOTA);
> +
> +	return 0;
> +}
> +
>  static int close_xgt_handle(void *_handle)
>  {
>  	xengnttab_close(*(xengnttab_handle **)_handle);
> diff --git a/tools/xenstored/domain.h b/tools/xenstored/domain.h
> index 62ce3b3166..6a06b0d1af 100644
> --- a/tools/xenstored/domain.h
> +++ b/tools/xenstored/domain.h
> @@ -93,6 +93,14 @@ int do_get_feature(const void *ctx, struct connection *conn,
>  int do_set_feature(const void *ctx, struct connection *conn,
>  		   struct buffered_data *in);
>  
> +/* Get quota names or value */

This could say "implement GET_QUOTA" or something instead. But a
comment here isn't going to give much value for internal functions.

> +int do_get_quota(const void *ctx, struct connection *conn,
> +		 struct buffered_data *in);
> +
> +/* Set quota value */
> +int do_set_quota(const void *ctx, struct connection *conn,
> +		 struct buffered_data *in);
> +
>  void domain_early_init(void);
>  void domain_init(int evtfd);
>  void init_domains(bool live_update);


Thanks,


--
Anthony Perard | Vates XCP-ng Developer

XCP-ng & Xen Orchestra - Vates solutions

web: https://vates.tech