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

[Osama Abdelkader <osama.abdelkader@gmail.com>]

On Tue, Sep 02, 2025 at 10:20:42AM +0100, Alex Bennée wrote:
> 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

Thanks Alex, exactly. I just checked that and sent another patch
since it's not only fprintf replace anymore.

Thank you,
Osama