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 <[email protected]> Date: Wed, 15 Jan 2025 15:46:06 +0000 Subject: [PATCH] hw/xen: Fix errp handling in xen_console Signed-off-by: David Woodhouse <[email protected]> --- 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
smime.p7s
Description: S/MIME cryptographic signature
