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

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

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.

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.

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.

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

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

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.