Re: [PATCH 04/11] tools/xenstored: add GLOBAL_QUOTA_DATA record for live update

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

On Thu, Mar 05, 2026 at 02:52:01PM +0100, Juergen Gross wrote:
> diff --git a/tools/xenstored/domain.c b/tools/xenstored/domain.c
> index acdcaa769e..694ae58973 100644
> --- a/tools/xenstored/domain.c
> +++ b/tools/xenstored/domain.c
> @@ -1332,6 +1332,27 @@ int do_set_feature(const void *ctx, struct connection *conn,
>  	return 0;
>  }
>  
> +static bool parse_quota_name(const char *name, unsigned int *qidx,
> +			     unsigned int *idx)
> +{
> +	unsigned int q;

What do you think of using something like:
    const char soft_prefix[] = "soft-";
    const size_t soft_prefix_len = sizeof(soft_prefix) - 1;
to explain the `5`, here and in e.g. the function build_quota_data() ?
We used this in libxl in one place:
    https://elixir.bootlin.com/xen/v4.21.0/source/tools/libs/light/libxl_qmp.c#L1288

But it's fine to leave it like that, as the '5's are close enought to
the prefix that we can guess easly enough.

> +
> +	if (strncmp(name, "soft-", 5)) {
> +		*idx = Q_IDX_HARD;
> +	} else {
> +		*idx = Q_IDX_SOFT;
> +		name += 5;
> +	}
> +	for (q = 0; q < ACC_N; q++) {
> +		if (quota_adm[q].name && !strcmp(quota_adm[q].name, name)) {
> +			*qidx = q;
> +			return false;
> +		}
> +	}
> +
> +	return true;
> +}
> +
>  static int close_xgt_handle(void *_handle)
>  {
>  	xengnttab_close(*(xengnttab_handle **)_handle);
> @@ -2001,6 +2022,61 @@ void read_state_connection(const void *ctx, const void *state)
>  	}
>  }
>  
> +static unsigned int get_quota_size(struct quota *quota, unsigned int *len)
> +{
> +	unsigned int q;
> +	unsigned int n = 0;
> +
> +	for (q = 0; q < ACC_N; q++) {
> +		if (!quota_adm[q].name)
> +			continue;
> +		if (quota[q].val[Q_IDX_HARD] != Q_VAL_DISABLED) {
> +			n++;
> +			*len += strlen(quota_adm[q].name) + 1;
> +		}
> +		if (quota[q].val[Q_IDX_SOFT] != Q_VAL_DISABLED) {
> +			n++;
> +			*len += strlen(quota_adm[q].name) + 5 + 1;

The value 5 here isn't explained. A comment would be nice.

> +		}
> +	}
> +
> +	return n;
> +}
> +
> +static void build_quota_data(struct quota *quota, uint32_t *val, char *name)

I guess we will need a leap of faith to trust that `val` is big enough,
after finding out that it's actually an output of multiple values, and
not an input of a single value.

And `name` seems to also be an output, and this is actually impossible
to guess from the prototype.

> +{
> +	unsigned int q;
> +	unsigned int n = 0;
> +
> +	for (q = 0; q < ACC_N; q++) {
> +		if (!quota_adm[q].name)
> +			continue;
> +		if (quota[q].val[Q_IDX_HARD] != Q_VAL_DISABLED) {
> +			val[n++] = quota[q].val[Q_IDX_HARD];
> +			strcpy(name, quota_adm[q].name);
> +			name += strlen(name) + 1;
> +		}
> +		if (quota[q].val[Q_IDX_SOFT] != Q_VAL_DISABLED) {
> +			val[n++] = quota[q].val[Q_IDX_SOFT];
> +			strcpy(name, "soft-");
> +			strcpy(name + 5, quota_adm[q].name);
> +			name += strlen(name) + 1;
> +		}
> +	}
> +}
> +
> +static void parse_quota_data(const uint32_t *val, const char *name,
> +			     unsigned int n, struct quota *quota)
> +{
> +	unsigned int i, q, idx;
> +
> +	for (i = 0; i < n; i++) {
> +		if (!parse_quota_name(name, &q, &idx))
> +			quota[q].val[idx] = val[i];
> +		name += strlen(name) + 1;

So for `val`, we have a size. But, we don't have a size for `name`, are
we sure that it's safe to keep reading past `NUL` characters ? Is the
size of name available somewhere?

> +	}
> +}
> +
>  static int dump_state_domain(const void *k, void *v, void *arg)
>  {
>  	struct domain *domain = v;
> @@ -2049,6 +2125,53 @@ void read_state_domain(const void *ctx, const void *state, unsigned int version)
>  		domain->features = sd->features;
>  }
>  
> +const char *dump_state_glb_quota(FILE *fp)
> +{
> +	struct xs_state_record_header *head;
> +	struct xs_state_glb_quota *glb;
> +	void *record;
> +	unsigned int n_quota;
> +	unsigned int len = sizeof(*glb);
> +	size_t ret;
> +
> +	n_quota = get_quota_size(quotas, &len);

So, get_quota_size is actually an "add" the size to this variable, and
not "store" the size in this variable. That's not confusing at all.

> +	len += n_quota * sizeof(glb->quota_val[0]);
> +	len = ROUNDUP(len, 3);
> +
> +	record = talloc_size(NULL, len + sizeof(*head));
> +	if (!record)
> +		return "Dump global quota allocation error";
> +
> +	head = record;
> +	head->type = XS_STATE_TYPE_GLB_QUOTA;
> +	head->length = len;
> +
> +	glb = (struct xs_state_glb_quota *)(head + 1);
> +	glb->n_dom_quota = n_quota;
> +	glb->n_glob_quota = 0;

Shouldn't `n_quota` be assigned to `n_glob_quota` instead? We don't have
per-domain quota yet, and only have global quota, right?

> +
> +	build_quota_data(quotas, glb->quota_val,
> +			 (char *)(glb->quota_val + n_quota));
> +
> +	ret = fwrite(record, len + sizeof(*head), 1, fp);
> +
> +	talloc_free(record);
> +
> +	if (ret != 1 || dump_state_align(fp))
> +		return "Dump global quota error";
> +
> +	return NULL;
> +}
> +
> +void read_state_glb_quota(const void *ctx, const void *state)
> +{
> +	const struct xs_state_glb_quota *glb = state;
> +	unsigned int n_quota = glb->n_dom_quota + glb->n_glob_quota;
> +	const char *name = (const char *)(glb->quota_val + n_quota);
> +
> +	parse_quota_data(glb->quota_val, name, n_quota, quotas);
> +}
> +

Thanks,


--
Anthony Perard | Vates XCP-ng Developer

XCP-ng & Xen Orchestra - Vates solutions

web: https://vates.tech