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

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

[Roger Pau Monne]

Hello,

First patch fixes some error handling paths that incorrectly used
error_prepend() in the Xen console driver.  Second patch removes usage
of the 'm' character in scanf directives, as it's not supported on
FreeBSD (see usages of "%ms").

Thanks, Roger.

Roger Pau Monné (2):
  xen/console: fix error handling in xen_console_device_create()
  xen: do not use '%ms' scanf specifier

 hw/char/xen_console.c | 15 +++++++++++----
 hw/xen/xen-bus.c      |  7 +++++--
 2 files changed, 16 insertions(+), 6 deletions(-)
-- 
2.46.0

[PATCH 1/2] xen/console: fix error handling in xen_console_device_create()

[Roger Pau Monne]

The usage of error_prepend() in some of the error contexts of
xen_console_device_create() is incorrect, as `errp` hasn't been initialized.
This leads to the following segmentation fault on error paths resulting from
xenstore reads:

Program terminated with signal SIGSEGV, Segmentation fault.
Address not mapped to object.
    fmt=0x15c4dfeade42 "failed to read console device type: ", ap=0x15cd0165ab50)
    at ../qemu-xen-dir-remote/util/error.c:142
142         g_string_append(newmsg, (*errp)->msg);
[...]
(gdb) bt
    (errp=0x15cd0165ae10, fmt=0x15c4dfeade42 "failed to read console device type: ", ap=0x15cd0165ab50) at ../qemu-xen-dir-remote/util/error.c:142
    (errp=0x15cd0165ae10, fmt=0x15c4dfeade42 "failed to read console device type: ")
    at ../qemu-xen-dir-remote/util/error.c:152
    (backend=0x43944de00660, opts=0x43944c929000, errp=0x15cd0165ae10)
    at ../qemu-xen-dir-remote/hw/char/xen_console.c:555

Replace usages of error_prepend() with error_setg() where appropriate.

Fixes: 9b7737469080 ('hw/xen: update Xen console to XenDevice model')
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: "Marc-André Lureau" <marcandre.lureau@redhat.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: xen-devel@lists.xenproject.org
---
 hw/char/xen_console.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/char/xen_console.c b/hw/char/xen_console.c
index ef0c2912efa1..af706c7ef440 100644
--- a/hw/char/xen_console.c
+++ b/hw/char/xen_console.c
@@ -551,7 +551,7 @@ static void xen_console_device_create(XenBackendInstance *backend,
     }
 
     if (xs_node_scanf(xsh, XBT_NULL, fe, "type", errp, "%ms", &type) != 1) {
-        error_prepend(errp, "failed to read console device type: ");
+        error_setg(errp, "failed to read console device type: ");
         goto fail;
     }
 
