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

[Philippe Mathieu-Daudé]

On 20/10/23 06:49, Markus Armbruster wrote:
> 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.

Fair.

> >                                   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?

Indirectly: yes.
Directly: no.

> 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 ;)

Let's continue ignoring the indirect calls.

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