Philippe Mathieu-Daudé <phi...@linaro.org> writes: > On 19/10/23 19:54, Markus Armbruster wrote: >> Bernhard Beschow <shen...@gmail.com> writes: >> >>> Am 19. Oktober 2023 07:33:07 UTC schrieb "Philippe Mathieu-Daudé" >>> <phi...@linaro.org>: >>>> pcspk_init() is a legacy init function, inline and remove it. >>>> >>>> Since the device is realized using &error_fatal, use the same >>>> error for setting the "pit" link. >>>> >>>> Reviewed-by: Mark Cave-Ayland <mark.cave-ayl...@ilande.co.uk> >>>> Signed-off-by: Philippe Mathieu-Daudé <phi...@linaro.org> >> [...] >> >>>> diff --git a/hw/isa/i82378.c b/hw/isa/i82378.c >>>> index 63e0857208..79ffbb52a0 100644 >>>> --- a/hw/isa/i82378.c >>>> +++ b/hw/isa/i82378.c >>>> @@ -67,6 +67,7 @@ static void i82378_realize(PCIDevice *pci, Error **errp) >>>> uint8_t *pci_conf; >>>> ISABus *isabus; >>>> ISADevice *pit; >>>> + ISADevice *pcspk; >>>> >>>> pci_conf = pci->config; >>>> pci_set_word(pci_conf + PCI_COMMAND, >>>> @@ -102,7 +103,9 @@ static void i82378_realize(PCIDevice *pci, Error >>>> **errp) >>>> pit = i8254_pit_init(isabus, 0x40, 0, NULL); >>>> >>>> /* speaker */ >>>> - pcspk_init(isa_new(TYPE_PC_SPEAKER), isabus, pit); >>>> + pcspk = isa_new(TYPE_PC_SPEAKER); >>>> + object_property_set_link(OBJECT(pcspk), "pit", OBJECT(pit), >>>> &error_fatal); >>>> + isa_realize_and_unref(pcspk, isabus, &error_fatal); >>> >>> Why not pass errp here? I think that was Mark's comment in v1. > > That would more than "inlining".
Limiting this patch to exactly "inlining" makes sense. It makes the "inapproproate use of &error_fatal" problem more visible. On the one hand, that makes it more likely to be fixed some day. On the other hand, it makes it a more effective bad example. Bad examples tend to multiply. > Can be updated on top, but so far > this function is not error proof, so I'm not really worried. Are there more inappropriate uses of &error_fatal in this function? If no, please throw in a second patch to fix this one. If yes, please add a FIXME comment. Unless you feel like fixing them all, in which case go right ahead ;) >> &error_fatal is almost always wrong in a function that takes Error **. >> Happy to explain in more detail if needed. >> [...]
