Re: [PATCH 01/11] tools/libs/store: add get- and set-quota related functions

[Jürgen Groß <jgross@xxxxxxxx>]

On 13.03.26 15:23, Anthony PERARD wrote:
> On Thu, Mar 05, 2026 at 02:51:58PM +0100, Juergen Gross wrote:
> > diff --git a/tools/include/xenstore.h b/tools/include/xenstore.h
> > index 423422dc50..6b661e5895 100644
> > --- a/tools/include/xenstore.h
> > +++ b/tools/include/xenstore.h
> > @@ -277,6 +277,25 @@ bool xs_get_features_domain(struct xs_handle *h, unsigned 
> > int domid,
> >   bool xs_set_features_domain(struct xs_handle *h, unsigned int domid,
> >                             unsigned int features);
> >   
> > +/* Get names of supported quota. */
> > +char **xs_get_quota_names(struct xs_handle *h, unsigned int *num);
> > +
> > +/* Get the value of one global quota. */
> > +bool xs_get_global_quota(struct xs_handle *h, char *quota,
> > +                        unsigned int *value);
> > +
> > +/* Set the value of one global quota. */
> > +bool xs_set_global_quota(struct xs_handle *h, char *quota,
> > +                        unsigned int value);
> > +
> > +/* Get the value of one domain quota. */
> > +bool xs_get_domain_quota(struct xs_handle *h, unsigned int domid,
> > +                        char *quota, unsigned int *value);
> > +
> > +/* Set the value of one domain quota. */
> > +bool xs_set_domain_quota(struct xs_handle *h, unsigned int domid,
> > +                        char *quota, unsigned int value);
> > +
> Do you think all those new prototype could get a bit more descriptions?
> Which parameter are actually output (and not input), what does it mean
> to return false, do they set errno, is there something to do with the
> return value of xs_get_quota_names?
Oh yes, of course.

> For output arguments, libxl have a convention (well at least a mention
> in the coding style) to suffix argument names with `_r` or `_out`.
Hmm, I don't think I'd like to change style now. This is not libxl after
all.

> For the strings, could we use `const char *` instead of non-const one?
Yes.

> > diff --git a/tools/libs/store/xs.c b/tools/libs/store/xs.c
> > index 8f4b90a3cf..dda37f7526 100644
> > --- a/tools/libs/store/xs.c
> > +++ b/tools/libs/store/xs.c
> > @@ -1456,6 +1456,117 @@ bool xs_set_features_domain(struct xs_handle *h, 
> > unsigned int domid,
> >         return xs_bool(xs_talkv(h, iov, ARRAY_SIZE(iov), NULL));
> >   }
> >   
> > +char **xs_get_quota_names(struct xs_handle *h, unsigned int *num)
> > +{
> > +       struct xsd_sockmsg msg = { .type = XS_GET_QUOTA };
> > +       struct iovec iov[1];
> > +       char **quota;
> > +       char *reply;
> > +       char *c;
> > +       unsigned int i;
> > +
> > +       iov[0].iov_base = &msg;
> > +       iov[0].iov_len  = sizeof(msg);
> > +
> > +       reply = xs_talkv(h, iov, ARRAY_SIZE(iov), NULL);
> > +       if (!reply)
> > +               return NULL;
> > +
> > +       *num = 1;
> > +       for (c = reply; *c; c++)
> > +               if (*c == ' ')
> > +                       (*num)++;
> > +
> > +       quota = malloc(*num * sizeof(char *) + strlen(reply) + 1);
> > +       c = (char *)(quota + *num);
> > +       strcpy(c, reply);
> > +       for (i = 0; i < *num; i++) {
> > +               quota[i] = c;
> > +               c = strchr(c, ' ');
> > +               if (c) {
> If `c` is NULL, it's likely that this is the last iteration of the `for`
> loop. But just in case, should we prevent the code from doing another
> round and prevent `strchr(NULL, ' ')`? (Or just check that `c` is !NULL,
> and let the loop finish set NULL for the remaining slot in `quota`)
Not sure this is really needed. *num is set just a few lines further up
using the same way to count the number of strings. Do we really need to do
consistency checks of intermediate results in such a short function?

> > +                       *c = 0;
> > +                       c++;
> > +               }
> > +       }
> > +
> > +       return quota;
> > +}
> > +
> > +bool xs_get_global_quota(struct xs_handle *h, char *quota,
> > +                        unsigned int *value)
> > +{
> > +       struct xsd_sockmsg msg = { .type = XS_GET_QUOTA };
> > +       struct iovec iov[2];
> > +
> > +       iov[0].iov_base = &msg;
> > +       iov[0].iov_len  = sizeof(msg);
> > +       iov[1].iov_base = quota;
> > +       iov[1].iov_len  = strlen(quota) + 1;
> > +
> > +       return xs_uint(xs_talkv(h, iov, ARRAY_SIZE(iov), NULL), value);
> > +}
> > +
> > +bool xs_set_global_quota(struct xs_handle *h, char *quota,
> > +                        unsigned int value)
> > +{
> > +       struct xsd_sockmsg msg = { .type = XS_SET_QUOTA };
> > +       char val_str[MAX_STRLEN(value)];
> MAX_STRLEN doesn't have a great name, I wounder what is was :-). And
> it's not about a maximum size of payload that could go on xs wire or
> something, it's actually the maximum string size that can take a
> numerical value, when converted to charaters.
Unfortunately the MAX_STRLEN() macro is defined in a public header file.
I could define another macro with a different name doing the same and
use that here, but MAX_STRLEN() would still be there.

What is your preference?


Juergen

Attachment: OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key

Attachment: OpenPGP_signature.asc
Description: OpenPGP digital signature