On Tue, Sep 02, 2025 at 10:20:42AM +0100, Alex Bennée wrote: > References: <[email protected]> > User-Agent: mu4e 1.12.12; emacs 30.1 > Osama Abdelkader <[email protected]> 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 <[email protected]> > > --- > > 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
