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

["Anthony PERARD" <anthony.perard@xxxxxxxxxx>]

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