@@ -582,7 +582,7 @@ static void xen_console_device_create(XenBackendInstance *backend,
     } else if (number) {
         cd = serial_hd(number);
         if (!cd) {
-            error_prepend(errp, "console: No serial device #%ld found: ",
+            error_setg(errp, "console: No serial device #%ld found: ",
                           number);
             goto fail;
         }
-- 
2.46.0

Re: [PATCH 1/2] xen/console: fix error handling in xen_console_device_create()

[Anthony PERARD]

On Tue, Jan 07, 2025 at 10:31:39AM +0100, Roger Pau Monne wrote:
> The usage of error_prepend() in some of the error contexts of
> xen_console_device_create() is incorrect, as `errp` hasn't been initialized.
> This leads to the following segmentation fault on error paths resulting from
> xenstore reads:
> 
> Program terminated with signal SIGSEGV, Segmentation fault.
> Address not mapped to object.
>     fmt=0x15c4dfeade42 "failed to read console device type: ", ap=0x15cd0165ab50)
>     at ../qemu-xen-dir-remote/util/error.c:142
> 142         g_string_append(newmsg, (*errp)->msg);
> [...]
> (gdb) bt
>     (errp=0x15cd0165ae10, fmt=0x15c4dfeade42 "failed to read console device type: ", ap=0x15cd0165ab50) at ../qemu-xen-dir-remote/util/error.c:142
>     (errp=0x15cd0165ae10, fmt=0x15c4dfeade42 "failed to read console device type: ")
>     at ../qemu-xen-dir-remote/util/error.c:152
>     (backend=0x43944de00660, opts=0x43944c929000, errp=0x15cd0165ae10)
>     at ../qemu-xen-dir-remote/hw/char/xen_console.c:555
> 
> Replace usages of error_prepend() with error_setg() where appropriate.
> 
> Fixes: 9b7737469080 ('hw/xen: update Xen console to XenDevice model')
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> ---
>  hw/char/xen_console.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/char/xen_console.c b/hw/char/xen_console.c
> index ef0c2912efa1..af706c7ef440 100644
> --- a/hw/char/xen_console.c
> +++ b/hw/char/xen_console.c
> @@ -551,7 +551,7 @@ static void xen_console_device_create(XenBackendInstance *backend,
>      }
>  
>      if (xs_node_scanf(xsh, XBT_NULL, fe, "type", errp, "%ms", &type) != 1) {
> -        error_prepend(errp, "failed to read console device type: ");
> +        error_setg(errp, "failed to read console device type: ");

According to error_setg() doc, *errp must be NULL but xs_node_scanf may
set it. Looking at the implementation, error_setg() seems to simply
discard this new error message if *errp is already set.

Currently, when there's an I/O error, we get something like:
    failed to read console device type: failed to read from /xenstore/path: doesn't exist
and when the format scan failed:
    SEGV

With this patch, when there's an I/O error, I think we get something
like:
    failed to read from /xenstore/path: doesn't exist
and when the format scan failed:
    failed to read console device type: 


So I think we'll want to distiguish between IO error from
xs_node_scanf() and format error, first one returns EOF (like vsscanf)
and second one returns a value >= 0 but we expect exactly 1.


>          goto fail;
>      }
>  
> @@ -582,7 +582,7 @@ static void xen_console_device_create(XenBackendInstance *backend,
>      } else if (number) {
>          cd = serial_hd(number);
>          if (!cd) {
> -            error_prepend(errp, "console: No serial device #%ld found: ",
> +            error_setg(errp, "console: No serial device #%ld found: ",
>                            number);

This change looks correct, ableit we could remove ":  " from the end of
the string since they shouldn't be anything after it.


Cheers,

-- 
Anthony PERARD

Re: [PATCH 1/2] xen/console: fix error handling in xen_console_device_create()

[Roger Pau Monné]

On Thu, Jan 09, 2025 at 11:13:45AM +0100, Anthony PERARD wrote:
> On Tue, Jan 07, 2025 at 10:31:39AM +0100, Roger Pau Monne wrote:
> > The usage of error_prepend() in some of the error contexts of
> > xen_console_device_create() is incorrect, as `errp` hasn't been initialized.
> > This leads to the following segmentation fault on error paths resulting from
> > xenstore reads:
> > 
> > Program terminated with signal SIGSEGV, Segmentation fault.
> > Address not mapped to object.
> >     fmt=0x15c4dfeade42 "failed to read console device type: ", ap=0x15cd0165ab50)
> >     at ../qemu-xen-dir-remote/util/error.c:142
> > 142         g_string_append(newmsg, (*errp)->msg);
> > [...]
> > (gdb) bt
> >     (errp=0x15cd0165ae10, fmt=0x15c4dfeade42 "failed to read console device type: ", ap=0x15cd0165ab50) at ../qemu-xen-dir-remote/util/error.c:142
> >     (errp=0x15cd0165ae10, fmt=0x15c4dfeade42 "failed to read console device type: ")
> >     at ../qemu-xen-dir-remote/util/error.c:152
> >     (backend=0x43944de00660, opts=0x43944c929000, errp=0x15cd0165ae10)
> >     at ../qemu-xen-dir-remote/hw/char/xen_console.c:555
> > 
> > Replace usages of error_prepend() with error_setg() where appropriate.
> > 
> > Fixes: 9b7737469080 ('hw/xen: update Xen console to XenDevice model')
> > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> > ---
> >  hw/char/xen_console.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/hw/char/xen_console.c b/hw/char/xen_console.c
> > index ef0c2912efa1..af706c7ef440 100644
> > --- a/hw/char/xen_console.c
> > +++ b/hw/char/xen_console.c
> > @@ -551,7 +551,7 @@ static void xen_console_device_create(XenBackendInstance *backend,
> >      }
> >  
> >      if (xs_node_scanf(xsh, XBT_NULL, fe, "type", errp, "%ms", &type) != 1) {
> > -        error_prepend(errp, "failed to read console device type: ");
> > +        error_setg(errp, "failed to read console device type: ");
> 
> According to error_setg() doc, *errp must be NULL but xs_node_scanf may
> set it. Looking at the implementation, error_setg() seems to simply
> discard this new error message if *errp is already set.
> 
> Currently, when there's an I/O error, we get something like:
>     failed to read console device type: failed to read from /xenstore/path: doesn't exist
> and when the format scan failed:
>     SEGV
> 
> With this patch, when there's an I/O error, I think we get something
> like:
>     failed to read from /xenstore/path: doesn't exist
> and when the format scan failed:
>     failed to read console device type: 
> 
> 
> So I think we'll want to distiguish between IO error from
> xs_node_scanf() and format error, first one returns EOF (like vsscanf)
> and second one returns a value >= 0 but we expect exactly 1.

The call to xs_node_scanf() will go away in the next patch replaced by
qemu_xen_xs_read(), at which point errp will never be initialized.

I can change the order of the patches if that makes it easier.

> 
> >          goto fail;
> >      }
> >  
> > @@ -582,7 +582,7 @@ static void xen_console_device_create(XenBackendInstance *backend,
> >      } else if (number) {
> >          cd = serial_hd(number);
> >          if (!cd) {
> > -            error_prepend(errp, "console: No serial device #%ld found: ",
> > +            error_setg(errp, "console: No serial device #%ld found: ",
> >                            number);
> 
> This change looks correct, ableit we could remove ":  " from the end of
> the string since they shouldn't be anything after it.

Thanks, Roger.

[PATCH 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
qemu_xen_xs_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>
---
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: "Marc-André Lureau" <marcandre.lureau@redhat.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: xen-devel@lists.xenproject.org
---
 hw/char/xen_console.c | 11 +++++++++--
 hw/xen/xen-bus.c      |  7 +++++--
 2 files changed, 14 insertions(+), 4 deletions(-)

diff --git a/hw/char/xen_console.c b/hw/char/xen_console.c
index af706c7ef440..18afd214c2f6 100644
--- a/hw/char/xen_console.c
+++ b/hw/char/xen_console.c
@@ -531,6 +531,7 @@ static void xen_console_device_create(XenBackendInstance *backend,
     const char *name = xen_backend_get_name(backend);
     unsigned long number;
     char *fe = NULL, *type = NULL, *output = NULL;
+    const char *node_path;
     char label[32];
     XenDevice *xendev = NULL;
     XenConsole *con;
@@ -550,7 +551,10 @@ static void xen_console_device_create(XenBackendInstance *backend,
         goto fail;
     }
 
-    if (xs_node_scanf(xsh, XBT_NULL, fe, "type", errp, "%ms", &type) != 1) {
+    node_path = g_strdup_printf("%s/type", fe);
+    type = qemu_xen_xs_read(xsh, XBT_NULL, node_path, NULL);
+    g_free(node_path);
+    if (!type) {
         error_setg(errp, "failed to read console device type: ");
         goto fail;
     }
@@ -568,7 +572,10 @@ 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) {
+    node_path = g_strdup_printf("%s/output", fe);
+    output = qemu_xen_xs_read(xsh, XBT_NULL, node_path, NULL);
+    g_free(node_path);
+    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..9be807649e77 100644
--- a/hw/xen/xen-bus.c
+++ b/hw/xen/xen-bus.c
@@ -142,6 +142,7 @@ again:
 
     opts = qdict_new();
     for (i = 0; i < n; i++) {
+        const char *node_path;
         char *val;
 
         /*
@@ -156,8 +157,10 @@ again:
             !strcmp(key[i], "hotplug-status"))
             continue;
 
-        if (xs_node_scanf(xenbus->xsh, tid, path, key[i], NULL, "%ms",
-                          &val) == 1) {
+        node_path = g_strdup_printf("%s/%s", path, key[i]);
+        val = qemu_xen_xs_read(xenbus->xsh, tid, node_path, NULL);
+        g_free(node_path);
+        if (val) {
             qdict_put_str(opts, key[i], val);
             free(val);
         }
-- 
2.46.0

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

[Anthony PERARD]

On Tue, Jan 07, 2025 at 10:31:40AM +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
> qemu_xen_xs_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>
> ---
>  hw/char/xen_console.c | 11 +++++++++--
>  hw/xen/xen-bus.c      |  7 +++++--
>  2 files changed, 14 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/char/xen_console.c b/hw/char/xen_console.c
> index af706c7ef440..18afd214c2f6 100644
> --- a/hw/char/xen_console.c
> +++ b/hw/char/xen_console.c
> @@ -531,6 +531,7 @@ static void xen_console_device_create(XenBackendInstance *backend,
>      const char *name = xen_backend_get_name(backend);
>      unsigned long number;
>      char *fe = NULL, *type = NULL, *output = NULL;
> +    const char *node_path;

Is "const" correct when we are changing to which string `node_path` is
pointing at? Also, why "const"? Also, compiler complains that we can't
free a "const something*".

>      char label[32];
>      XenDevice *xendev = NULL;
>      XenConsole *con;
> @@ -550,7 +551,10 @@ static void xen_console_device_create(XenBackendInstance *backend,
>          goto fail;
>      }
>  
> -    if (xs_node_scanf(xsh, XBT_NULL, fe, "type", errp, "%ms", &type) != 1) {
> +    node_path = g_strdup_printf("%s/type", fe);
> +    type = qemu_xen_xs_read(xsh, XBT_NULL, node_path, NULL);
> +    g_free(node_path);

I feel like we want "xs_node_read()" which would be similair to
xs_node_vscanf() but would simply return the result of
qemu_xen_xs_read(). This would avoid the need format of the node path in
several place in the code. But it's OK like that as well.

Cheers,

-- 
Anthony PERARD

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

[David Woodhouse]

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

On Thu, 2025-01-09 at 11:59 +0100, Anthony PERARD wrote:
> 
> >       char label[32];
> >       XenDevice *xendev = NULL;
> >       XenConsole *con;
> > @@ -550,7 +551,10 @@ static void xen_console_device_create(XenBackendInstance *backend,
> >           goto fail;
> >       }
> >   
> > -    if (xs_node_scanf(xsh, XBT_NULL, fe, "type", errp, "%ms", &type) != 1) {
> > +    node_path = g_strdup_printf("%s/type", fe);
> > +    type = qemu_xen_xs_read(xsh, XBT_NULL, node_path, NULL);
> > +    g_free(node_path);
> 
> I feel like we want "xs_node_read()" which would be similair to
> xs_node_vscanf() but would simply return the result of
> qemu_xen_xs_read(). This would avoid the need format of the node path in
> several place in the code. But it's OK like that as well.

If you look at the other callers of qemu_xen_xs_read(), it looks like
the majority of them create the path with snprintf and then pass it in.
Or with g_strdup_printf(), pass it in, then free it afterwards.

So perhaps qemu_xen_xs_read() should be a printf-style function too,
with its last arg(s) being the node name.

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

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

[Roger Pau Monné]

On Thu, Jan 09, 2025 at 11:25:13AM +0000, David Woodhouse wrote:
> On Thu, 2025-01-09 at 11:59 +0100, Anthony PERARD wrote:
> > 
> > >       char label[32];
> > >       XenDevice *xendev = NULL;
> > >       XenConsole *con;
> > > @@ -550,7 +551,10 @@ static void xen_console_device_create(XenBackendInstance *backend,
> > >           goto fail;
> > >       }
> > >   
> > > -    if (xs_node_scanf(xsh, XBT_NULL, fe, "type", errp, "%ms", &type) != 1) {
> > > +    node_path = g_strdup_printf("%s/type", fe);
> > > +    type = qemu_xen_xs_read(xsh, XBT_NULL, node_path, NULL);
> > > +    g_free(node_path);
> > 
> > I feel like we want "xs_node_read()" which would be similair to
> > xs_node_vscanf() but would simply return the result of
> > qemu_xen_xs_read(). This would avoid the need format of the node path in
> > several place in the code. But it's OK like that as well.
> 
> If you look at the other callers of qemu_xen_xs_read(), it looks like
> the majority of them create the path with snprintf and then pass it in.
> Or with g_strdup_printf(), pass it in, then free it afterwards.
> 
> So perhaps qemu_xen_xs_read() should be a printf-style function too,
> with its last arg(s) being the node name.

I just went with Anthony suggestion and introduced xs_node_read(), as
I didn't want to play with qemu_xen_xs_read().  Not that I think the
suggestion is not valid, just seemed more work than what I wanted to
do right now.

Thanks, Roger.

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

[David Woodhouse]

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

On Thu, 2025-01-09 at 17:55 +0100, Roger Pau Monné wrote:
> On Thu, Jan 09, 2025 at 11:25:13AM +0000, David Woodhouse wrote:
> > On Thu, 2025-01-09 at 11:59 +0100, Anthony PERARD wrote:
> > > 
> > > >       char label[32];
> > > >       XenDevice *xendev = NULL;
> > > >       XenConsole *con;
> > > > @@ -550,7 +551,10 @@ static void xen_console_device_create(XenBackendInstance *backend,
> > > >           goto fail;
> > > >       }
> > > >   
> > > > -    if (xs_node_scanf(xsh, XBT_NULL, fe, "type", errp, "%ms", &type) != 1) {
> > > > +    node_path = g_strdup_printf("%s/type", fe);
> > > > +    type = qemu_xen_xs_read(xsh, XBT_NULL, node_path, NULL);
> > > > +    g_free(node_path);
> > > 
> > > I feel like we want "xs_node_read()" which would be similair to
> > > xs_node_vscanf() but would simply return the result of
> > > qemu_xen_xs_read(). This would avoid the need format of the node path in
> > > several place in the code. But it's OK like that as well.
> > 
> > If you look at the other callers of qemu_xen_xs_read(), it looks like
> > the majority of them create the path with snprintf and then pass it in.
> > Or with g_strdup_printf(), pass it in, then free it afterwards.
> > 
> > So perhaps qemu_xen_xs_read() should be a printf-style function too,
> > with its last arg(s) being the node name.
> 
> I just went with Anthony suggestion and introduced xs_node_read(), as
> I didn't want to play with qemu_xen_xs_read().  Not that I think the
> suggestion is not valid, just seemed more work than what I wanted to
> do right now.

Makes sense. Something like this¹?

char *xs_node_read(struct qemu_xs_handle *h, xs_transaction_t tid,
                   Error **errp, unsigned int *len,
                   const char *node_fmt, ...)
    G_GNUC_PRINTF(5, 6);

There's a %ms in hw/xen/xen-block.c too, btw. Did you catch that one?


¹ https://git.infradead.org/?p=users/dwmw2/qemu.git;a=commitdiff;h=percentms;hp=bc6afa1c711da5b4f37c9685a812c77b114d84cb

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

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

[Philippe Mathieu-Daudé]

On 10/1/25 09:08, David Woodhouse wrote:
> On Thu, 2025-01-09 at 17:55 +0100, Roger Pau Monné wrote:
>> On Thu, Jan 09, 2025 at 11:25:13AM +0000, David Woodhouse wrote:
>>> On Thu, 2025-01-09 at 11:59 +0100, Anthony PERARD wrote:
>>>>
>>>>>        char label[32];
>>>>>        XenDevice *xendev = NULL;
>>>>>        XenConsole *con;
>>>>> @@ -550,7 +551,10 @@ static void xen_console_device_create(XenBackendInstance *backend,
>>>>>            goto fail;
>>>>>        }
>>>>>    
>>>>> -    if (xs_node_scanf(xsh, XBT_NULL, fe, "type", errp, "%ms", &type) != 1) {
>>>>> +    node_path = g_strdup_printf("%s/type", fe);
>>>>> +    type = qemu_xen_xs_read(xsh, XBT_NULL, node_path, NULL);
>>>>> +    g_free(node_path);
>>>>
>>>> I feel like we want "xs_node_read()" which would be similair to
>>>> xs_node_vscanf() but would simply return the result of
>>>> qemu_xen_xs_read(). This would avoid the need format of the node path in
>>>> several place in the code. But it's OK like that as well.
>>>
>>> If you look at the other callers of qemu_xen_xs_read(), it looks like
>>> the majority of them create the path with snprintf and then pass it in.
>>> Or with g_strdup_printf(), pass it in, then free it afterwards.
>>>
>>> So perhaps qemu_xen_xs_read() should be a printf-style function too,
>>> with its last arg(s) being the node name.
>>
>> I just went with Anthony suggestion and introduced xs_node_read(), as
>> I didn't want to play with qemu_xen_xs_read().  Not that I think the
>> suggestion is not valid, just seemed more work than what I wanted to
>> do right now.
> 
> Makes sense. Something like this¹?
> 
> char *xs_node_read(struct qemu_xs_handle *h, xs_transaction_t tid,
>                     Error **errp, unsigned int *len,

Maybe switch len <-> errp arg order.

>                     const char *node_fmt, ...)
>      G_GNUC_PRINTF(5, 6);
> 
> There's a %ms in hw/xen/xen-block.c too, btw. Did you catch that one?
> 
> 
> ¹ https://git.infradead.org/?p=users/dwmw2/qemu.git;a=commitdiff;h=percentms;hp=bc6afa1c711da5b4f37c9685a812c77b114d84cb

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

[David Woodhouse]

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

On Fri, 2025-01-10 at 09:16 +0100, Philippe Mathieu-Daudé wrote:
> On 10/1/25 09:08, David Woodhouse wrote:
> > On Thu, 2025-01-09 at 17:55 +0100, Roger Pau Monné wrote:
> > > On Thu, Jan 09, 2025 at 11:25:13AM +0000, David Woodhouse wrote:
> > > > On Thu, 2025-01-09 at 11:59 +0100, Anthony PERARD wrote:
> > > > > 
> > > > > >         char label[32];
> > > > > >         XenDevice *xendev = NULL;
> > > > > >         XenConsole *con;
> > > > > > @@ -550,7 +551,10 @@ static void xen_console_device_create(XenBackendInstance *backend,
> > > > > >             goto fail;
> > > > > >         }
> > > > > >     
> > > > > > -    if (xs_node_scanf(xsh, XBT_NULL, fe, "type", errp, "%ms", &type) != 1) {
> > > > > > +    node_path = g_strdup_printf("%s/type", fe);
> > > > > > +    type = qemu_xen_xs_read(xsh, XBT_NULL, node_path, NULL);
> > > > > > +    g_free(node_path);
> > > > > 
> > > > > I feel like we want "xs_node_read()" which would be similair to
> > > > > xs_node_vscanf() but would simply return the result of
> > > > > qemu_xen_xs_read(). This would avoid the need format of the node path in
> > > > > several place in the code. But it's OK like that as well.
> > > > 
> > > > If you look at the other callers of qemu_xen_xs_read(), it looks like
> > > > the majority of them create the path with snprintf and then pass it in.
> > > > Or with g_strdup_printf(), pass it in, then free it afterwards.
> > > > 
> > > > So perhaps qemu_xen_xs_read() should be a printf-style function too,
> > > > with its last arg(s) being the node name.
> > > 
> > > I just went with Anthony suggestion and introduced xs_node_read(), as
> > > I didn't want to play with qemu_xen_xs_read().  Not that I think the
> > > suggestion is not valid, just seemed more work than what I wanted to
> > > do right now.
> > 
> > Makes sense. Something like this¹?
> > 
> > char *xs_node_read(struct qemu_xs_handle *h, xs_transaction_t tid,
> >                      Error **errp, unsigned int *len,
> 
> Maybe switch len <-> errp arg order.

Ack. Changed in my 'percentms' branch in case Roger wants to use it.

We can then clean up a few users of snprintf+qemu_xen_xs_read() to use
xs_node_read() too.

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

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

[Roger Pau Monné]

On Thu, Jan 09, 2025 at 11:59:48AM +0100, Anthony PERARD wrote:
> On Tue, Jan 07, 2025 at 10:31:40AM +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
> > qemu_xen_xs_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>
> > ---
> >  hw/char/xen_console.c | 11 +++++++++--
> >  hw/xen/xen-bus.c      |  7 +++++--
> >  2 files changed, 14 insertions(+), 4 deletions(-)
> > 
> > diff --git a/hw/char/xen_console.c b/hw/char/xen_console.c
> > index af706c7ef440..18afd214c2f6 100644
> > --- a/hw/char/xen_console.c
> > +++ b/hw/char/xen_console.c
> > @@ -531,6 +531,7 @@ static void xen_console_device_create(XenBackendInstance *backend,
> >      const char *name = xen_backend_get_name(backend);
> >      unsigned long number;
> >      char *fe = NULL, *type = NULL, *output = NULL;
> > +    const char *node_path;
> 
> Is "const" correct when we are changing to which string `node_path` is
> pointing at? Also, why "const"? Also, compiler complains that we can't
> free a "const something*".

It's my understanding that the proposed const signals that the pointer
value can change, but the contents of the pointer cannot (iow: the
string cannot be modified).

My build seem to be fine, but I see that g_free() doesn't seem to take
a const pointer.  Will adjust.

> >      char label[32];
> >      XenDevice *xendev = NULL;
> >      XenConsole *con;
> > @@ -550,7 +551,10 @@ static void xen_console_device_create(XenBackendInstance *backend,
> >          goto fail;
> >      }
> >  
> > -    if (xs_node_scanf(xsh, XBT_NULL, fe, "type", errp, "%ms", &type) != 1) {
> > +    node_path = g_strdup_printf("%s/type", fe);
> > +    type = qemu_xen_xs_read(xsh, XBT_NULL, node_path, NULL);
> > +    g_free(node_path);
> 
> I feel like we want "xs_node_read()" which would be similair to
> xs_node_vscanf() but would simply return the result of
> qemu_xen_xs_read(). This would avoid the need format of the node path in
> several place in the code. But it's OK like that as well.

I was about to introduce such, but didn't end up doing.  I can do.

Thanks, Roger.