| Message ID | 20210819171547.2879725-3-philmd@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | hw/usb: Always expect 'dma' link property to be set to simplify | expand |
+Markus On Thu, Aug 19, 2021 at 07:15:46PM +0200, Philippe Mathieu-Daudé wrote: > Do not ignore eventual error if we failed at setting the 'host' > property of the TYPE_XHCI model. > > Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> > --- > hw/usb/hcd-xhci-pci.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/hw/usb/hcd-xhci-pci.c b/hw/usb/hcd-xhci-pci.c > index e934b1a5b1f..71f6629ccde 100644 > --- a/hw/usb/hcd-xhci-pci.c > +++ b/hw/usb/hcd-xhci-pci.c > @@ -115,7 +115,7 @@ static void usb_xhci_pci_realize(struct PCIDevice *dev, Error **errp) > dev->config[PCI_CACHE_LINE_SIZE] = 0x10; > dev->config[0x60] = 0x30; /* release number */ > > - object_property_set_link(OBJECT(&s->xhci), "host", OBJECT(s), NULL); > + object_property_set_link(OBJECT(&s->xhci), "host", OBJECT(s), &error_fatal); If this fails, it's due to programmer error, isn't? Shouldn't we use &error_abort on that case? > s->xhci.intr_update = xhci_pci_intr_update; > s->xhci.intr_raise = xhci_pci_intr_raise; > if (!qdev_realize(DEVICE(&s->xhci), NULL, errp)) { > -- > 2.31.1 >
Eduardo Habkost <ehabkost@redhat.com> writes: > +Markus > > On Thu, Aug 19, 2021 at 07:15:46PM +0200, Philippe Mathieu-Daudé wrote: >> Do not ignore eventual error if we failed at setting the 'host' >> property of the TYPE_XHCI model. >> >> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> >> --- >> hw/usb/hcd-xhci-pci.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/hw/usb/hcd-xhci-pci.c b/hw/usb/hcd-xhci-pci.c >> index e934b1a5b1f..71f6629ccde 100644 >> --- a/hw/usb/hcd-xhci-pci.c >> +++ b/hw/usb/hcd-xhci-pci.c >> @@ -115,7 +115,7 @@ static void usb_xhci_pci_realize(struct PCIDevice *dev, Error **errp) >> dev->config[PCI_CACHE_LINE_SIZE] = 0x10; >> dev->config[0x60] = 0x30; /* release number */ >> >> - object_property_set_link(OBJECT(&s->xhci), "host", OBJECT(s), NULL); >> + object_property_set_link(OBJECT(&s->xhci), "host", OBJECT(s), &error_fatal); > > If this fails, it's due to programmer error, isn't? Shouldn't we > use &error_abort on that case? I think so. In functions with an Error **errp parameter, use of &error_fatal is almost always wrong. >> s->xhci.intr_update = xhci_pci_intr_update; >> s->xhci.intr_raise = xhci_pci_intr_raise; >> if (!qdev_realize(DEVICE(&s->xhci), NULL, errp)) { >> -- >> 2.31.1 >>
On 8/24/21 10:13 AM, Markus Armbruster wrote: > Eduardo Habkost <ehabkost@redhat.com> writes: > >> +Markus >> >> On Thu, Aug 19, 2021 at 07:15:46PM +0200, Philippe Mathieu-Daudé wrote: >>> Do not ignore eventual error if we failed at setting the 'host' >>> property of the TYPE_XHCI model. >>> >>> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> >>> --- >>> hw/usb/hcd-xhci-pci.c | 2 +- >>> 1 file changed, 1 insertion(+), 1 deletion(-) >>> >>> diff --git a/hw/usb/hcd-xhci-pci.c b/hw/usb/hcd-xhci-pci.c >>> index e934b1a5b1f..71f6629ccde 100644 >>> --- a/hw/usb/hcd-xhci-pci.c >>> +++ b/hw/usb/hcd-xhci-pci.c >>> @@ -115,7 +115,7 @@ static void usb_xhci_pci_realize(struct PCIDevice *dev, Error **errp) >>> dev->config[PCI_CACHE_LINE_SIZE] = 0x10; >>> dev->config[0x60] = 0x30; /* release number */ >>> >>> - object_property_set_link(OBJECT(&s->xhci), "host", OBJECT(s), NULL); >>> + object_property_set_link(OBJECT(&s->xhci), "host", OBJECT(s), &error_fatal); >> >> If this fails, it's due to programmer error, isn't? Shouldn't we >> use &error_abort on that case? > > I think so. > > In functions with an Error **errp parameter, use of &error_fatal is > almost always wrong. OK, thanks!
On 8/24/21 10:35 AM, Philippe Mathieu-Daudé wrote: > On 8/24/21 10:13 AM, Markus Armbruster wrote: >> Eduardo Habkost <ehabkost@redhat.com> writes: >> >>> +Markus >>> >>> On Thu, Aug 19, 2021 at 07:15:46PM +0200, Philippe Mathieu-Daudé wrote: >>>> Do not ignore eventual error if we failed at setting the 'host' >>>> property of the TYPE_XHCI model. >>>> >>>> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> >>>> --- >>>> hw/usb/hcd-xhci-pci.c | 2 +- >>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>> >>>> diff --git a/hw/usb/hcd-xhci-pci.c b/hw/usb/hcd-xhci-pci.c >>>> index e934b1a5b1f..71f6629ccde 100644 >>>> --- a/hw/usb/hcd-xhci-pci.c >>>> +++ b/hw/usb/hcd-xhci-pci.c >>>> @@ -115,7 +115,7 @@ static void usb_xhci_pci_realize(struct PCIDevice *dev, Error **errp) >>>> dev->config[PCI_CACHE_LINE_SIZE] = 0x10; >>>> dev->config[0x60] = 0x30; /* release number */ >>>> >>>> - object_property_set_link(OBJECT(&s->xhci), "host", OBJECT(s), NULL); >>>> + object_property_set_link(OBJECT(&s->xhci), "host", OBJECT(s), &error_fatal); >>> >>> If this fails, it's due to programmer error, isn't? Shouldn't we >>> use &error_abort on that case? >> >> I think so. >> >> In functions with an Error **errp parameter, use of &error_fatal is >> almost always wrong. Having used 'abort' in the subject, no clue why I used &error_fatal then...
On Tue, 24 Aug 2021 at 09:14, Markus Armbruster <armbru@redhat.com> wrote: > In functions with an Error **errp parameter, use of &error_fatal is > almost always wrong. What are the cases where it is not wrong? My guess is "in board code and other places where the error handling would have been 'print a message and call exit()' anyway". -- PMM
Peter Maydell <peter.maydell@linaro.org> writes: > On Tue, 24 Aug 2021 at 09:14, Markus Armbruster <armbru@redhat.com> wrote: >> In functions with an Error **errp parameter, use of &error_fatal is >> almost always wrong. > > What are the cases where it is not wrong? I can't think of a use that isn't wrong. Doesn't mean no such use could exist. Most rules have exceptions... > My guess is "in board > code and other places where the error handling would have been > 'print a message and call exit()' anyway". When you know that all callers handle errors like &error_fatal does, use of &error_fatal doesn't produce wrong behavior. It's still kind of wrong, because relying on such a non-local argument without a genuine need is.
On Tue, 24 Aug 2021 at 13:05, Markus Armbruster <armbru@redhat.com> wrote: > When you know that all callers handle errors like &error_fatal does, use > of &error_fatal doesn't produce wrong behavior. It's still kind of > wrong, because relying on such a non-local argument without a genuine > need is. Not using error_fatal results in quite a bit of extra boilerplate for all those extra explicit "check for failure, print the error message and exit" points. If the MachineState init function took an Error** that might be a mild encouragement to "pass an Error upward rather than exiting"; but it doesn't. Right now we have nearly a thousand instances of error_fatal in the codebase, incidentally. -- PMM
On Tue, Aug 24, 2021 at 01:16:40PM +0100, Peter Maydell wrote: > On Tue, 24 Aug 2021 at 13:05, Markus Armbruster <armbru@redhat.com> wrote: > > When you know that all callers handle errors like &error_fatal does, use > > of &error_fatal doesn't produce wrong behavior. It's still kind of > > wrong, because relying on such a non-local argument without a genuine > > need is. > > Not using error_fatal results in quite a bit of extra boilerplate > for all those extra explicit "check for failure, print the error > message and exit" points. I don't get it. There's no need for extra boilerplate if the caller is using &error_fatal. > If the MachineState init function took > an Error** that might be a mild encouragement to "pass an Error > upward rather than exiting"; but it doesn't. Agreed. > > Right now we have nearly a thousand instances of error_fatal > in the codebase, incidentally. It looks like 73 of them are in functions that take an Error** argument. Coccinelle patch for finding them: @@ typedef Error; type T; identifier errp, fn; @@ T fn(..., Error **errp) { ... *&error_fatal ... } Coccinelle output: diff -u -p ./hw/pci-host/pnv_phb3.c /tmp/nothing/hw/pci-host/pnv_phb3.c --- ./hw/pci-host/pnv_phb3.c +++ /tmp/nothing/hw/pci-host/pnv_phb3.c @@ -1054,7 +1054,6 @@ static void pnv_phb3_realize(DeviceState /* Add a single Root port */ qdev_prop_set_uint8(DEVICE(&phb->root), "chassis", phb->chip_id); qdev_prop_set_uint16(DEVICE(&phb->root), "slot", phb->phb_id); - qdev_realize(DEVICE(&phb->root), BUS(pci->bus), &error_fatal); } void pnv_phb3_update_regions(PnvPHB3 *phb) diff -u -p ./hw/pci-host/q35.c /tmp/nothing/hw/pci-host/q35.c --- ./hw/pci-host/q35.c +++ /tmp/nothing/hw/pci-host/q35.c @@ -67,7 +67,6 @@ static void q35_host_realize(DeviceState PC_MACHINE(qdev_get_machine())->bus = pci->bus; pci->bypass_iommu = PC_MACHINE(qdev_get_machine())->default_bus_bypass_iommu; - qdev_realize(DEVICE(&s->mch), BUS(pci->bus), &error_fatal); } static const char *q35_host_root_bus_path(PCIHostState *host_bridge, diff -u -p ./hw/pci-host/mv64361.c /tmp/nothing/hw/pci-host/mv64361.c --- ./hw/pci-host/mv64361.c +++ /tmp/nothing/hw/pci-host/mv64361.c @@ -875,7 +875,6 @@ static void mv64361_realize(DeviceState TYPE_MV64361_PCI); DeviceState *pci = DEVICE(&s->pci[i]); qdev_prop_set_uint8(pci, "index", i); - sysbus_realize_and_unref(SYS_BUS_DEVICE(pci), &error_fatal); } sysbus_init_irq(SYS_BUS_DEVICE(dev), &s->cpu_irq); qdev_init_gpio_in_named(dev, mv64361_gpp_irq, "gpp", 32); diff -u -p ./hw/pci-host/designware.c /tmp/nothing/hw/pci-host/designware.c --- ./hw/pci-host/designware.c +++ /tmp/nothing/hw/pci-host/designware.c @@ -707,7 +707,6 @@ static void designware_pcie_host_realize "pcie-bus-address-space"); pci_setup_iommu(pci->bus, designware_pcie_host_set_iommu, s); - qdev_realize(DEVICE(&s->root), BUS(pci->bus), &error_fatal); } static const VMStateDescription vmstate_designware_pcie_host = { diff -u -p ./hw/pci-host/raven.c /tmp/nothing/hw/pci-host/raven.c --- ./hw/pci-host/raven.c +++ /tmp/nothing/hw/pci-host/raven.c @@ -335,7 +335,6 @@ static void raven_realize(PCIDevice *d, d->config[0x34] = 0x00; // capabilities_pointer memory_region_init_rom_nomigrate(&s->bios, OBJECT(s), "bios", BIOS_SIZE, - &error_fatal); memory_region_add_subregion(get_system_memory(), (uint32_t)(-BIOS_SIZE), &s->bios); if (s->bios_name) { diff -u -p ./hw/pci-host/gpex.c /tmp/nothing/hw/pci-host/gpex.c --- ./hw/pci-host/gpex.c +++ /tmp/nothing/hw/pci-host/gpex.c @@ -138,7 +138,6 @@ static void gpex_host_realize(DeviceStat &s->io_ioport, 0, 4, TYPE_PCIE_BUS); pci_bus_set_route_irq_fn(pci->bus, gpex_route_intx_pin_to_irq); - qdev_realize(DEVICE(&s->gpex_root), BUS(pci->bus), &error_fatal); } static const char *gpex_host_root_bus_path(PCIHostState *host_bridge, diff -u -p ./hw/pci-host/xilinx-pcie.c /tmp/nothing/hw/pci-host/xilinx-pcie.c --- ./hw/pci-host/xilinx-pcie.c +++ /tmp/nothing/hw/pci-host/xilinx-pcie.c @@ -137,7 +137,6 @@ static void xilinx_pcie_host_realize(Dev pci_swizzle_map_irq_fn, s, &s->mmio, &s->io, 0, 4, TYPE_PCIE_BUS); - qdev_realize(DEVICE(&s->root), BUS(pci->bus), &error_fatal); } static const char *xilinx_pcie_host_root_bus_path(PCIHostState *host_bridge, diff -u -p ./hw/ppc/spapr_irq.c /tmp/nothing/hw/ppc/spapr_irq.c --- ./hw/ppc/spapr_irq.c +++ /tmp/nothing/hw/ppc/spapr_irq.c @@ -337,7 +337,6 @@ void spapr_irq_init(SpaprMachineState *s qdev_prop_set_uint32(dev, "nr-ends", nr_servers << 3); object_property_set_link(OBJECT(dev), "xive-fabric", OBJECT(spapr), &error_abort); - sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal); spapr->xive = SPAPR_XIVE(dev); diff -u -p ./hw/ppc/pnv.c /tmp/nothing/hw/ppc/pnv.c --- ./hw/ppc/pnv.c +++ /tmp/nothing/hw/ppc/pnv.c @@ -1144,7 +1144,6 @@ static void pnv_chip_power8_realize(Devi /* Processor Service Interface (PSI) Host Bridge */ object_property_set_int(OBJECT(&chip8->psi), "bar", PNV_PSIHB_BASE(chip), - &error_fatal); object_property_set_link(OBJECT(&chip8->psi), ICS_PROP_XICS, OBJECT(chip8->xics), &error_abort); if (!qdev_realize(DEVICE(&chip8->psi), NULL, errp)) { @@ -1587,7 +1586,6 @@ static void pnv_chip_power10_realize(Dev /* Processor Service Interface (PSI) Host Bridge */ object_property_set_int(OBJECT(&chip10->psi), "bar", - PNV10_PSIHB_BASE(chip), &error_fatal); if (!qdev_realize(DEVICE(&chip10->psi), NULL, errp)) { return; } diff -u -p ./hw/intc/exynos4210_gic.c /tmp/nothing/hw/intc/exynos4210_gic.c --- ./hw/intc/exynos4210_gic.c +++ /tmp/nothing/hw/intc/exynos4210_gic.c @@ -301,7 +301,6 @@ static void exynos4210_gic_realize(Devic qdev_prop_set_uint32(s->gic, "num-cpu", s->num_cpu); qdev_prop_set_uint32(s->gic, "num-irq", EXYNOS4210_GIC_NIRQ); gicbusdev = SYS_BUS_DEVICE(s->gic); - sysbus_realize_and_unref(gicbusdev, &error_fatal); /* Pass through outbound IRQ lines from the GIC */ sysbus_pass_irq(sbd, gicbusdev); diff -u -p ./hw/intc/spapr_xive.c /tmp/nothing/hw/intc/spapr_xive.c --- ./hw/intc/spapr_xive.c +++ /tmp/nothing/hw/intc/spapr_xive.c @@ -311,7 +311,6 @@ static void spapr_xive_realize(DeviceSta * Initialize the internal sources, for IPIs and virtual devices. */ object_property_set_int(OBJECT(xsrc), "nr-irqs", xive->nr_irqs, - &error_fatal); object_property_set_link(OBJECT(xsrc), "xive", OBJECT(xive), &error_abort); if (!qdev_realize(DEVICE(xsrc), NULL, errp)) { return; diff -u -p ./hw/intc/pnv_xive.c /tmp/nothing/hw/intc/pnv_xive.c --- ./hw/intc/pnv_xive.c +++ /tmp/nothing/hw/intc/pnv_xive.c @@ -1833,7 +1833,6 @@ static void pnv_xive_realize(DeviceState * to limit accesses to resources not provisioned. */ object_property_set_int(OBJECT(xsrc), "nr-irqs", PNV_XIVE_NR_IRQS, - &error_fatal); object_property_set_link(OBJECT(xsrc), "xive", OBJECT(xive), &error_abort); if (!qdev_realize(DEVICE(xsrc), NULL, errp)) { return; diff -u -p ./hw/riscv/shakti_c.c /tmp/nothing/hw/riscv/shakti_c.c --- ./hw/riscv/shakti_c.c +++ /tmp/nothing/hw/riscv/shakti_c.c @@ -137,7 +137,6 @@ static void shakti_c_soc_state_realize(D /* ROM */ memory_region_init_rom(&sss->rom, OBJECT(dev), "riscv.shakti.c.rom", - shakti_c_memmap[SHAKTI_C_ROM].size, &error_fatal); memory_region_add_subregion(system_memory, shakti_c_memmap[SHAKTI_C_ROM].base, &sss->rom); } diff -u -p ./hw/riscv/sifive_e.c /tmp/nothing/hw/riscv/sifive_e.c --- ./hw/riscv/sifive_e.c +++ /tmp/nothing/hw/riscv/sifive_e.c @@ -192,7 +192,6 @@ static void sifive_e_soc_realize(DeviceS /* Mask ROM */ memory_region_init_rom(&s->mask_rom, OBJECT(dev), "riscv.sifive.e.mrom", - memmap[SIFIVE_E_DEV_MROM].size, &error_fatal); memory_region_add_subregion(sys_mem, memmap[SIFIVE_E_DEV_MROM].base, &s->mask_rom); diff -u -p ./hw/display/bochs-display.c /tmp/nothing/hw/display/bochs-display.c --- ./hw/display/bochs-display.c +++ /tmp/nothing/hw/display/bochs-display.c @@ -280,7 +280,6 @@ static void bochs_display_realize(PCIDev s->con = graphic_console_init(DEVICE(dev), 0, &bochs_display_gfx_ops, s); memory_region_init_ram(&s->vram, obj, "bochs-display-vram", s->vgamem, - &error_fatal); memory_region_init_io(&s->vbe, obj, &bochs_display_vbe_ops, s, "bochs dispi interface", PCI_VGA_BOCHS_SIZE); memory_region_init_io(&s->qext, obj, &bochs_display_qext_ops, s, diff -u -p ./hw/display/tcx.c /tmp/nothing/hw/display/tcx.c --- ./hw/display/tcx.c +++ /tmp/nothing/hw/display/tcx.c @@ -818,7 +818,6 @@ static void tcx_realizefn(DeviceState *d char *fcode_filename; memory_region_init_ram_nomigrate(&s->vram_mem, OBJECT(s), "tcx.vram", - s->vram_size * (1 + 4 + 4), &error_fatal); vmstate_register_ram_global(&s->vram_mem); memory_region_set_log(&s->vram_mem, true, DIRTY_MEMORY_VGA); vram_base = memory_region_get_ram_ptr(&s->vram_mem); diff -u -p ./hw/display/next-fb.c /tmp/nothing/hw/display/next-fb.c --- ./hw/display/next-fb.c +++ /tmp/nothing/hw/display/next-fb.c @@ -108,7 +108,6 @@ static void nextfb_realize(DeviceState * NeXTFbState *s = NEXTFB(dev); memory_region_init_ram(&s->fb_mr, OBJECT(dev), "next-video", 0x1CB100, - &error_fatal); sysbus_init_mmio(SYS_BUS_DEVICE(dev), &s->fb_mr); s->invalidate = 1; diff -u -p ./hw/display/qxl.c /tmp/nothing/hw/display/qxl.c --- ./hw/display/qxl.c +++ /tmp/nothing/hw/display/qxl.c @@ -2232,7 +2232,6 @@ static void qxl_realize_secondary(PCIDev qxl_init_ramsize(qxl); memory_region_init_ram(&qxl->vga.vram, OBJECT(dev), "qxl.vgavram", - qxl->vga.vram_size, &error_fatal); qxl->vga.vram_ptr = memory_region_get_ram_ptr(&qxl->vga.vram); qxl->vga.con = graphic_console_init(DEVICE(dev), 0, &qxl_ops, qxl); qxl->ssd.dcl.con = qxl->vga.con; diff -u -p ./hw/display/cg3.c /tmp/nothing/hw/display/cg3.c --- ./hw/display/cg3.c +++ /tmp/nothing/hw/display/cg3.c @@ -311,7 +311,6 @@ static void cg3_realizefn(DeviceState *d } memory_region_init_ram(&s->vram_mem, NULL, "cg3.vram", s->vram_size, - &error_fatal); memory_region_set_log(&s->vram_mem, true, DIRTY_MEMORY_VGA); sysbus_init_mmio(sbd, &s->vram_mem); diff -u -p ./hw/dma/sparc32_dma.c /tmp/nothing/hw/dma/sparc32_dma.c --- ./hw/dma/sparc32_dma.c +++ /tmp/nothing/hw/dma/sparc32_dma.c @@ -309,7 +309,6 @@ static void sparc32_espdma_device_realiz esp->dma_opaque = SPARC32_DMA_DEVICE(dev); sysbus->it_shift = 2; esp->dma_enabled = 1; - sysbus_realize(SYS_BUS_DEVICE(sysbus), &error_fatal); } static void sparc32_espdma_device_class_init(ObjectClass *klass, void *data) @@ -344,7 +343,6 @@ static void sparc32_ledma_device_realize SysBusPCNetState *lance = SYSBUS_PCNET(&s->lance); object_property_set_link(OBJECT(lance), "dma", OBJECT(dev), &error_abort); - sysbus_realize(SYS_BUS_DEVICE(lance), &error_fatal); } static void sparc32_ledma_device_class_init(ObjectClass *klass, void *data) diff -u -p ./hw/block/onenand.c /tmp/nothing/hw/block/onenand.c --- ./hw/block/onenand.c +++ /tmp/nothing/hw/block/onenand.c @@ -812,7 +812,6 @@ static void onenand_realize(DeviceState s->otp = memset(g_malloc((64 + 2) << PAGE_SHIFT), 0xff, (64 + 2) << PAGE_SHIFT); memory_region_init_ram_nomigrate(&s->ram, OBJECT(s), "onenand.ram", - 0xc000 << s->shift, &error_fatal); vmstate_register_ram_global(&s->ram); ram = memory_region_get_ram_ptr(&s->ram); s->boot[0] = ram + (0x0000 << s->shift); diff -u -p ./hw/isa/vt82c686.c /tmp/nothing/hw/isa/vt82c686.c --- ./hw/isa/vt82c686.c +++ /tmp/nothing/hw/isa/vt82c686.c @@ -618,7 +618,6 @@ static void vt82c686b_realize(PCIDevice qdev_init_gpio_out(dev, &s->cpu_intr, 1); isa_irq = qemu_allocate_irqs(via_isa_request_i8259_irq, s, 1); isa_bus = isa_bus_new(dev, get_system_memory(), pci_address_space_io(d), - &error_fatal); isa_bus_irqs(isa_bus, i8259_init(isa_bus, *isa_irq)); i8254_pit_init(isa_bus, 0x40, 0, NULL); i8257_dma_init(isa_bus, 0); @@ -699,7 +698,6 @@ static void vt8231_realize(PCIDevice *d, qdev_init_gpio_out(dev, &s->cpu_intr, 1); isa_irq = qemu_allocate_irqs(via_isa_request_i8259_irq, s, 1); isa_bus = isa_bus_new(dev, get_system_memory(), pci_address_space_io(d), - &error_fatal); isa_bus_irqs(isa_bus, i8259_init(isa_bus, *isa_irq)); i8254_pit_init(isa_bus, 0x40, 0, NULL); i8257_dma_init(isa_bus, 0); diff -u -p ./hw/isa/isa-superio.c /tmp/nothing/hw/isa/isa-superio.c --- ./hw/isa/isa-superio.c +++ /tmp/nothing/hw/isa/isa-superio.c @@ -141,7 +141,6 @@ static void isa_superio_realize(DeviceSt /* Keyboard, mouse */ isa = isa_new(TYPE_I8042); object_property_add_child(OBJECT(sio), TYPE_I8042, OBJECT(isa)); - isa_realize_and_unref(isa, bus, &error_fatal); sio->kbc = isa; /* IDE */ diff -u -p ./hw/isa/isa-bus.c /tmp/nothing/hw/isa/isa-bus.c --- ./hw/isa/isa-bus.c +++ /tmp/nothing/hw/isa/isa-bus.c @@ -61,7 +61,6 @@ ISABus *isa_bus_new(DeviceState *dev, Me } if (!dev) { dev = qdev_new("isabus-bridge"); - sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal); } isabus = ISA_BUS(qbus_create(TYPE_ISA_BUS, dev, NULL)); diff -u -p ./hw/misc/macio/macio.c /tmp/nothing/hw/misc/macio/macio.c --- ./hw/misc/macio/macio.c +++ /tmp/nothing/hw/misc/macio/macio.c @@ -286,7 +286,6 @@ static void macio_newworld_realize(PCIDe /* OpenPIC */ qdev_prop_set_uint32(pic_dev, "model", OPENPIC_MODEL_KEYLARGO); sysbus_dev = SYS_BUS_DEVICE(&ns->pic); - sysbus_realize_and_unref(sysbus_dev, &error_fatal); memory_region_add_subregion(&s->bar, 0x40000, sysbus_mmio_get_region(sysbus_dev, 0)); diff -u -p ./hw/misc/xlnx-versal-xramc.c /tmp/nothing/hw/misc/xlnx-versal-xramc.c --- ./hw/misc/xlnx-versal-xramc.c +++ /tmp/nothing/hw/misc/xlnx-versal-xramc.c @@ -182,7 +182,6 @@ static void xram_ctrl_realize(DeviceStat memory_region_init_ram(&s->ram, OBJECT(s), object_get_canonical_path_component(OBJECT(s)), - s->cfg.size, &error_fatal); sysbus_init_mmio(sbd, &s->ram); } diff -u -p ./hw/sparc64/sun4u.c /tmp/nothing/hw/sparc64/sun4u.c --- ./hw/sparc64/sun4u.c +++ /tmp/nothing/hw/sparc64/sun4u.c @@ -509,7 +509,6 @@ static void ram_realize(DeviceState *dev SysBusDevice *sbd = SYS_BUS_DEVICE(dev); memory_region_init_ram_nomigrate(&d->ram, OBJECT(d), "sun4u.ram", d->size, - &error_fatal); vmstate_register_ram_global(&d->ram); sysbus_init_mmio(sbd, &d->ram); } diff -u -p ./hw/nvram/eeprom_at24c.c /tmp/nothing/hw/nvram/eeprom_at24c.c --- ./hw/nvram/eeprom_at24c.c +++ /tmp/nothing/hw/nvram/eeprom_at24c.c @@ -135,7 +135,6 @@ static void at24c_eeprom_realize(DeviceS } if (blk_set_perm(ee->blk, BLK_PERM_CONSISTENT_READ | BLK_PERM_WRITE, - BLK_PERM_ALL, &error_fatal) < 0) { error_setg(errp, "%s: Backing file incorrect permission", TYPE_AT24C_EE); diff -u -p ./hw/arm/xlnx-zynqmp.c /tmp/nothing/hw/arm/xlnx-zynqmp.c --- ./hw/arm/xlnx-zynqmp.c +++ /tmp/nothing/hw/arm/xlnx-zynqmp.c @@ -219,7 +219,6 @@ static void xlnx_zynqmp_create_rpu(Machi } } - qdev_realize(DEVICE(&s->rpu_cluster), NULL, &error_fatal); } static void xlnx_zynqmp_init(Object *obj) @@ -352,7 +351,6 @@ static void xlnx_zynqmp_realize(DeviceSt qdev_prop_set_bit(DEVICE(&s->gic), "has-virtualization-extensions", s->virt); - qdev_realize(DEVICE(&s->apu_cluster), NULL, &error_fatal); /* Realize APUs before realizing the GIC. KVM requires this. */ for (i = 0; i < num_apus; i++) { diff -u -p ./hw/arm/allwinner-a10.c /tmp/nothing/hw/arm/allwinner-a10.c --- ./hw/arm/allwinner-a10.c +++ /tmp/nothing/hw/arm/allwinner-a10.c @@ -99,7 +99,6 @@ static void aw_a10_realize(DeviceState * sysbus_connect_irq(sysbusdev, 5, qdev_get_gpio_in(dev, 68)); memory_region_init_ram(&s->sram_a, OBJECT(dev), "sram A", 48 * KiB, - &error_fatal); memory_region_add_subregion(get_system_memory(), 0x00000000, &s->sram_a); create_unimplemented_device("a10-sram-ctrl", 0x01c00000, 4 * KiB); diff -u -p ./hw/arm/xlnx-versal.c /tmp/nothing/hw/arm/xlnx-versal.c --- ./hw/arm/xlnx-versal.c +++ /tmp/nothing/hw/arm/xlnx-versal.c @@ -403,7 +403,6 @@ static void versal_realize(DeviceState * /* Create the On Chip Memory (OCM). */ memory_region_init_ram(&s->lpd.mr_ocm, OBJECT(s), "ocm", - MM_OCM_SIZE, &error_fatal); memory_region_add_subregion_overlap(&s->mr_ps, MM_OCM, &s->lpd.mr_ocm, 0); memory_region_add_subregion_overlap(&s->fpd.apu.mr, 0, &s->mr_ps, 0); diff -u -p ./hw/pci/pci.c /tmp/nothing/hw/pci/pci.c --- ./hw/pci/pci.c +++ /tmp/nothing/hw/pci/pci.c @@ -2412,7 +2412,6 @@ static void pci_add_option_rom(PCIDevice snprintf(name, sizeof(name), "%s.rom", object_get_typename(OBJECT(pdev))); } pdev->has_rom = true; - memory_region_init_rom(&pdev->rom, OBJECT(pdev), name, pdev->romsize, &error_fatal); ptr = memory_region_get_ram_ptr(&pdev->rom); if (load_image_size(path, ptr, size) < 0) { error_setg(errp, "failed to load romfile \"%s\"", pdev->romfile); diff -u -p ./hw/pci-bridge/pci_expander_bridge.c /tmp/nothing/hw/pci-bridge/pci_expander_bridge.c --- ./hw/pci-bridge/pci_expander_bridge.c +++ /tmp/nothing/hw/pci-bridge/pci_expander_bridge.c @@ -264,7 +264,6 @@ static void pxb_dev_realize_common(PCIDe goto err_register_bus; } - sysbus_realize_and_unref(SYS_BUS_DEVICE(ds), &error_fatal); if (bds) { qdev_realize_and_unref(bds, &bus->qbus, &error_fatal); } diff -u -p ./softmmu/vl.c /tmp/nothing/softmmu/vl.c --- ./softmmu/vl.c +++ /tmp/nothing/softmmu/vl.c @@ -2189,7 +2189,6 @@ static void qemu_record_config_group(con assert(!from_json); keyval_merge(machine_opts_dict, dict, errp); } else if (g_str_equal(group, "smp-opts")) { - machine_merge_property("smp", dict, &error_fatal); } else { abort(); } @@ -2309,7 +2308,6 @@ static int do_configure_accelerator(void object_apply_compat_props(OBJECT(accel)); qemu_opt_foreach(opts, accelerator_set_property, accel, - &error_fatal); ret = accel_init_machine(accel, current_machine); if (ret < 0) {
On Tue, 24 Aug 2021 at 14:58, Eduardo Habkost <ehabkost@redhat.com> wrote: > > On Tue, Aug 24, 2021 at 01:16:40PM +0100, Peter Maydell wrote: > > On Tue, 24 Aug 2021 at 13:05, Markus Armbruster <armbru@redhat.com> wrote: > > > When you know that all callers handle errors like &error_fatal does, use > > > of &error_fatal doesn't produce wrong behavior. It's still kind of > > > wrong, because relying on such a non-local argument without a genuine > > > need is. > > > > Not using error_fatal results in quite a bit of extra boilerplate > > for all those extra explicit "check for failure, print the error > > message and exit" points. > > I don't get it. There's no need for extra boilerplate if the > caller is using &error_fatal. Yes, that is what I mean: if you do not use error_fatal, then there is extra boilerplate. Markus is suggesting that we should avoid using error_fatal, and my response is "the cost of that would be that we add boilerplate". > > Right now we have nearly a thousand instances of error_fatal > > in the codebase, incidentally. > > It looks like 73 of them are in functions that take an Error** > argument. Those could probably be fairly easily adjusted to pass the error back instead. Still leaves nearly 900 to go :-) -- PMM
Peter Maydell <peter.maydell@linaro.org> writes: > On Tue, 24 Aug 2021 at 13:05, Markus Armbruster <armbru@redhat.com> wrote: >> When you know that all callers handle errors like &error_fatal does, use >> of &error_fatal doesn't produce wrong behavior. It's still kind of >> wrong, because relying on such a non-local argument without a genuine >> need is. > > Not using error_fatal results in quite a bit of extra boilerplate > for all those extra explicit "check for failure, print the error > message and exit" points. If the MachineState init function took > an Error** that might be a mild encouragement to "pass an Error > upward rather than exiting"; but it doesn't. > > Right now we have nearly a thousand instances of error_fatal > in the codebase, incidentally. Use of &error_fatal is clearly superior to accomplishing the same behavior the way you describe. My point was this behavior is usually wrong in functions with an Error **errp parameter. When a function is only ever used in contexts where errors are handled the way &error_fatal does, then you can keep things a bit more readable with &error_fatal and without an Error **errp parameter. Nothing wrong with that.
Eduardo Habkost <ehabkost@redhat.com> writes: > On Tue, Aug 24, 2021 at 01:16:40PM +0100, Peter Maydell wrote: >> On Tue, 24 Aug 2021 at 13:05, Markus Armbruster <armbru@redhat.com> wrote: >> > When you know that all callers handle errors like &error_fatal does, use >> > of &error_fatal doesn't produce wrong behavior. It's still kind of >> > wrong, because relying on such a non-local argument without a genuine >> > need is. >> >> Not using error_fatal results in quite a bit of extra boilerplate >> for all those extra explicit "check for failure, print the error >> message and exit" points. > > I don't get it. There's no need for extra boilerplate if the > caller is using &error_fatal. > >> If the MachineState init function took >> an Error** that might be a mild encouragement to "pass an Error >> upward rather than exiting"; but it doesn't. > > Agreed. > >> >> Right now we have nearly a thousand instances of error_fatal >> in the codebase, incidentally. > > It looks like 73 of them are in functions that take an Error** > argument. > > Coccinelle patch for finding them: > > @@ > typedef Error; > type T; > identifier errp, fn; > @@ > T fn(..., Error **errp) > { > ... > *&error_fatal > ... > } > > > Coccinelle output: [...] These do look suspicious to me.
On Tue, 24 Aug 2021 at 15:27, Markus Armbruster <armbru@redhat.com> wrote: > > Peter Maydell <peter.maydell@linaro.org> writes: > > > On Tue, 24 Aug 2021 at 13:05, Markus Armbruster <armbru@redhat.com> wrote: > >> When you know that all callers handle errors like &error_fatal does, use > >> of &error_fatal doesn't produce wrong behavior. It's still kind of > >> wrong, because relying on such a non-local argument without a genuine > >> need is. > > > > Not using error_fatal results in quite a bit of extra boilerplate > > for all those extra explicit "check for failure, print the error > > message and exit" points. If the MachineState init function took > > an Error** that might be a mild encouragement to "pass an Error > > upward rather than exiting"; but it doesn't. > > > > Right now we have nearly a thousand instances of error_fatal > > in the codebase, incidentally. > > Use of &error_fatal is clearly superior to accomplishing the same > behavior the way you describe. > > My point was this behavior is usually wrong in functions with an Error > **errp parameter. Right, but as Eduardo has noted, only about 8% of our use of error_fatal is like that. The vast bulk is other cases, so if you want to call it "kind of wrong" we ought to have a view of how it could be done otherwise. -- PMM
Peter Maydell <peter.maydell@linaro.org> writes: > On Tue, 24 Aug 2021 at 15:27, Markus Armbruster <armbru@redhat.com> wrote: >> >> Peter Maydell <peter.maydell@linaro.org> writes: >> >> > On Tue, 24 Aug 2021 at 13:05, Markus Armbruster <armbru@redhat.com> wrote: >> >> When you know that all callers handle errors like &error_fatal does, use >> >> of &error_fatal doesn't produce wrong behavior. It's still kind of >> >> wrong, because relying on such a non-local argument without a genuine >> >> need is. >> > >> > Not using error_fatal results in quite a bit of extra boilerplate >> > for all those extra explicit "check for failure, print the error >> > message and exit" points. If the MachineState init function took >> > an Error** that might be a mild encouragement to "pass an Error >> > upward rather than exiting"; but it doesn't. >> > >> > Right now we have nearly a thousand instances of error_fatal >> > in the codebase, incidentally. >> >> Use of &error_fatal is clearly superior to accomplishing the same >> behavior the way you describe. >> >> My point was this behavior is usually wrong in functions with an Error >> **errp parameter. > > Right, but as Eduardo has noted, only about 8% of our use of > error_fatal is like that. The vast bulk is other cases, so > if you want to call it "kind of wrong" we ought to have a view > of how it could be done otherwise. True, except when I called it "kind of wrong", I was still talking about functions with an Error **errp parameter. Many (most?) existing uses of &error_fatal are just fine. Which pleases me, having created it in commit a29a37b994c.
On Tue, 24 Aug 2021 at 16:15, Markus Armbruster <armbru@redhat.com> wrote: > True, except when I called it "kind of wrong", I was still talking about > functions with an Error **errp parameter. Oh yes, so you were. I even quoted your sentence starting "In functions with an Error **errp parameter ...". I must have been half-asleep still this morning. Apologies for starting an unnecessary thread after which we all turn out to be in complete agreement :-) -- PMM
Peter Maydell <peter.maydell@linaro.org> writes: > On Tue, 24 Aug 2021 at 16:15, Markus Armbruster <armbru@redhat.com> wrote: >> True, except when I called it "kind of wrong", I was still talking about >> functions with an Error **errp parameter. > > Oh yes, so you were. I even quoted your sentence starting > "In functions with an Error **errp parameter ...". > I must have been half-asleep still this morning. > > Apologies for starting an unnecessary thread after which we all > turn out to be in complete agreement :-) No problem at all :)
diff --git a/hw/usb/hcd-xhci-pci.c b/hw/usb/hcd-xhci-pci.c index e934b1a5b1f..71f6629ccde 100644 --- a/hw/usb/hcd-xhci-pci.c +++ b/hw/usb/hcd-xhci-pci.c @@ -115,7 +115,7 @@ static void usb_xhci_pci_realize(struct PCIDevice *dev, Error **errp) dev->config[PCI_CACHE_LINE_SIZE] = 0x10; dev->config[0x60] = 0x30; /* release number */ - object_property_set_link(OBJECT(&s->xhci), "host", OBJECT(s), NULL); + object_property_set_link(OBJECT(&s->xhci), "host", OBJECT(s), &error_fatal); s->xhci.intr_update = xhci_pci_intr_update; s->xhci.intr_raise = xhci_pci_intr_raise; if (!qdev_realize(DEVICE(&s->xhci), NULL, errp)) {
Do not ignore eventual error if we failed at setting the 'host' property of the TYPE_XHCI model. Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> --- hw/usb/hcd-xhci-pci.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)