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

[David Woodhouse]

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

smime.p7s
Description: S/MIME cryptographic signature