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;
+    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);
+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,
-- 
2.46.0

On Fri, Jan 10, 2025 at 10:35:30AM +0100, Roger Pau Monne wrote:
> 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);
> +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);

Could you add a comment about this new functions? It's quite different
from every other function in this header which deal with a xenstore
path. Every other function use "${node}/${key}" (As explain in the
comment above xs_node_vscanf()), but this one uses a printf format in
`node_fmt` (which could probably better be named `path_fmt` instead).

Otherwise, patch looks fine to me.

Cheers,

-- 
Anthony PERARD

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.