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

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

On Mon, Mar 16, 2026 at 04:27:43PM +0100, Juergen Gross wrote:
> On 16.03.26 16:08, Anthony PERARD wrote:
> > On Thu, Mar 05, 2026 at 02:52:04PM +0100, Juergen Gross wrote:
> > > +         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.

On the daemon, no, not really.

> 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).

It would be a bug, indeed, but xenstored can help telling exactly where
there's a bug, instead of ignoring it and carry-on. Well, just return
EINVAL when something other than a number is found. You do that for
quota, why not for domid as well?

That can also help when the daemon is replace by a different
implementation that actually do the checks. (well it would help finding
bug in the client earlier)


> > > + } 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.

Sounds good.

Cheers,


--
Anthony Perard | Vates XCP-ng Developer

XCP-ng & Xen Orchestra - Vates solutions

web: https://vates.tech