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

[David Woodhouse <dwmw2@xxxxxxxxxxxxx>]

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: smime.p7s
Description: S/MIME cryptographic signature