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

[Juergen Gross <jgross@xxxxxxxx>]

On 16.03.26 16:08, Anthony PERARD wrote:
> 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.
Right.

> > +               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?
Yes. Right now all hard quota are supported and only one soft quota
is supported.

> > +                               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.
I have followed the pattern used in other places where a domid is expected.

In the end nothing will really break.

Any integer not being a domid will result in ENOENT, while the case of not
a digit is a bug in privileged software (domids can be specified by dom0
only).

> > +                       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).
parse_quota_name() should have returned true in this case, so EINVAL should
be returned.

Will fix that.

> > +               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.
Right, I'll check for val != Q_VAL_DISABLED.

> > +       } 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.
If nobody objects I'll drop the comment.

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


Juergen

Attachment: OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key

Attachment: OpenPGP_signature.asc
Description: OpenPGP digital signature