Re: [PATCH v2 1/2] hw/xen: Add xs_node_read() helper function

["Philippe Mathieu-Daudé" <philmd@linaro.org>]

On 10/1/25 10:35, Roger Pau Monne wrote:
> From: David Woodhouse <dwmw@amazon.co.uk>
> 
> This returns the full contents of the node, having created the node path
> from the printf-style format string provided in its arguments.
> 
> This will save various callers from having to do so for themselves (and
> from using xs_node_scanf() with the non-portable %ms format string.
> 
> Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
> [remove double newline and constify trace parameters]
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> ---
> Cc: Stefano Stabellini <sstabellini@kernel.org>
> Cc: Anthony PERARD <anthony@xenproject.org>
> Cc: Paul Durrant <paul@xen.org>
> Cc: "Edgar E. Iglesias" <edgar.iglesias@gmail.com>
> Cc: xen-devel@lists.xenproject.org
> ---
>   hw/xen/trace-events             |  1 +
>   hw/xen/xen-bus-helper.c         | 22 ++++++++++++++++++++++
>   include/hw/xen/xen-bus-helper.h |  4 ++++
>   3 files changed, 27 insertions(+)
> 
> diff --git a/hw/xen/trace-events b/hw/xen/trace-events
> index a07fe41c6d3b..461dee7b239f 100644
> --- a/hw/xen/trace-events
> +++ b/hw/xen/trace-events
> @@ -39,6 +39,7 @@ xs_node_create(const char *node) "%s"
>   xs_node_destroy(const char *node) "%s"
>   xs_node_vprintf(char *path, char *value) "%s %s"
>   xs_node_vscanf(char *path, char *value) "%s %s"
> +xs_node_read(const char *path, const char *value) "%s %s"
>   xs_node_watch(char *path) "%s"
>   xs_node_unwatch(char *path) "%s"
>   
> diff --git a/hw/xen/xen-bus-helper.c b/hw/xen/xen-bus-helper.c
> index b2b2cc9c5d5e..0fba7946c55e 100644
> --- a/hw/xen/xen-bus-helper.c
> +++ b/hw/xen/xen-bus-helper.c
> @@ -142,6 +142,28 @@ int xs_node_scanf(struct qemu_xs_handle *h,  xs_transaction_t tid,
>       return rc;
>   }
>   
> +char *xs_node_read(struct qemu_xs_handle *h, xs_transaction_t tid,
> +                   unsigned int *len, Error **errp,
> +                   const char *node_fmt, ...)
> +{
> +    char *path, *value;

Alternatively use g_autofree.

> +    va_list ap;
> +
> +    va_start(ap, node_fmt);
> +    path = g_strdup_vprintf(node_fmt, ap);
> +    va_end(ap);
> +
> +    value = qemu_xen_xs_read(h, tid, path, len);
> +    trace_xs_node_read(path, value);
> +    if (!value) {
> +        error_setg_errno(errp, errno, "failed to read from '%s'", path);
> +    }
> +
> +    g_free(path);
> +
> +    return value;
> +}
> +
>   struct qemu_xs_watch *xs_node_watch(struct qemu_xs_handle *h, const char *node,
>                                       const char *key, xs_watch_fn fn,
>                                       void *opaque, Error **errp)
> diff --git a/include/hw/xen/xen-bus-helper.h b/include/hw/xen/xen-bus-helper.h
> index d8dcc2f0107d..6478d25be5e6 100644
> --- a/include/hw/xen/xen-bus-helper.h
> +++ b/include/hw/xen/xen-bus-helper.h
> @@ -37,6 +37,10 @@ int xs_node_scanf(struct qemu_xs_handle *h,  xs_transaction_t tid,
>                     const char *node, const char *key, Error **errp,
>                     const char *fmt, ...)
>       G_GNUC_SCANF(6, 7);

While I suppose the same comment still applies here ("/* Read from
node/key unless node is empty, in which case read from key */"), it
would be nice to precise the returned value.

> +char *xs_node_read(struct qemu_xs_handle *h, xs_transaction_t tid,
> +                   unsigned int *len, Error **errp,
> +                   const char *node_fmt, ...)
> +    G_GNUC_PRINTF(5, 6);
>   
>   /* Watch node/key unless node is empty, in which case watch key */
>   struct qemu_xs_watch *xs_node_watch(struct qemu_xs_handle *h, const char *node,

Mostly nitpicking, otherwise patch LGTM.