Re: [PATCH v2] hw/audio/pcspk: Inline pcspk_init()

["Philippe Mathieu-Daudé" <philmd@linaro.org>]

On 19/10/23 19:54, Markus Armbruster wrote:
> Bernhard Beschow <shentey@gmail.com> writes:
> 
>> Am 19. Oktober 2023 07:33:07 UTC schrieb "Philippe Mathieu-Daudé" <philmd@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-ayland@ilande.co.uk>
>>> Signed-off-by: Philippe Mathieu-Daudé <philmd@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". Can be updated on top, but so far
this function is not error proof, so I'm not really worried.

> &error_fatal is almost always wrong in a function that takes Error **.
> Happy to explain in more detail if needed.
> 
> [...]
>