Communicate the global quota settings via the GLOBAL_QUOTA_DATA
record to the new Xenstore instance.
This avoids to lose global quota settings done via xenstore-control.
In theory it would be possible to drop any quota related command line
parameters in the live update case, but they don't do any harm, as
the record data is applied on top of the command line data.
For soft-quota just prepend "soft-" to the quota name.
Use sub-functions for building and analyzing the quota part of the
migration stream, as they will be reused for per-domain quotas.
Signed-off-by: Juergen Gross <jgross@suse.com>
---
tools/xenstored/domain.c | 123 +++++++++++++++++++++++++++++++++++++++
tools/xenstored/domain.h | 2 +
tools/xenstored/lu.c | 6 ++
3 files changed, 131 insertions(+)
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;
+
+ 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;
+ }
+ }
+
+ return n;
+}
+
+static void build_quota_data(struct quota *quota, uint32_t *val, char *name)
+{
+ 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;
+ }
+}
+
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);
+ 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;
+
+ 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);
+}
+
struct domain_acc {
unsigned int domid;
int nodes;
diff --git a/tools/xenstored/domain.h b/tools/xenstored/domain.h
index a6db358fdc..62ce3b3166 100644
--- a/tools/xenstored/domain.h
+++ b/tools/xenstored/domain.h
@@ -173,10 +173,12 @@ void wrl_apply_debit_trans_commit(struct connection *conn);
const char *dump_state_connections(FILE *fp);
const char *dump_state_domains(FILE *fp);
+const char *dump_state_glb_quota(FILE *fp);
void read_state_connection(const void *ctx, const void *state);
void read_state_domain(const void *ctx, const void *state,
unsigned int version);
+void read_state_glb_quota(const void *ctx, const void *state);
struct hashtable *domain_check_acc_init(void);
void domain_check_acc_add(const struct node *node, struct hashtable *domains);
diff --git a/tools/xenstored/lu.c b/tools/xenstored/lu.c
index fa8395eb1e..eaffdbc69e 100644
--- a/tools/xenstored/lu.c
+++ b/tools/xenstored/lu.c
@@ -192,6 +192,9 @@ void lu_read_state(void)
case XS_STATE_TYPE_DOMAIN:
read_state_domain(ctx, state.buf, version);
break;
+ case XS_STATE_TYPE_GLB_QUOTA:
+ read_state_glb_quota(ctx, state.buf);
+ break;
default:
xprintf("live-update: unknown state record %08x\n",
head.type);
@@ -319,6 +322,9 @@ static const char *lu_dump_state(const void *ctx, struct connection *conn)
}
ret = dump_state_global(fp);
+ if (ret)
+ goto out;
+ ret = dump_state_glb_quota(fp);
if (ret)
goto out;
ret = dump_state_connections(fp);
--
2.53.0On 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
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
On Mon, Mar 16, 2026 at 09:15:47AM +0100, Jürgen Groß wrote:
> 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.
Sounds good.
> > > +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.
Thanks.
> >
> > > + }
> > > + }
> > > +
> > > + 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.
Right. I'm probably just looking at function as been independent of the
rest of the program a bit too much.
> >
> > And `name` seems to also be an output, and this is actually impossible
> > to guess from the prototype.
>
> True. What about names?
`names` would be better here, indeed.
> >
> > > +{
> > > + 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())?
Do you mean adding a new variable `names_len`? And having get_quota_size
set it to 0 before calculating the size?
I would be ok also with adding `sizeof(*glb)` to len after calling
get_quota_size(), but still need to have get_quota_size() start counting
from 0.
> >
> > > + 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.
Right, I think I figured that out only after seen the patch creating a
different record for per-domain quota. (And seen that these two field
were not changed in later patches). But thank for the explanation, and
the possible way that n_glob_quota could be used.
Cheers,
--
Anthony Perard | Vates XCP-ng Developer
XCP-ng & Xen Orchestra - Vates solutions
web: https://vates.tech
On 16.03.26 09:15, Jürgen Groß wrote:
> 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?
I have chosen names_buf instead, making it more clear that this is an
output parameter for multiple 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())?
In the end I have chosen to add a comment above get_quota_size() and
rename "len" to "rec_len" in dump_state_glb_quota().
Juergen
On Wed, Mar 18, 2026 at 01:16:30PM +0100, Juergen Gross wrote:
> On 16.03.26 09:15, Jürgen Groß wrote:
> > On 13.03.26 18:08, Anthony PERARD wrote:
> > > On Thu, Mar 05, 2026 at 02:52:01PM +0100, Juergen Gross wrote:
> > > > +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?
>
> I have chosen names_buf instead, making it more clear that this is an
> output parameter for multiple names.
Sounds good.
> > > > +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())?
>
> In the end I have chosen to add a comment above get_quota_size() and
> rename "len" to "rec_len" in dump_state_glb_quota().
It would still be counter-intuitive if get_quota_size() returns different
values depending on the initial value of the second parameter. A comment
won't help. So I would still propose to add "sizeof(*glb)" to `len` or
`rec_len` after calling get_quota_size.
Cheers,
--
Anthony Perard | Vates XCP-ng Developer
XCP-ng & Xen Orchestra - Vates solutions
web: https://vates.tech
On 19.03.26 17:15, Anthony PERARD wrote:
> On Wed, Mar 18, 2026 at 01:16:30PM +0100, Juergen Gross wrote:
>> On 16.03.26 09:15, Jürgen Groß wrote:
>>> On 13.03.26 18:08, Anthony PERARD wrote:
>>>> On Thu, Mar 05, 2026 at 02:52:01PM +0100, Juergen Gross wrote:
>>>>> +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?
>>
>> I have chosen names_buf instead, making it more clear that this is an
>> output parameter for multiple names.
>
> Sounds good.
>
>>>>> +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())?
>>
>> In the end I have chosen to add a comment above get_quota_size() and
>> rename "len" to "rec_len" in dump_state_glb_quota().
>
> It would still be counter-intuitive if get_quota_size() returns different
> values depending on the initial value of the second parameter. A comment
> won't help. So I would still propose to add "sizeof(*glb)" to `len` or
> `rec_len` after calling get_quota_size.
Okay, will change that.
Juergen
© 2016 - 2026 Red Hat, Inc.