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

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

[Osama Abdelkader]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset=y, Size: 928 bytes --]

Replace direct fprintf(stderr, …) with QEMU's error_report() API,
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)");
         ret = -1;
     } else {
         qemu_fdt_add_subnode(fdt, nodename);
-- 
2.43.0

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

[Alex Bennée]

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

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

[Osama Abdelkader]

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