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

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

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