Re: [PATCH] hw/arm/raspi4b: replace fprintf with error_report

["Alex Bennée" <alex.bennee@linaro.org>]

References: <20250901213607.69603-1-osama.abdelkader@gmail.com>
User-Agent: mu4e 1.12.12; emacs 30.1
Osama Abdelkader <osama.abdelkader@gmail.com> writes:

> Replace direct fprintf(stderr, .%80.) with QEMU's
> error_report() API,

Not sure what happened with the encoding there, it seems to be non-utf-8.

> which ensures consistent formatting and integrates with QEMU's
> logging infrastructure.
>
> Signed-off-by: Osama Abdelkader <osama.abdelkader@gmail.com>
> ---
>  hw/arm/raspi4b.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/hw/arm/raspi4b.c b/hw/arm/raspi4b.c
> index 20082d5266..4a5f0eb91e 100644
> --- a/hw/arm/raspi4b.c
> +++ b/hw/arm/raspi4b.c
> @@ -47,7 +47,7 @@ static int raspi_add_memory_node(void *fdt, hwaddr mem_base, hwaddr mem_len)
>      scells = qemu_fdt_getprop_cell(fdt, "/", "#size-cells",
>                                     NULL, &error_fatal);
>      if (acells == 0 || scells == 0) {
> -        fprintf(stderr, "dtb file invalid (#address-cells or #size-cells 0)\n");
> +        error_report("dtb file invalid (#address-cells or #size-cells 0)");

This change is fine as far as it goes but I wonder if it is an
error_report or a warn_report. The reason being:

>          ret = -1;

we set -1 to ret here but it is ignored by the call:

    if (info->ram_size > UPPER_RAM_BASE) {
        raspi_add_memory_node(fdt, UPPER_RAM_BASE, ram_size - UPPER_RAM_BASE);
    }

which implies this isn't a fatal error, but the user should certainly be
warned they won't get all the memory they were expecting.

While these single line patches are a good way to get comfortable with
the review and submission process I would also encourage you to look at
the call chain.

In this case we get here from arm_load_dtb:

    if (binfo->modify_dtb) {
        binfo->modify_dtb(binfo, fdt);
    }

And you can see further up that function we do the same test:

    acells = qemu_fdt_getprop_cell(fdt, "/", "#address-cells",
                                   NULL, &error_fatal);
    scells = qemu_fdt_getprop_cell(fdt, "/", "#size-cells",
                                   NULL, &error_fatal);
    if (acells == 0 || scells == 0) {
        fprintf(stderr, "dtb file invalid (#address-cells or #size-cells 0)\n");
        goto fail;
    }

which fails and ultimately causes QEMU to exit as it can't continue. In
that case I don't think we could ever hit this condition. As it would be
a programming failure I think we could replace the whole if leg with:

  /* validated by arm_load_dtb */
  g_assert(acells && scells);

and maybe remove the ignored (and redundant) return value from
raspi_add_memory_node().

>      } else {
>          qemu_fdt_add_subnode(fdt, nodename);

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro
Date: Tue, 02 Sep 2025 10:20:42 +0100