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

["Jürgen Groß" <jgross@suse.com>]

[-- Attachment #1.1.1: Type: text/plain, Size: 6391 bytes --]

On 13.03.26 18:08, Anthony PERARD wrote:
> 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.

I can change it, but I'd prefer to use macros for that purpose.

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

Using the macro mentioned above will make it more descriptive.

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

That's what get_quota_size() is calculating.

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

True. What about names?

> 
>> +{
>> +	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?

Yes. get_quota_size() calculated that as well.

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

Would it be better if len is renamed to names_len (both here and the
parameter of get_quota_size())?

> 
>> +	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?

We are applying all global quota values to the domains, so this is fine.

It isn't about where we store the quota (per domain or globally), but
how the quota values are used.

It would be possible to have e.g. "total_memory" or "total_nodes" quota
which would not apply to single domains, but to all of xenstore. Those
would need to be counted by n_glob_quota.

> 
>> +
>> +	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);
>> +}
>> +

Juergen

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3743 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]