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

[Anthony PERARD]

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.

> +    if (output) {
>          /*
>           * FIXME: sure we want to support implicit
>           * muxed monitors here?

Thanks,

-- 
Anthony PERARD