[PATCH v2 0/2] xen: error handling and FreeBSD compatibility fixes

[PATCH v2 0/2] xen: error handling and FreeBSD compatibility fixes

[Roger Pau Monne]

Hello,

First patch from David introduces a new helper to fetch xenstore nodes,
while second patch removes the usage of scanf related functions with the
"%ms" format specifier, as it's not supported by the FreeBSD scanf libc
implementation.

Thanks, Roger.

David Woodhouse (1):
  hw/xen: Add xs_node_read() helper function

Roger Pau Monné (1):
  xen: do not use '%ms' scanf specifier

 hw/block/xen-block.c            |  3 ++-
 hw/char/xen_console.c           | 14 ++++++++------
 hw/xen/trace-events             |  1 +
 hw/xen/xen-bus-helper.c         | 22 ++++++++++++++++++++++
 hw/xen/xen-bus.c                | 14 ++++++++++++--
 include/hw/xen/xen-bus-helper.h |  4 ++++
 include/hw/xen/xen-bus.h        |  1 +
 7 files changed, 50 insertions(+), 9 deletions(-)

-- 
2.46.0

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

[Roger Pau Monne]

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

[PATCH v2 2/2] xen: do not use '%ms' scanf specifier

[Roger Pau Monne]

The 'm' parameter used to request auto-allocation of the destination variable
is not supported on FreeBSD, and as such leads to failures to parse.

What's more, the current usage of '%ms' with xs_node_scanf() is pointless, as
it just leads to a double allocation of the same string.  Instead use
xs_node_read() to read the whole xenstore node.

Fixes: a783f8ad4ec9 ('xen: add a mechanism to automatically create XenDevice-s...')
Fixes: 9b7737469080 ('hw/xen: update Xen console to XenDevice model')
Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Changes since v2:
 - New version of xs_node_read().
 - Fix usage of %ms in xen-block.c

Changes since v1:
 - Introduce xs_node_read() helper.
 - Merge with errp fixes.
---
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: Kevin Wolf <kwolf@redhat.com>
Cc: Hanna Reitz <hreitz@redhat.com>
Cc: "Marc-André Lureau" <marcandre.lureau@redhat.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: xen-devel@lists.xenproject.org
Cc: qemu-block@nongnu.org
---
 hw/block/xen-block.c     |  3 ++-
 hw/char/xen_console.c    |  6 ++++--
 hw/xen/xen-bus.c         | 14 ++++++++++++--
 include/hw/xen/xen-bus.h |  1 +
 4 files changed, 19 insertions(+), 5 deletions(-)

diff --git a/hw/block/xen-block.c b/hw/block/xen-block.c
index 306d38927cf4..034a18b70e28 100644
--- a/hw/block/xen-block.c
+++ b/hw/block/xen-block.c
@@ -239,7 +239,8 @@ static void xen_block_connect(XenDevice *xendev, Error **errp)
         return;
     }
 
