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
