[PATCH v4 0/4] hw/audio/pcspk: Inline pcspk_init()

[PATCH v4 0/4] hw/audio/pcspk: Inline pcspk_init()

[Philippe Mathieu-Daudé]

Unfortunately v2 was merged as commit 40f8214fcd,
so adapt v3 to clean the mess.

Philippe Mathieu-Daudé (4):
  hw/i386/pc: Pass Error** argument to pc_basic_device_init()
  hw/i386/pc: Propagate error if HPET device creation failed
  hw/i386/pc: Propagate error if PC_SPEAKER device creation failed
  hw/isa/i82378: Propagate error if PC_SPEAKER device creation failed

 include/hw/i386/pc.h |  5 +++--
 hw/i386/pc.c         | 15 +++++++++++----
 hw/i386/pc_piix.c    |  2 +-
 hw/i386/pc_q35.c     |  2 +-
 hw/isa/i82378.c      |  4 +++-
 5 files changed, 19 insertions(+), 9 deletions(-)

-- 
2.41.0

[PATCH v4 1/4] hw/i386/pc: Pass Error** argument to pc_basic_device_init()

[Philippe Mathieu-Daudé]

pc_basic_device_init() creates devices which can fail,
allow to propagate error to caller.

Suggested-by: Markus Armbruster <armbru@redhat.com>
Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 include/hw/i386/pc.h | 5 +++--
 hw/i386/pc.c         | 7 +++++--
 hw/i386/pc_piix.c    | 2 +-
 hw/i386/pc_q35.c     | 2 +-
 4 files changed, 10 insertions(+), 6 deletions(-)

diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
index bec38cb92c..069c27368d 100644
--- a/include/hw/i386/pc.h
+++ b/include/hw/i386/pc.h
@@ -174,11 +174,12 @@ void pc_memory_init(PCMachineState *pcms,
                     uint64_t pci_hole64_size);
 uint64_t pc_pci_hole64_start(void);
 DeviceState *pc_vga_init(ISABus *isa_bus, PCIBus *pci_bus);
-void pc_basic_device_init(struct PCMachineState *pcms,
+bool pc_basic_device_init(struct PCMachineState *pcms,
                           ISABus *isa_bus, qemu_irq *gsi,
                           ISADevice *rtc_state,
                           bool create_fdctrl,
-                          uint32_t hpet_irqs);
+                          uint32_t hpet_irqs,
+                          Error **errp);
 void pc_cmos_init(PCMachineState *pcms,
                   BusState *ide0, BusState *ide1,
                   ISADevice *s);
diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index f7ee638bec..2481c11e83 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -1189,11 +1189,12 @@ static void pc_superio_init(ISABus *isa_bus, bool create_fdctrl,
     g_free(a20_line);
 }
 