-    if (xen_device_frontend_scanf(xendev, "protocol", "%ms", &str) != 1) {
+    str = xen_device_frontend_read(xendev, "protocol");
+    if (!str) {
         /* x86 defaults to the 32-bit protocol even for 64-bit guests. */
         if (object_dynamic_cast(OBJECT(qdev_get_machine()), "x86-machine")) {
             protocol = BLKIF_PROTOCOL_X86_32;
diff --git a/hw/char/xen_console.c b/hw/char/xen_console.c
index ef0c2912efa1..989e75fef88f 100644
--- a/hw/char/xen_console.c
+++ b/hw/char/xen_console.c
@@ -550,7 +550,8 @@ static void xen_console_device_create(XenBackendInstance *backend,
         goto fail;
     }
 
-    if (xs_node_scanf(xsh, XBT_NULL, fe, "type", errp, "%ms", &type) != 1) {
+    type = xs_node_read(xsh, XBT_NULL, NULL, errp, "%s/%s", fe, "type");
+    if (!type) {
         error_prepend(errp, "failed to read console device type: ");
         goto fail;
     }
@@ -568,7 +569,8 @@ static void xen_console_device_create(XenBackendInstance *backend,
 
     snprintf(label, sizeof(label), "xencons%ld", number);
 
-    if (xs_node_scanf(xsh, XBT_NULL, fe, "output", NULL, "%ms", &output) == 1) {
+    output = xs_node_read(xsh, XBT_NULL, NULL, errp, "%s/%s", fe, "output");
+    if (output) {
         /*
          * FIXME: sure we want to support implicit
          * muxed monitors here?
diff --git a/hw/xen/xen-bus.c b/hw/xen/xen-bus.c
index adfc4efad035..85b92cded4e2 100644
--- a/hw/xen/xen-bus.c
+++ b/hw/xen/xen-bus.c
@@ -156,8 +156,8 @@ again:
             !strcmp(key[i], "hotplug-status"))
             continue;
 
-        if (xs_node_scanf(xenbus->xsh, tid, path, key[i], NULL, "%ms",
-                          &val) == 1) {
+        val = xs_node_read(xenbus->xsh, tid, NULL, NULL, "%s/%s", path, key[i]);
+        if (val) {
             qdict_put_str(opts, key[i], val);
             free(val);
         }
@@ -650,6 +650,16 @@ int xen_device_frontend_scanf(XenDevice *xendev, const char *key,
     return rc;
 }
 
+char *xen_device_frontend_read(XenDevice *xendev, const char *key)
+{
+    XenBus *xenbus = XEN_BUS(qdev_get_parent_bus(DEVICE(xendev)));
+
+    g_assert(xenbus->xsh);
+
+    return xs_node_read(xenbus->xsh, XBT_NULL, NULL, NULL, "%s/%s",
+                        xendev->frontend_path, key);;
+}
+
 static void xen_device_frontend_set_state(XenDevice *xendev,
                                           enum xenbus_state state,
                                           bool publish)
diff --git a/include/hw/xen/xen-bus.h b/include/hw/xen/xen-bus.h
index 38d40afa3798..2adb2af83919 100644
--- a/include/hw/xen/xen-bus.h
+++ b/include/hw/xen/xen-bus.h
@@ -91,6 +91,7 @@ void xen_device_frontend_printf(XenDevice *xendev, const char *key,
 int xen_device_frontend_scanf(XenDevice *xendev, const char *key,
                               const char *fmt, ...)
     G_GNUC_SCANF(3, 4);
+char *xen_device_frontend_read(XenDevice *xendev, const char *key);
 
 void xen_device_set_max_grant_refs(XenDevice *xendev, unsigned int nr_refs,
                                    Error **errp);
-- 
2.46.0

Re: [PATCH v2 2/2] xen: do not use '%ms' scanf specifier

[David Woodhouse]

[-- Attachment #1: Type: text/plain, Size: 709 bytes --]

On Fri, 2025-01-10 at 10:35 +0100, Roger Pau Monne wrote:
> The 'm' parameter used to request auto-allocation of the destination variable
> is not supported on FreeBSD, and as such leads to failures to parse.
> 
> What's more, the current usage of '%ms' with xs_node_scanf() is pointless, as
> it just leads to a double allocation of the same string.  Instead use
> xs_node_read() to read the whole xenstore node.
> 
> Fixes: a783f8ad4ec9 ('xen: add a mechanism to automatically create XenDevice-s...')
> Fixes: 9b7737469080 ('hw/xen: update Xen console to XenDevice model')
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>

Reviewed-by: David Woodhouse <dwmw@amazon.co.uk>

Thanks.


[-- Attachment #2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 5069 bytes --]

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

[Philippe Mathieu-Daudé]

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.

Re: [PATCH v2 0/2] xen: error handling and FreeBSD compatibility fixes

[David Woodhouse]

[-- Attachment #1: Type: text/plain, Size: 1154 bytes --]

On Fri, 2025-01-10 at 10:35 +0100, Roger Pau Monne wrote:
> Hello,
> 
> First patch from David introduces a new helper to fetch xenstore nodes,
> while second patch removes the usage of scanf related functions with the
> "%ms" format specifier, as it's not supported by the FreeBSD scanf libc
> implementation.
> 
> Thanks, Roger.

Thanks. I've got a handful of non-bugfix cleanups to use the new
xs_node_read in my tree at
https://git.infradead.org/users/dwmw2/qemu.git/shortlog/refs/heads/xs_node_read

David Woodhouse (4):
      hw/xen: Use xs_node_read() from xs_node_vscanf()
      hw/xen: Use xs_node_read() from xen_console_get_name()
      hw/xen: Use xs_node_read() from xen_netdev_get_name()
      hw/xen: Use xs_node_read() from xenstore_read_str() instead of open-coding it

I'm slightly dubious about the last one; xen_pvdev.c didn't previously
use anything from xen-bus-helper.c and even hardcodes zero for
XBT_NULL. And I look at the way it deliberately reallocates the string,
and wonder if we should be doing that in qemu_xen_xs_read() for the
true Xen case. And does it even matter anywhere except Windows?



[-- Attachment #2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 5069 bytes --]

[PATCH 1/4] hw/xen: Use xs_node_read() from xs_node_vscanf()

[David Woodhouse]

From: David Woodhouse <dwmw@amazon.co.uk>

Reduce some duplication.

Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
---
 hw/xen/trace-events     |  1 -
 hw/xen/xen-bus-helper.c | 15 ++++++---------
 2 files changed, 6 insertions(+), 10 deletions(-)

diff --git a/hw/xen/trace-events b/hw/xen/trace-events
index 461dee7b23..b67942d07b 100644
--- a/hw/xen/trace-events
+++ b/hw/xen/trace-events
@@ -38,7 +38,6 @@ xen_device_remove_watch(const char *type, char *name, const char *node, const ch
 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 0fba7946c5..57697799f3 100644
--- a/hw/xen/xen-bus-helper.c
+++ b/hw/xen/xen-bus-helper.c
@@ -105,25 +105,22 @@ int xs_node_vscanf(struct qemu_xs_handle *h,  xs_transaction_t tid,
                    const char *node, const char *key, Error **errp,
                    const char *fmt, va_list ap)
 {
-    char *path, *value;
+    char *value;
     int rc;
 
-    path = (strlen(node) != 0) ? g_strdup_printf("%s/%s", node, key) :
-        g_strdup(key);
-    value = qemu_xen_xs_read(h, tid, path, NULL);
-
-    trace_xs_node_vscanf(path, value);
+    if (node && strlen(node) != 0) {
+        value = xs_node_read(h, tid, NULL, errp, "%s/%s", node, key);
+    } else {
+        value = xs_node_read(h, tid, NULL, errp, "%s", key);
+    }
 
     if (value) {
         rc = vsscanf(value, fmt, ap);
     } else {
-        error_setg_errno(errp, errno, "failed to read from '%s'",
-                         path);
         rc = EOF;
     }
 
     free(value);
-    g_free(path);
 
     return rc;
 }
-- 
2.47.0

[PATCH 2/4] hw/xen: Use xs_node_read() from xen_console_get_name()

[David Woodhouse]

From: David Woodhouse <dwmw@amazon.co.uk>

Now that xs_node_read() can construct a node path, no need to open-code it.

Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
---
 hw/char/xen_console.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/hw/char/xen_console.c b/hw/char/xen_console.c
index 989e75fef8..9338e00473 100644
--- a/hw/char/xen_console.c
+++ b/hw/char/xen_console.c
@@ -367,28 +367,28 @@ static char *xen_console_get_name(XenDevice *xendev, Error **errp)
 
     if (con->dev == -1) {
         XenBus *xenbus = XEN_BUS(qdev_get_parent_bus(DEVICE(xendev)));
-        char fe_path[XENSTORE_ABS_PATH_MAX + 1];
         int idx = (xen_mode == XEN_EMULATE) ? 0 : 1;
+        Error *local_err = NULL;
         char *value;
 
         /* Theoretically we could go up to INT_MAX here but that's overkill */
         while (idx < 100) {
             if (!idx) {
-                snprintf(fe_path, sizeof(fe_path),
-                         "/local/domain/%u/console", xendev->frontend_id);
+                value = xs_node_read(xenbus->xsh, XBT_NULL, NULL, &local_err,
+                                     "/local/domain/%u/console",
+                                     xendev->frontend_id);
             } else {
-                snprintf(fe_path, sizeof(fe_path),
-                         "/local/domain/%u/device/console/%u",
-                         xendev->frontend_id, idx);
+                value = xs_node_read(xenbus->xsh, XBT_NULL, NULL, &local_err,
+                                     "/local/domain/%u/device/console/%u",
+                                     xendev->frontend_id, idx);
             }
-            value = qemu_xen_xs_read(xenbus->xsh, XBT_NULL, fe_path, NULL);
             if (!value) {
                 if (errno == ENOENT) {
                     con->dev = idx;
+                    error_free(local_err);
                     goto found;
                 }
-                error_setg(errp, "cannot read %s: %s", fe_path,
-                           strerror(errno));
+                error_propagate(errp, local_err);
                 return NULL;
             }
             free(value);
-- 
2.47.0

[PATCH 3/4] hw/xen: Use xs_node_read() from xen_netdev_get_name()

[David Woodhouse]

From: David Woodhouse <dwmw@amazon.co.uk>

Now that xs_node_read() can construct a node path, no need to open-code it.

Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
---
 hw/net/xen_nic.c | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/hw/net/xen_nic.c b/hw/net/xen_nic.c
index 97ebd9fa30..5410039490 100644
--- a/hw/net/xen_nic.c
+++ b/hw/net/xen_nic.c
@@ -510,23 +510,22 @@ static char *xen_netdev_get_name(XenDevice *xendev, Error **errp)
 
     if (netdev->dev == -1) {
         XenBus *xenbus = XEN_BUS(qdev_get_parent_bus(DEVICE(xendev)));
-        char fe_path[XENSTORE_ABS_PATH_MAX + 1];
         int idx = (xen_mode == XEN_EMULATE) ? 0 : 1;
+        Error *local_err = NULL;
         char *value;
 
         /* Theoretically we could go up to INT_MAX here but that's overkill */
         while (idx < 100) {
-            snprintf(fe_path, sizeof(fe_path),
-                     "/local/domain/%u/device/vif/%u",
-                     xendev->frontend_id, idx);
-            value = qemu_xen_xs_read(xenbus->xsh, XBT_NULL, fe_path, NULL);
+            value = xs_node_read(xenbus->xsh, XBT_NULL, NULL, &local_err,
+                                 "/local/domain/%u/device/vif/%u",
+                                 xendev->frontend_id, idx);
             if (!value) {
                 if (errno == ENOENT) {
                     netdev->dev = idx;
+                    error_free(local_err);
                     goto found;
                 }
-                error_setg(errp, "cannot read %s: %s", fe_path,
-                           strerror(errno));
+                error_propagate(errp, local_err);
                 return NULL;
             }
             free(value);
-- 
2.47.0

[PATCH 4/4] hw/xen: Use xs_node_read() from xenstore_read_str() instead of open-coding it

[David Woodhouse]

From: David Woodhouse <dwmw@amazon.co.uk>

Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
---
 hw/xen/xen_pvdev.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/hw/xen/xen_pvdev.c b/hw/xen/xen_pvdev.c
index c5ad71e8dc..c9143ba259 100644
--- a/hw/xen/xen_pvdev.c
+++ b/hw/xen/xen_pvdev.c
@@ -22,6 +22,7 @@
 #include "qemu/main-loop.h"
 #include "hw/qdev-core.h"
 #include "hw/xen/xen-legacy-backend.h"
+#include "hw/xen/xen-bus-helper.h"
 #include "hw/xen/xen_pvdev.h"
 
 /* private */
@@ -81,12 +82,9 @@ int xenstore_write_str(const char *base, const char *node, const char *val)
 
 char *xenstore_read_str(const char *base, const char *node)
 {
-    char abspath[XEN_BUFSIZE];
-    unsigned int len;
     char *str, *ret = NULL;
 
-    snprintf(abspath, sizeof(abspath), "%s/%s", base, node);
-    str = qemu_xen_xs_read(xenstore, 0, abspath, &len);
+    str = xs_node_read(xenstore, 0, NULL, NULL, "%s/%s", base, node);
     if (str != NULL) {
         /* move to qemu-allocated memory to make sure
          * callers can safely g_free() stuff. */
-- 
2.47.0

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

[Anthony PERARD]

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

Re: [PATCH v2 0/2] xen: error handling and FreeBSD compatibility fixes

[Roger Pau Monné]

On Fri, Jan 10, 2025 at 10:02:53AM +0000, David Woodhouse wrote:
> On Fri, 2025-01-10 at 10:35 +0100, Roger Pau Monne wrote:
> > Hello,
> > 
> > First patch from David introduces a new helper to fetch xenstore nodes,
> > while second patch removes the usage of scanf related functions with the
> > "%ms" format specifier, as it's not supported by the FreeBSD scanf libc
> > implementation.
> > 
> > Thanks, Roger.
> 
> Thanks. I've got a handful of non-bugfix cleanups to use the new
> xs_node_read in my tree at
> https://git.infradead.org/users/dwmw2/qemu.git/shortlog/refs/heads/xs_node_read
> 
> David Woodhouse (4):
>       hw/xen: Use xs_node_read() from xs_node_vscanf()
>       hw/xen: Use xs_node_read() from xen_console_get_name()
>       hw/xen: Use xs_node_read() from xen_netdev_get_name()
>       hw/xen: Use xs_node_read() from xenstore_read_str() instead of open-coding it

Acked-by: Roger Pau Monné <roger.pau@citrix.com>

> I'm slightly dubious about the last one; xen_pvdev.c didn't previously
> use anything from xen-bus-helper.c and even hardcodes zero for
> XBT_NULL. And I look at the way it deliberately reallocates the string,
> and wonder if we should be doing that in qemu_xen_xs_read() for the
> true Xen case. And does it even matter anywhere except Windows?

I would take the opportunity to use XBT_NULL instead of 0 on
xen_pvdev.c for the transaction.

Thanks, Roger.

Re: [PATCH v2 2/2] xen: do not use '%ms' scanf specifier

[Anthony PERARD]

On Fri, Jan 10, 2025 at 10:35:31AM +0100, Roger Pau Monne wrote:
> diff --git a/hw/char/xen_console.c b/hw/char/xen_console.c
> index ef0c2912efa1..989e75fef88f 100644
> --- a/hw/char/xen_console.c
> +++ b/hw/char/xen_console.c
> @@ -550,7 +550,8 @@ static void xen_console_device_create(XenBackendInstance *backend,
>          goto fail;
>      }
>  
> -    if (xs_node_scanf(xsh, XBT_NULL, fe, "type", errp, "%ms", &type) != 1) {
> +    type = xs_node_read(xsh, XBT_NULL, NULL, errp, "%s/%s", fe, "type");
> +    if (!type) {
>          error_prepend(errp, "failed to read console device type: ");
>          goto fail;
>      }
> @@ -568,7 +569,8 @@ static void xen_console_device_create(XenBackendInstance *backend,
>  
>      snprintf(label, sizeof(label), "xencons%ld", number);
>  
> -    if (xs_node_scanf(xsh, XBT_NULL, fe, "output", NULL, "%ms", &output) == 1) {
> +    output = xs_node_read(xsh, XBT_NULL, NULL, errp, "%s/%s", fe, "output");

This now set `errp` on error, when `output == NULL`. In case `output` is
NULL, we check for `number` instead and may generate an error message
that probably doesn't really make sense.
    "console: No serial device #2 found: failed to read from /frontend_path/output"
And if number == 0, we tried to create a null device, and if that
failed, the error message will just be about the missing xenstore path
as error_setg() will not set `errp` again.

Could you keep ignoring errors from xs_node_read() like it was done with
xs_node_scanf() (I mean pass `NULL` instead of `errp`)? And we will need
another patch to fix the wrong use of `error_prepend()` and use
`error_setg` instead when `serial_hd()` fails.

> +    if (output) {
>          /*
>           * FIXME: sure we want to support implicit
>           * muxed monitors here?

Thanks,

-- 
Anthony PERARD

Re: [PATCH v2 0/2] xen: error handling and FreeBSD compatibility fixes

[David Woodhouse]

[-- Attachment #1: Type: text/plain, Size: 1637 bytes --]

On Wed, 2025-01-15 at 15:34 +0100, Roger Pau Monné wrote:
> On Fri, Jan 10, 2025 at 10:02:53AM +0000, David Woodhouse wrote:
> > On Fri, 2025-01-10 at 10:35 +0100, Roger Pau Monne wrote:
> > > Hello,
> > > 
> > > First patch from David introduces a new helper to fetch xenstore nodes,
> > > while second patch removes the usage of scanf related functions with the
> > > "%ms" format specifier, as it's not supported by the FreeBSD scanf libc
> > > implementation.
> > > 
> > > Thanks, Roger.
> > 
> > Thanks. I've got a handful of non-bugfix cleanups to use the new
> > xs_node_read in my tree at
> > https://git.infradead.org/users/dwmw2/qemu.git/shortlog/refs/heads/xs_node_read
> > 
> > David Woodhouse (4):
> >       hw/xen: Use xs_node_read() from xs_node_vscanf()
> >       hw/xen: Use xs_node_read() from xen_console_get_name()
> >       hw/xen: Use xs_node_read() from xen_netdev_get_name()
> >       hw/xen: Use xs_node_read() from xenstore_read_str() instead of open-coding it
> 
> Acked-by: Roger Pau Monné <roger.pau@citrix.com>

Thanks. I'll add that on each of the four.

> > I'm slightly dubious about the last one; xen_pvdev.c didn't previously
> > use anything from xen-bus-helper.c and even hardcodes zero for
> > XBT_NULL. And I look at the way it deliberately reallocates the string,
> > and wonder if we should be doing that in qemu_xen_xs_read() for the
> > true Xen case. And does it even matter anywhere except Windows?
> 
> I would take the opportunity to use XBT_NULL instead of 0 on
> xen_pvdev.c for the transaction.

Yeah, probably a separate patch.

[-- Attachment #2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 5069 bytes --]

Re: [PATCH 1/4] hw/xen: Use xs_node_read() from xs_node_vscanf()

[Anthony PERARD]

On Fri, Jan 10, 2025 at 10:03:23AM +0000, David Woodhouse wrote:
> From: David Woodhouse <dwmw@amazon.co.uk>
> 
> Reduce some duplication.
> 
> Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>

Reviewed-by: Anthony PERARD <anthony.perard@vates.tech>

Thanks,

-- 
Anthony PERARD

Re: [PATCH 2/4] hw/xen: Use xs_node_read() from xen_console_get_name()

[Anthony PERARD]

On Fri, Jan 10, 2025 at 10:03:24AM +0000, David Woodhouse wrote:
> From: David Woodhouse <dwmw@amazon.co.uk>
> 
> Now that xs_node_read() can construct a node path, no need to open-code it.
> 
> Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>

Reviewed-by: Anthony PERARD <anthony.perard@vates.tech>

Thanks,

-- 
Anthony PERARD

Re: [PATCH 3/4] hw/xen: Use xs_node_read() from xen_netdev_get_name()

[Anthony PERARD]

On Fri, Jan 10, 2025 at 10:03:25AM +0000, David Woodhouse wrote:
> From: David Woodhouse <dwmw@amazon.co.uk>
> 
> Now that xs_node_read() can construct a node path, no need to open-code it.
> 
> Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>

Reviewed-by: Anthony PERARD <anthony.perard@vates.tech>

Thanks,

-- 
Anthony PERARD

Re: [PATCH 4/4] hw/xen: Use xs_node_read() from xenstore_read_str() instead of open-coding it

[Anthony PERARD]

On Fri, Jan 10, 2025 at 10:03:26AM +0000, David Woodhouse wrote:
> From: David Woodhouse <dwmw@amazon.co.uk>
> 
> Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>

Reviewed-by: Anthony PERARD <anthony.perard@vates.tech>

Thanks,

-- 
Anthony PERARD

Re: [PATCH v2 2/2] xen: do not use '%ms' scanf specifier

[David Woodhouse]

[-- Attachment #1: Type: text/plain, Size: 4338 bytes --]

On Wed, 2025-01-15 at 15:36 +0100, Anthony PERARD wrote:
> On Fri, Jan 10, 2025 at 10:35:31AM +0100, Roger Pau Monne wrote:
> > diff --git a/hw/char/xen_console.c b/hw/char/xen_console.c
> > index ef0c2912efa1..989e75fef88f 100644
> > --- a/hw/char/xen_console.c
> > +++ b/hw/char/xen_console.c
> > @@ -550,7 +550,8 @@ static void xen_console_device_create(XenBackendInstance *backend,
> >          goto fail;
> >      }
> >  
> > -    if (xs_node_scanf(xsh, XBT_NULL, fe, "type", errp, "%ms", &type) != 1) {
> > +    type = xs_node_read(xsh, XBT_NULL, NULL, errp, "%s/%s", fe, "type");
> > +    if (!type) {
> >          error_prepend(errp, "failed to read console device type: ");
> >          goto fail;
> >      }
> > @@ -568,7 +569,8 @@ static void xen_console_device_create(XenBackendInstance *backend,
> >  
> >      snprintf(label, sizeof(label), "xencons%ld", number);
> >  
> > -    if (xs_node_scanf(xsh, XBT_NULL, fe, "output", NULL, "%ms", &output) == 1) {
> > +    output = xs_node_read(xsh, XBT_NULL, NULL, errp, "%s/%s", fe, "output");
> 
> This now set `errp` on error, when `output == NULL`. In case `output` is
> NULL, we check for `number` instead and may generate an error message
> that probably doesn't really make sense.
>     "console: No serial device #2 found: failed to read from /frontend_path/output"
> And if number == 0, we tried to create a null device, and if that
> failed, the error message will just be about the missing xenstore path
> as error_setg() will not set `errp` again.
> 
> Could you keep ignoring errors from xs_node_read() like it was done with
> xs_node_scanf() (I mean pass `NULL` instead of `errp`)? And we will need
> another patch to fix the wrong use of `error_prepend()` and use
> `error_setg` instead when `serial_hd()` fails.

Ack. I'll make that s/errp/NULL/ change in the original patch, and then
add something like this on top...

From c6ea20c9055f6c5cdd44a56fd6f7f82d301412d1 Mon Sep 17 00:00:00 2001
From: David Woodhouse <dwmw@amazon.co.uk>
Date: Wed, 15 Jan 2025 15:46:06 +0000
Subject: [PATCH] hw/xen: Fix errp handling in xen_console

Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
---
 hw/char/xen_console.c | 32 ++++++++++++++++++++------------
 1 file changed, 20 insertions(+), 12 deletions(-)

diff --git a/hw/char/xen_console.c b/hw/char/xen_console.c
index 9338e00473..9e7f6da343 100644
--- a/hw/char/xen_console.c
+++ b/hw/char/xen_console.c
@@ -569,7 +569,7 @@ static void xen_console_device_create(XenBackendInstance *backend,
 
     snprintf(label, sizeof(label), "xencons%ld", number);
 
-    output = xs_node_read(xsh, XBT_NULL, NULL, NULL, "%s/%s", fe, "output");
+    output = xs_node_read(xsh, XBT_NULL, NULL, errp, "%s/%s", fe, "output");
     if (output) {
         /*
          * FIXME: sure we want to support implicit

@@ -581,19 +581,27 @@ static void xen_console_device_create(XenBackendInstance *backend,
                        output);
             goto fail;
         }
-    } else if (number) {
-        cd = serial_hd(number);
-        if (!cd) {
-            error_prepend(errp, "console: No serial device #%ld found: ",
-                          number);
-            goto fail;
-        }
+    } else if (errno != ENOENT) {
+        error_prepend(errp, "console: No valid chardev found: ");
+        goto fail;
     } else {
-        /* No 'output' node on primary console: use null. */
-        cd = qemu_chr_new(label, "null", NULL);
-        if (!cd) {
-            error_setg(errp, "console: failed to create null device");
-            goto fail;
+        if (errp) {
+            error_free(*errp);
+        }
+        if (number) {
+            cd = serial_hd(number);
+            if (!cd) {
+                error_setg(errp, "console: No serial device #%ld found: ",
+                           number);
+                goto fail;
+            }
+        } else {
+            /* No 'output' node on primary console: use null. */
+            cd = qemu_chr_new(label, "null", NULL);
+            if (!cd) {
+                error_setg(errp, "console: failed to create null device");
+                goto fail;
+            }
         }
     }
 
-- 
2.47.0



[-- Attachment #2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 5069 bytes --]