[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v2] hw/audio/pcspk: Inline pcspk_init()
From: |
Markus Armbruster |
Subject: |
Re: [PATCH v2] hw/audio/pcspk: Inline pcspk_init() |
Date: |
Fri, 20 Oct 2023 06:49:27 +0200 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/28.2 (gnu/linux) |
Philippe Mathieu-Daudé <philmd@linaro.org> writes:
> 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".
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.
>> [...]