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

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

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@xxxxxxxxxx>
> > ---
> >  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.