Re: [PATCH 07/11] tools/xenstored: implement the GET/SET_QUOTA commands

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

On Thu, Mar 05, 2026 at 02:52:04PM +0100, Juergen Gross wrote:
> diff --git a/tools/xenstored/core.c b/tools/xenstored/core.c
> index 8a06b35808..e283d47184 100644
> --- a/tools/xenstored/core.c
> +++ b/tools/xenstored/core.c
> @@ -2034,6 +2034,10 @@ static struct {
>           { "GET_FEATURE",   do_get_feature,  XS_FLAG_PRIV },
>       [XS_SET_FEATURE]       =
>           { "SET_FEATURE",   do_set_feature,  XS_FLAG_PRIV },
> +     [XS_GET_QUOTA]         =
> +         { "GET_QUOTA",     do_get_quota,    XS_FLAG_PRIV },
> +     [XS_SET_QUOTA]         =
> +         { "SET_QUOTA",     do_set_quota,    XS_FLAG_PRIV },
>  };
>  
>  static const char *sockmsg_string(enum xsd_sockmsg_type type)
> diff --git a/tools/xenstored/domain.c b/tools/xenstored/domain.c
> index 8e52351695..c0bc8a3eb7 100644
> --- a/tools/xenstored/domain.c
> +++ b/tools/xenstored/domain.c
> @@ -1363,6 +1363,112 @@ static bool parse_quota_name(const char *name, 
> unsigned int *qidx,
>       return true;
>  }
>  
> +int do_get_quota(const void *ctx, struct connection *conn,
> +              struct buffered_data *in)
> +{
> +     const char *vec[2];
> +     unsigned int n_pars;
> +     unsigned int domid;
> +     unsigned int q;
> +     unsigned int idx;
> +     char *resp;
> +     const char *name;
> +     const struct quota *quota;
> +     const struct domain *domain;
> +
> +     n_pars = get_strings(in, vec, ARRAY_SIZE(vec));
> +
> +     if (n_pars > 2)
> +             return EINVAL;
> +
> +     if (n_pars == 0) {
> +             resp = talloc_asprintf(ctx, "%s", "");

This could be written with talloc_strdup() instead, since there's no
formatting involve.

> +             if (!resp)
> +                     return ENOMEM;
> +             for (q = 0; q < ACC_N; q++) {
> +                     if (!quota_adm[q].name)
> +                             continue;
> +                     if (quotas[q].val[Q_IDX_HARD] != Q_VAL_DISABLED) {

Having set internally a value of Q_VAL_DISABLED, does it mean the named
quota is unsupported?

> +                             resp = talloc_asprintf_append(resp, "%s%s",
> +                                     *resp ? " " : "", quota_adm[q].name);
> +                             if (!resp)
> +                                     return ENOMEM;
> +                     }
> +                     if (quotas[q].val[Q_IDX_SOFT] != Q_VAL_DISABLED) {
> +                             resp = talloc_asprintf_append(resp, "%ssoft-%s",
> +                                     *resp ? " " : "", quota_adm[q].name);
> +                             if (!resp)
> +                                     return ENOMEM;
> +                     }
> +             }
> +     } else {
> +             if (n_pars == 1) {
> +                     quota = quotas;
> +                     name = vec[0];
> +             } else {
> +                     domid = atoi(vec[0]);

Shall we check that vec[0] actually contain a plausible domid? (An
integer between 0..65535). Right now, this accept everything, and would
return 0 if there's not a single digit.

> +                     domain = find_or_alloc_existing_domain(domid);
> +                     if (!domain)
> +                             return ENOENT;
> +                     quota = domain->acc;
> +                     name = vec[1];
> +             }
> +
> +             if (parse_quota_name(name, &q, &idx))
> +                     return EINVAL;
> +
> +             resp = talloc_asprintf(ctx, "%u", quota[q].val[idx]);

Why do we return 4294967295 for disabled quota check when the spec say
to return "0" when a quota check is disabled? That is for quota names
that are supposed to be not supported (if we ask "GET_QUOTA" first).

> +             if (!resp)
> +                     return ENOMEM;
> +     }
> +
> +     send_reply(conn, XS_GET_QUOTA, resp, strlen(resp) + 1);
> +
> +     return 0;
> +}
> +
> +int do_set_quota(const void *ctx, struct connection *conn,
> +              struct buffered_data *in)
> +{
> +     const char *vec[3];
> +     unsigned int n_pars;
> +     unsigned int domid;
> +     unsigned int q;
> +     unsigned int idx;
> +     const char *name;
> +     unsigned int val;
> +     struct quota *quota;
> +     struct domain *domain;
> +
> +     n_pars = get_strings(in, vec, ARRAY_SIZE(vec));
> +
> +     if (n_pars < 2 || n_pars > 3)
> +             return EINVAL;
> +
> +     if (n_pars == 2) {
> +             quota = quotas;
> +             name = vec[0];
> +             val = atoi(vec[1]);

We should check that vec[1] is a valid quota value, and also not an
internal value. Otherwise, we can just have "-1" on the wire, and have
unexpected changes for example. Only "0" is documented as a quota been
disabled, "-1" or "4294967295" isn't.

> +     } else {
> +             domid = atoi(vec[0]);
> +             domain = find_or_alloc_existing_domain(domid);
> +             if (!domain)
> +                     return ENOENT;
> +             quota = domain->acc;
> +             name = vec[1];
> +             val = atoi(vec[2]);
> +     }
> +
> +     if (parse_quota_name(name, &q, &idx))
> +             return EINVAL;
> +
> +     quota[q].val[idx] = val;
> +
> +     send_ack(conn, XS_SET_QUOTA);
> +
> +     return 0;
> +}
> +
>  static int close_xgt_handle(void *_handle)
>  {
>       xengnttab_close(*(xengnttab_handle **)_handle);
> diff --git a/tools/xenstored/domain.h b/tools/xenstored/domain.h
> index 62ce3b3166..6a06b0d1af 100644
> --- a/tools/xenstored/domain.h
> +++ b/tools/xenstored/domain.h
> @@ -93,6 +93,14 @@ int do_get_feature(const void *ctx, struct connection 
> *conn,
>  int do_set_feature(const void *ctx, struct connection *conn,
>                  struct buffered_data *in);
>  
> +/* Get quota names or value */

This could say "implement GET_QUOTA" or something instead. But a
comment here isn't going to give much value for internal functions.

> +int do_get_quota(const void *ctx, struct connection *conn,
> +              struct buffered_data *in);
> +
> +/* Set quota value */
> +int do_set_quota(const void *ctx, struct connection *conn,
> +              struct buffered_data *in);
> +
>  void domain_early_init(void);
>  void domain_init(int evtfd);
>  void init_domains(bool live_update);


Thanks,


--
Anthony Perard | Vates XCP-ng Developer

XCP-ng & Xen Orchestra - Vates solutions

web: https://vates.tech