Re: [PATCH] hw/intc: Handle the error of IOAPICCommonClass.realize()

[Philippe Mathieu-Daudé]

Hi Zhao,

On 31/1/24 15:29, Zhao Liu wrote:
> From: Zhao Liu <zhao1.liu@intel.com>
>
> IOAPICCommonClass implements its own private realize(), and this private
> realize() allows error.
>
> Therefore, return directly if IOAPICCommonClass.realize() meets error.
>
> Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
> ---
>   hw/intc/ioapic_common.c | 3 +++
>   1 file changed, 3 insertions(+)
>
> diff --git a/hw/intc/ioapic_common.c b/hw/intc/ioapic_common.c
> index cb9bf6214608..3772863377c2 100644
> --- a/hw/intc/ioapic_common.c
> +++ b/hw/intc/ioapic_common.c
> @@ -162,6 +162,9 @@ static void ioapic_common_realize(DeviceState *dev, Error 
> **errp)
>   
>       info = IOAPIC_COMMON_GET_CLASS(s);
>       info->realize(dev, errp);
> +    if (*errp) {
> +        return;
> +    }

Could be clearer to deviate from DeviceRealize and let the
handler return a boolean:

-- >8 --
diff --git a/hw/intc/ioapic_internal.h b/hw/intc/ioapic_internal.h
index 37b8565539..9664bb3e00 100644
--- a/hw/intc/ioapic_internal.h
+++ b/hw/intc/ioapic_internal.h
@@ -92,3 +92,3 @@ struct IOAPICCommonClass {

-    DeviceRealize realize;
+    bool (*realize)(DeviceState *dev, Error **errp);
     DeviceUnrealize unrealize;
diff --git a/hw/i386/kvm/ioapic.c b/hw/i386/kvm/ioapic.c
index 409d0c8c76..96747ef2b8 100644
--- a/hw/i386/kvm/ioapic.c
+++ b/hw/i386/kvm/ioapic.c
@@ -121,3 +121,3 @@ static void kvm_ioapic_set_irq(void *opaque, int 
irq, int level)

-static void kvm_ioapic_realize(DeviceState *dev, Error **errp)
+static bool kvm_ioapic_realize(DeviceState *dev, Error **errp)
 {
@@ -133,2 +133,4 @@ static void kvm_ioapic_realize(DeviceState *dev, 
Error **errp)
     qdev_init_gpio_in(dev, kvm_ioapic_set_irq, IOAPIC_NUM_PINS);
+
+    return true;
 }
diff --git a/hw/intc/ioapic_common.c b/hw/intc/ioapic_common.c
index cb9bf62146..beab65be04 100644
--- a/hw/intc/ioapic_common.c
+++ b/hw/intc/ioapic_common.c
@@ -163,3 +163,5 @@ static void ioapic_common_realize(DeviceState *dev, 
Error **errp)
     info = IOAPIC_COMMON_GET_CLASS(s);
-    info->realize(dev, errp);
+    if (!info->realize(dev, errp)) {
+        return;
+    }

---

What do you think?