-void pc_basic_device_init(struct PCMachineState *pcms,
+bool pc_basic_device_init(struct PCMachineState *pcms,
                           ISABus *isa_bus, qemu_irq *gsi,
                           ISADevice *rtc_state,
                           bool create_fdctrl,
-                          uint32_t hpet_irqs)
+                          uint32_t hpet_irqs,
+                          Error **errp)
 {
     int i;
     DeviceState *hpet = NULL;
@@ -1291,6 +1292,8 @@ void pc_basic_device_init(struct PCMachineState *pcms,
     /* Super I/O */
     pc_superio_init(isa_bus, create_fdctrl, pcms->i8042_enabled,
                     pcms->vmport != ON_OFF_AUTO_ON);
+
+    return true;
 }
 
 void pc_nic_init(PCMachineClass *pcmc, ISABus *isa_bus, PCIBus *pci_bus)
diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index e36a3262b2..0d9cdf773e 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -320,7 +320,7 @@ static void pc_init1(MachineState *machine,
 
     /* init basic PC hardware */
     pc_basic_device_init(pcms, isa_bus, x86ms->gsi, rtc_state, true,
-                         0x4);
+                         0x4, &error_fatal);
 
     pc_nic_init(pcmc, isa_bus, pci_bus);
 
diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
index a7386f2ca2..e4b05e3139 100644
--- a/hw/i386/pc_q35.c
+++ b/hw/i386/pc_q35.c
@@ -301,7 +301,7 @@ static void pc_q35_init(MachineState *machine)
 
     /* init basic PC hardware */
     pc_basic_device_init(pcms, isa_bus, x86ms->gsi, rtc_state, !mc->no_floppy,
-                         0xff0104);
+                         0xff0104, &error_fatal);
 
     if (pcms->sata_enabled) {
         /* ahci and SATA device, for q35 1 ahci controller is built-in */
-- 
2.41.0

[PATCH v4 2/4] hw/i386/pc: Propagate error if HPET device creation failed

[Philippe Mathieu-Daudé]

Reported-by: Bernhard Beschow <shentey@gmail.com>
Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/i386/pc.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 2481c11e83..6750ed427a 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -1235,7 +1235,9 @@ bool pc_basic_device_init(struct PCMachineState *pcms,
         if (!compat) {
             qdev_prop_set_uint32(hpet, HPET_INTCAP, hpet_irqs);
         }
-        sysbus_realize_and_unref(SYS_BUS_DEVICE(hpet), &error_fatal);
+        if (!sysbus_realize_and_unref(SYS_BUS_DEVICE(hpet), errp)) {
+            return false;
+        }
         sysbus_mmio_map(SYS_BUS_DEVICE(hpet), 0, HPET_BASE);
 
         for (i = 0; i < IOAPIC_NUM_PINS; i++) {
-- 
2.41.0

[PATCH v4 3/4] hw/i386/pc: Propagate error if PC_SPEAKER device creation failed

[Philippe Mathieu-Daudé]

In commit 40f8214fcd ("hw/audio/pcspk: Inline pcspk_init()")
we neglected to give a change to the caller to handle failed
device creation cleanly. Here this can not happen because
pc_basic_device_init() uses &error_fatal. Anyhow, do the
proper error reporting to avoid bad code example spreading.

Reported-by: Bernhard Beschow <shentey@gmail.com>
Suggested-by: Markus Armbruster <armbru@redhat.com>
Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/i386/pc.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 6750ed427a..3937d88355 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -1288,7 +1288,9 @@ bool pc_basic_device_init(struct PCMachineState *pcms,
         }
         object_property_set_link(OBJECT(pcms->pcspk), "pit",
                                  OBJECT(pit), &error_fatal);
-        isa_realize_and_unref(pcms->pcspk, isa_bus, &error_fatal);
+        if (!isa_realize_and_unref(pcms->pcspk, isa_bus, errp)) {
+            return false;
+        }
     }
 
     /* Super I/O */
-- 
2.41.0

[PATCH v4 4/4] hw/isa/i82378: Propagate error if PC_SPEAKER device creation failed

[Philippe Mathieu-Daudé]

In commit 40f8214fcd ("hw/audio/pcspk: Inline pcspk_init()")
we neglected to give a change to the caller to handle failed
device creation cleanly. Respect the caller API contract and
propagate the error if creating the PC_SPEAKER device ever
failed. This avoid yet another bad API use to be taken as
example and copy / pasted all over the code base.

Reported-by: Bernhard Beschow <shentey@gmail.com>
Suggested-by: Markus Armbruster <armbru@redhat.com>
Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/isa/i82378.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/hw/isa/i82378.c b/hw/isa/i82378.c
index 79ffbb52a0..203b92c264 100644
--- a/hw/isa/i82378.c
+++ b/hw/isa/i82378.c
@@ -105,7 +105,9 @@ static void i82378_realize(PCIDevice *pci, Error **errp)
     /* speaker */
     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);
+    if (!isa_realize_and_unref(pcspk, isabus, errp)) {
+        return;
+    }
 
     /* 2 82C37 (dma) */
     isa_create_simple(isabus, "i82374");
-- 
2.41.0

Re: [PATCH v4 1/4] hw/i386/pc: Pass Error** argument to pc_basic_device_init()

[Richard Henderson]

On 10/20/23 10:15, Philippe Mathieu-Daudé wrote:
> pc_basic_device_init() creates devices which can fail,
> allow to propagate error to caller.
> 
> Suggested-by: Markus Armbruster <armbru@redhat.com>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   include/hw/i386/pc.h | 5 +++--
>   hw/i386/pc.c         | 7 +++++--
>   hw/i386/pc_piix.c    | 2 +-
>   hw/i386/pc_q35.c     | 2 +-
>   4 files changed, 10 insertions(+), 6 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

Re: [PATCH v4 2/4] hw/i386/pc: Propagate error if HPET device creation failed

[Richard Henderson]

On 10/20/23 10:15, Philippe Mathieu-Daudé wrote:
> Reported-by: Bernhard Beschow <shentey@gmail.com>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   hw/i386/pc.c | 4 +++-
>   1 file changed, 3 insertions(+), 1 deletion(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

Re: [PATCH v4 3/4] hw/i386/pc: Propagate error if PC_SPEAKER device creation failed

[Richard Henderson]

On 10/20/23 10:15, Philippe Mathieu-Daudé wrote:
> In commit 40f8214fcd ("hw/audio/pcspk: Inline pcspk_init()")
> we neglected to give a change to the caller to handle failed
> device creation cleanly. Here this can not happen because
> pc_basic_device_init() uses &error_fatal. Anyhow, do the
> proper error reporting to avoid bad code example spreading.
> 
> Reported-by: Bernhard Beschow <shentey@gmail.com>
> Suggested-by: Markus Armbruster <armbru@redhat.com>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   hw/i386/pc.c | 4 +++-
>   1 file changed, 3 insertions(+), 1 deletion(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

Re: [PATCH v4 4/4] hw/isa/i82378: Propagate error if PC_SPEAKER device creation failed

[Richard Henderson]

On 10/20/23 10:15, Philippe Mathieu-Daudé wrote:
> In commit 40f8214fcd ("hw/audio/pcspk: Inline pcspk_init()")
> we neglected to give a change to the caller to handle failed
> device creation cleanly. Respect the caller API contract and
> propagate the error if creating the PC_SPEAKER device ever
> failed. This avoid yet another bad API use to be taken as
> example and copy / pasted all over the code base.
> 
> Reported-by: Bernhard Beschow <shentey@gmail.com>
> Suggested-by: Markus Armbruster <armbru@redhat.com>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   hw/isa/i82378.c | 4 +++-
>   1 file changed, 3 insertions(+), 1 deletion(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

Re: [PATCH v4 4/4] hw/isa/i82378: Propagate error if PC_SPEAKER device creation failed

[Bernhard Beschow]

Am 20. Oktober 2023 17:15:08 UTC schrieb "Philippe Mathieu-Daudé" <philmd@linaro.org>:
>In commit 40f8214fcd ("hw/audio/pcspk: Inline pcspk_init()")
>we neglected to give a change to the caller to handle failed
>device creation cleanly. Respect the caller API contract and
>propagate the error if creating the PC_SPEAKER device ever
>failed. This avoid yet another bad API use to be taken as
>example and copy / pasted all over the code base.
>
>Reported-by: Bernhard Beschow <shentey@gmail.com>
>Suggested-by: Markus Armbruster <armbru@redhat.com>
>Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Reviewed-by: Bernhard Beschow <shentey@gmail.com>

>---
> hw/isa/i82378.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
>diff --git a/hw/isa/i82378.c b/hw/isa/i82378.c
>index 79ffbb52a0..203b92c264 100644
>--- a/hw/isa/i82378.c
>+++ b/hw/isa/i82378.c
>@@ -105,7 +105,9 @@ static void i82378_realize(PCIDevice *pci, Error **errp)
>     /* speaker */
>     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);
>+    if (!isa_realize_and_unref(pcspk, isabus, errp)) {
>+        return;
>+    }
> 
>     /* 2 82C37 (dma) */
>     isa_create_simple(isabus, "i82374");

Re: [PATCH v4 0/4] hw/audio/pcspk: Inline pcspk_init()

[Bernhard Beschow]

Am 20. Oktober 2023 17:15:04 UTC schrieb "Philippe Mathieu-Daudé" <philmd@linaro.org>:
>Unfortunately v2 was merged as commit 40f8214fcd,
>so adapt v3 to clean the mess.
>
>Philippe Mathieu-Daudé (4):
>  hw/i386/pc: Pass Error** argument to pc_basic_device_init()
>  hw/i386/pc: Propagate error if HPET device creation failed
>  hw/i386/pc: Propagate error if PC_SPEAKER device creation failed

I'm not sure if I'd do these first three patches. The reason is that machines don't inherit from DeviceState and therefore don't have canonical methods such as realize() to propagate errors. Propagating the errors in the machine init helper methods seem a bit ad-hoc to me.

>  hw/isa/i82378: Propagate error if PC_SPEAKER device creation failed

The reason I suggested use of `errp` here is that it is already a parameter.

Best regards,
Bernhard

>
> include/hw/i386/pc.h |  5 +++--
> hw/i386/pc.c         | 15 +++++++++++----
> hw/i386/pc_piix.c    |  2 +-
> hw/i386/pc_q35.c     |  2 +-
> hw/isa/i82378.c      |  4 +++-
> 5 files changed, 19 insertions(+), 9 deletions(-)
>

Re: [PATCH v4 0/4] hw/audio/pcspk: Inline pcspk_init()

[Markus Armbruster]

Bernhard Beschow <shentey@gmail.com> writes:

> Am 20. Oktober 2023 17:15:04 UTC schrieb "Philippe Mathieu-Daudé" <philmd@linaro.org>:
>>Unfortunately v2 was merged as commit 40f8214fcd,
>>so adapt v3 to clean the mess.
>>
>>Philippe Mathieu-Daudé (4):
>>  hw/i386/pc: Pass Error** argument to pc_basic_device_init()
>>  hw/i386/pc: Propagate error if HPET device creation failed
>>  hw/i386/pc: Propagate error if PC_SPEAKER device creation failed
>
> I'm not sure if I'd do these first three patches. The reason is that machines don't inherit from DeviceState and therefore don't have canonical methods such as realize() to propagate errors. Propagating the errors in the machine init helper methods seem a bit ad-hoc to me.

The Error interface enables separation of error detection and error
handling.  On detection, we create an Error object, and handling
consumes it.

A function that leaves error handling to its callers generally requires
its callees to leave it, too.  Use of &error_fatal is wrong then.

Even when error handling need not be left to callers, leaving it can
result in simpler or more robust code.

When a function handles errors itself, say by use of &error_fatal or
error_report(), it's only usable in contexts where this handling is
appropriate.

Sometimes the context is obvious enough, and unlikely to change.
Handling directly is fine then, and can be simpler.

When the context isn't that obvious, leaving error handling to callers
liberates you from thinking about the context, and also enables safe
reuse of the function in other contexts.

I think pc_basic_device_init() doesn't *need* the change, as it's
context is obvious enough.  But the change is fine, and if we apply it,
we never have to think about the context again.  Matter of taste.

>>  hw/isa/i82378: Propagate error if PC_SPEAKER device creation failed
>
> The reason I suggested use of `errp` here is that it is already a parameter.

Use of &error_fatal in a function taking @errp is almost always wrong.
The patch fixes an instance of "wrong".

Re: [PATCH v4 0/4] hw/audio/pcspk: Inline pcspk_init()

[Philippe Mathieu-Daudé]

On 3/11/23 09:56, Markus Armbruster wrote:
> Bernhard Beschow <shentey@gmail.com> writes:
> 
>> Am 20. Oktober 2023 17:15:04 UTC schrieb "Philippe Mathieu-Daudé" <philmd@linaro.org>:
>>> Unfortunately v2 was merged as commit 40f8214fcd,
>>> so adapt v3 to clean the mess.
>>>
>>> Philippe Mathieu-Daudé (4):
>>>   hw/i386/pc: Pass Error** argument to pc_basic_device_init()
>>>   hw/i386/pc: Propagate error if HPET device creation failed
>>>   hw/i386/pc: Propagate error if PC_SPEAKER device creation failed
>>
>> I'm not sure if I'd do these first three patches. The reason is that machines don't inherit from DeviceState and therefore don't have canonical methods such as realize() to propagate errors. Propagating the errors in the machine init helper methods seem a bit ad-hoc to me.
> 
> The Error interface enables separation of error detection and error
> handling.  On detection, we create an Error object, and handling
> consumes it.
> 
> A function that leaves error handling to its callers generally requires
> its callees to leave it, too.  Use of &error_fatal is wrong then.
> 
> Even when error handling need not be left to callers, leaving it can
> result in simpler or more robust code.
> 
> When a function handles errors itself, say by use of &error_fatal or
> error_report(), it's only usable in contexts where this handling is
> appropriate.
> 
> Sometimes the context is obvious enough, and unlikely to change.
> Handling directly is fine then, and can be simpler.
> 
> When the context isn't that obvious, leaving error handling to callers
> liberates you from thinking about the context, and also enables safe
> reuse of the function in other contexts.
> 
> I think pc_basic_device_init() doesn't *need* the change, as it's
> context is obvious enough.  But the change is fine, and if we apply it,
> we never have to think about the context again.  Matter of taste.

I disagree with Bernhard because pc_basic_device_init() could end up
refactored and called elsewhere where error can be propagated -- think
qdev modules --, and in its current form we'll keep ignoring the caller
errp and use &error_fatal (see patch #2 and #3). Also, better to have
an unified style rather that trying to "optimize" arguments on a per
case basis. Anyhow, my 2 cents.

> 
>>>   hw/isa/i82378: Propagate error if PC_SPEAKER device creation failed
>>
>> The reason I suggested use of `errp` here is that it is already a parameter.
> 
> Use of &error_fatal in a function taking @errp is almost always wrong.
> The patch fixes an instance of "wrong".

Due to Bernhard concerns, I'm only queuing patch #4.

Regards,

Phil.