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

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>
---
 include/hw/audio/pcspk.h | 10 ----------
 hw/i386/pc.c             |  4 +++-
 hw/isa/i82378.c          |  5 ++++-
 hw/mips/jazz.c           |  5 ++++-
 4 files changed, 11 insertions(+), 13 deletions(-)

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>
>---
> include/hw/audio/pcspk.h | 10 ----------
> hw/i386/pc.c             |  4 +++-
> hw/isa/i82378.c          |  5 ++++-
> hw/mips/jazz.c           |  5 ++++-
> 4 files changed, 11 insertions(+), 13 deletions(-)
>
>diff --git a/include/hw/audio/pcspk.h b/include/hw/audio/pcspk.h
>index 9506179587..6be75a6b86 100644
>--- a/include/hw/audio/pcspk.h
>+++ b/include/hw/audio/pcspk.h
>@@ -25,16 +25,6 @@
> #ifndef HW_PCSPK_H
> #define HW_PCSPK_H
> 
>-#include "hw/isa/isa.h"
>-#include "hw/qdev-properties.h"
>-#include "qapi/error.h"
>-
> #define TYPE_PC_SPEAKER "isa-pcspk"
> 
>-static inline void pcspk_init(ISADevice *isadev, ISABus *bus, ISADevice *pit)
>-{
>-    object_property_set_link(OBJECT(isadev), "pit", OBJECT(pit), NULL);
>-    isa_realize_and_unref(isadev, bus, &error_fatal);
>-}
>-
> #endif /* HW_PCSPK_H */
>diff --git a/hw/i386/pc.c b/hw/i386/pc.c
>index bb3854d1d0..f7ee638bec 100644
>--- a/hw/i386/pc.c
>+++ b/hw/i386/pc.c
>@@ -1283,7 +1283,9 @@ void pc_basic_device_init(struct PCMachineState *pcms,
>             /* connect PIT to output control line of the HPET */
>             qdev_connect_gpio_out(hpet, 0, qdev_get_gpio_in(DEVICE(pit), 0));
>         }
>-        pcspk_init(pcms->pcspk, isa_bus, pit);
>+        object_property_set_link(OBJECT(pcms->pcspk), "pit",
>+                                 OBJECT(pit), &error_fatal);
>+        isa_realize_and_unref(pcms->pcspk, isa_bus, &error_fatal);
>     }
> 
>     /* Super I/O */
>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.

> 
>     /* 2 82C37 (dma) */
>     isa_create_simple(isabus, "i82374");
>diff --git a/hw/mips/jazz.c b/hw/mips/jazz.c
>index c32d2b0b0a..cdc37126c2 100644
>--- a/hw/mips/jazz.c
>+++ b/hw/mips/jazz.c
>@@ -177,6 +177,7 @@ static void mips_jazz_init(MachineState *machine,
>     SysBusDevice *sysbus;
>     ISABus *isa_bus;
>     ISADevice *pit;
>+    ISADevice *pcspk;
>     DriveInfo *fds[MAX_FD];
>     MemoryRegion *bios = g_new(MemoryRegion, 1);
>     MemoryRegion *bios2 = g_new(MemoryRegion, 1);
>@@ -279,7 +280,9 @@ static void mips_jazz_init(MachineState *machine,
>     isa_bus_register_input_irqs(isa_bus, i8259);
>     i8257_dma_init(isa_bus, 0);
>     pit = i8254_pit_init(isa_bus, 0x40, 0, NULL);
>-    pcspk_init(isa_new(TYPE_PC_SPEAKER), isa_bus, pit);
>+    pcspk = isa_new(TYPE_PC_SPEAKER);
>+    object_property_set_link(OBJECT(pcspk), "pit", OBJECT(pit), &error_fatal);
>+    isa_realize_and_unref(pcspk, isa_bus, &error_fatal);
> 
>     /* Video card */
>     switch (jazz_model) {

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.

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

[...]

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.
> 
> [...]
>

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.
>> [...]

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.
>>> [...]
>