Re: [PATCH 08/11] tools/libxl: add functions for retrieving and setting xenstore quota

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

On Thu, Mar 05, 2026 at 02:52:05PM +0100, Juergen Gross wrote:
> diff --git a/tools/include/libxl.h b/tools/include/libxl.h
> index bc35e412da..a70d9d347f 100644
> --- a/tools/include/libxl.h
> +++ b/tools/include/libxl.h
> @@ -1537,6 +1537,18 @@ void libxl_mac_copy(libxl_ctx *ctx, libxl_mac *dst, 
> const libxl_mac *src);
>   */
>  #define LIBXL_HAVE_XEN_PLATFORM_PCI_BAR_UC
>  
> +/*
> + * LIBXL_HAVE_XENSTORE_QUOTA
> + *
> + * If this is defined the Xenstore quota related functions
> + * libxl_xsquota_global_get()
> + * libxl_xsquota_global_set()
> + * libxl_xsquota_domain_get()
> + * libxl_xsquota_domain_set()
> + * are available.
> + */
> +#define LIBXL_HAVE_XENSTORE_QUOTA
> +
>  typedef char **libxl_string_list;
>  void libxl_string_list_dispose(libxl_string_list *sl);
>  int libxl_string_list_length(const libxl_string_list *sl);
> @@ -3011,6 +3023,14 @@ static inline int 
> libxl_qemu_monitor_command_0x041200(libxl_ctx *ctx,
>  #define libxl_qemu_monitor_command libxl_qemu_monitor_command_0x041200
>  #endif
>  
> +/* Get/set global and per-domain Xenstore quota. */
> +int libxl_xsquota_global_get(libxl_ctx *ctx, libxl_xs_quota_set *q);

Could you rename the second arg as "q_r" or "q_out" ?

> +int libxl_xsquota_global_set(libxl_ctx *ctx, libxl_xs_quota_set *q);
> +int libxl_xsquota_domain_get(libxl_ctx *ctx, uint32_t domid,
> +                             libxl_xs_quota_set *q);

Same here.

> +int libxl_xsquota_domain_set(libxl_ctx *ctx, uint32_t domid,
> +                             libxl_xs_quota_set *q);

Could we prefix them all with "libxl_xs_quota_" ? I would rather that we
only use "xs_quota" or "xsquota".

> +
>  #include <libxl_event.h>
>  
>  /*
> diff --git a/tools/libs/light/Makefile b/tools/libs/light/Makefile
> index bc60c46558..ca22a40c6c 100644
> --- a/tools/libs/light/Makefile
> +++ b/tools/libs/light/Makefile
> @@ -106,6 +106,7 @@ OBJS-y += libxl_pvcalls.o
>  OBJS-y += libxl_vsnd.o
>  OBJS-y += libxl_vkb.o
>  OBJS-y += libxl_virtio.o
> +OBJS-y += libxl_xsquota.o
>  OBJS-y += libxl_genid.o
>  OBJS-y += _libxl_types.o
>  OBJS-y += libxl_flask.o
> diff --git a/tools/libs/light/libxl_types.idl 
> b/tools/libs/light/libxl_types.idl
> index d64a573ff3..c5ddc40f35 100644
> --- a/tools/libs/light/libxl_types.idl
> +++ b/tools/libs/light/libxl_types.idl
> @@ -574,6 +574,15 @@ libxl_altp2m_mode = Enumeration("altp2m_mode", [
>      (3, "limited"),
>      ], init_val = "LIBXL_ALTP2M_MODE_DISABLED")
>  
> +libxl_xs_quota_item = Struct("xs_quota_item", [
> +    ("name", string),
> +    ("val",  uint32),
> +    ])
> +
> +libxl_xs_quota_set = Struct("xs_quota_set", [

Could you use "_list" as a suffix instead? It's a bit confusing to have
the word "set" as a double meaning, with a _set() function that takes a
set.

> +    ("quota", Array(libxl_xs_quota_item, "num_quota"))
> +    ])
> +
>  libxl_domain_build_info = Struct("domain_build_info",[
>      ("max_vcpus",       integer),
>      ("avail_vcpus",     libxl_bitmap),
> diff --git a/tools/libs/light/libxl_xsquota.c 
> b/tools/libs/light/libxl_xsquota.c
> new file mode 100644
> index 0000000000..b9afa1c914
> --- /dev/null
> +++ b/tools/libs/light/libxl_xsquota.c
> @@ -0,0 +1,102 @@
> +/* SPDX-License-Identifier: LGPL-2.1-only */
> +
> +/* Xenstore quota handling functions. */
> +
> +#include "libxl_internal.h"
> +
> +static int get_quota(libxl_ctx *ctx, unsigned int domid, libxl_xs_quota_set 
> *q,
> +                     bool (func)(struct xs_handle *h, unsigned int domid,
> +                                 char *quota, unsigned int *value))
> +{
> +    char **names;
> +    unsigned int num, i;
> +    int rc = 0;

We don't init `rc` variable in libxl function. Set `rc` to 0 just before
the "out" label.

> +    GC_INIT(ctx);
> +
> +    names = xs_get_quota_names(ctx->xsh, &num);
> +    if (!names) {
> +        /* Xenstore quota support is optional! */
> +        if (errno != ENOSYS)
> +            rc = ERROR_FAIL;
> +        q->num_quota = 0;

It feels wrong to make changes to the output argument on error, if we
can avoid it. And here, I don't see any reason to change `q`.

> +        goto out;
> +    }
> +

Can you call libxl_xs_quota_set_init() first? As you call _dispose()
later.

> +    q->num_quota = num;
> +    q->quota = libxl__calloc(NOGC, num, sizeof(*q->quota));
> +    for (i = 0; i < num; i++) {
> +        q->quota[i].name = libxl__strdup(NOGC, names[i]);
> +        if (!func(ctx->xsh, domid, q->quota[i].name, &q->quota[i].val)) {

Could you store the return value of `func()` in `ok`, and test `ok` in the
if instead?

> +            libxl_xs_quota_set_dispose(q);
> +            rc = ERROR_FAIL;
> +            break;

This can be `goto out` once free(names) is moved to the out label.

> +        }
> +    }
> +
> +    free(names);

Could you do that after the "out" label? And init `names` to NULL.

> +
> + out:
> +    GC_FREE;
> +    return rc;
> +}
> +
> +static int set_quota(libxl_ctx *ctx, unsigned int domid, libxl_xs_quota_set 
> *q,
> +                     bool (func)(struct xs_handle *h, unsigned int domid,
> +                                 char *quota, unsigned int value))
> +{
> +    unsigned int i;
> +    int rc = 0;
> +    GC_INIT(ctx);
> +
> +    for (i = 0; i < q->num_quota; i++) {
> +        if (!func(ctx->xsh, domid, q->quota[i].name, q->quota[i].val)) {
> +            rc = ERROR_FAIL;
> +            break;

It would be better to write `goto out` instead.

> +        }
> +    }
> +
> +    GC_FREE;
> +    return rc;
> +}

Thanks,


--
 | Vates

XCP-ng & Xen Orchestra - Vates solutions

web: https://vates.tech