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
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
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
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.
© 2016 - 2025 Red Hat, Inc.