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

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

Posted by Zhao Liu 9 months, 4 weeks ago

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;
+    }
 
     sysbus_init_mmio(SYS_BUS_DEVICE(s), &s->io_memory);
     ioapic_no++;
-- 
2.34.1

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

Posted by Philippe Mathieu-Daudé 9 months, 4 weeks ago

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?

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

Posted by Zhao Liu 9 months, 4 weeks ago

Hi Philippe,

On Wed, Jan 31, 2024 at 05:48:24PM +0100, Philippe Mathieu-Daudé wrote:
> Date: Wed, 31 Jan 2024 17:48:24 +0100
> From: Philippe Mathieu-Daudé <philmd@linaro.org>
> Subject: Re: [PATCH] hw/intc: Handle the error of
>  IOAPICCommonClass.realize()
> 
> 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);

What about I change the name of this interface?

Maybe ioapic_realize(), to distinguish it from DeviceClass.realize().

>      DeviceUnrealize unrealize;

Additionally, if I change the pattern of realize(), should I also avoid
the DeviceUnrealize macro for symmetry's sake and just declare a similar
function pointer as you said?

Further, do you think it's necessary to introduce InternalRealize and
InternalUnrealize macros for qdev to wrap these special realize/unrealize
to differentiate them from normal DeviceRealize/DeviceUnrealize?

Because I found that this pattern of realize() (i.e. registering the
realize() of the child class in the parent class instead of DeviceClass,
and then calling the registered realize() in parent realize()) is also
widely used in many cases:

* xen_block_realize()
* virtser_port_device_realize()
* x86_iommu_realize()
* virtio_input_device_realize()
* apic_common_realize()
* pc_dimm_realize()
* virtio_device_realize()
...

I'm not quite sure if this is a generic way to use it, although it looks
like it could easily be confused with DeviceClass.realize().

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

I'm OK with the change here, but not sure if the return of private
realize() should be changed elsewhere as well.

Thanks,
Zhao

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

Posted by Markus Armbruster 9 months, 3 weeks ago

Zhao Liu <zhao1.liu@linux.intel.com> writes:

> Hi Philippe,
>
> On Wed, Jan 31, 2024 at 05:48:24PM +0100, Philippe Mathieu-Daudé wrote:
>> Date: Wed, 31 Jan 2024 17:48:24 +0100
>> From: Philippe Mathieu-Daudé <philmd@linaro.org>
>> Subject: Re: [PATCH] hw/intc: Handle the error of  IOAPICCommonClass.realize()
>> 
>> 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;
>> > +    }

This is wrong, although it'll work in practice.

It's wrong, because dereferencing @errp requires ERRP_GUARD().
qapi/error.h:

 * = Why, when and how to use ERRP_GUARD() =
 *
 * Without ERRP_GUARD(), use of the @errp parameter is restricted:
 * - It must not be dereferenced, because it may be null.
 * - It should not be passed to error_prepend() or
 *   error_append_hint(), because that doesn't work with &error_fatal.
 * ERRP_GUARD() lifts these restrictions.
 *
 * To use ERRP_GUARD(), add it right at the beginning of the function.
 * @errp can then be used without worrying about the argument being
 * NULL or &error_fatal.
 *
 * Using it when it's not needed is safe, but please avoid cluttering
 * the source with useless code.

It'll work anyway, because the caller never passes null.

Obvious fix:

diff --git a/hw/intc/ioapic_common.c b/hw/intc/ioapic_common.c
index cb9bf62146..280404cba5 100644
--- a/hw/intc/ioapic_common.c
+++ b/hw/intc/ioapic_common.c
@@ -152,6 +152,7 @@ static int ioapic_dispatch_post_load(void *opaque, int version_id)

 static void ioapic_common_realize(DeviceState *dev, Error **errp)
 {
+    ERRP_GUARD();
     IOAPICCommonState *s = IOAPIC_COMMON(dev);
     IOAPICCommonClass *info;

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

qapi.error.h advises:

 * - Whenever practical, also return a value that indicates success /
 *   failure.  This can make the error checking more concise, and can
 *   avoid useless error object creation and destruction.  Note that
 *   we still have many functions returning void.  We recommend
 *   • bool-valued functions return true on success / false on failure,
 *   • pointer-valued functions return non-null / null pointer, and
 *   • integer-valued functions return non-negative / negative.

The patch then becomes

          info = IOAPIC_COMMON_GET_CLASS(s);
     -    info->realize(dev, errp);
     +    if (!info->realize(dev, errp) {
     +        return;
     +    }

DeviceClass and BusClass callbacks realize, unrealize ignore this
advice: they return void.  Why?

Following the advice makes calls easier to read, but the callees have to
do a tiny bit of extra work: return something.  Good trade when we have
at least as many callers as callees.

But these callbacks have many more callees: many devices implement them,
but only a few places call.  Changing them to return something looked
like more trouble than it's worth, so we didn't.

> What about I change the name of this interface?
>
> Maybe ioapic_realize(), to distinguish it from DeviceClass.realize().

I wouldn't bother.

>>      DeviceUnrealize unrealize;
>
> Additionally, if I change the pattern of realize(), should I also avoid
> the DeviceUnrealize macro for symmetry's sake and just declare a similar
> function pointer as you said?
>
> Further, do you think it's necessary to introduce InternalRealize and
> InternalUnrealize macros for qdev

You mean typedefs?

>                          for qdev to wrap these special realize/unrealize
> to differentiate them from normal DeviceRealize/DeviceUnrealize?
>
> Because I found that this pattern of realize() (i.e. registering the
> realize() of the child class in the parent class instead of DeviceClass,
> and then calling the registered realize() in parent realize()) is also
> widely used in many cases:
>
> * xen_block_realize()
> * virtser_port_device_realize()
> * x86_iommu_realize()
> * virtio_input_device_realize()
> * apic_common_realize()
> * pc_dimm_realize()
> * virtio_device_realize()
> ...

Yes.

When a subtype overrides a supertype's method, it often makes sense to
have the subtype's method call the supertype's method.

> I'm not quite sure if this is a generic way to use it, although it looks
> like it could easily be confused with DeviceClass.realize().

Did I answer your question?  I'm not sure :)

>> 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?
>
> I'm OK with the change here, but not sure if the return of private
> realize() should be changed elsewhere as well.

I think I'd add the missing ERRP_GUARD() and call it a day.

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

Posted by Zhao Liu 9 months, 3 weeks ago

Hi Markus,

On Wed, Feb 07, 2024 at 07:51:52AM +0100, Markus Armbruster wrote:
> Date: Wed, 07 Feb 2024 07:51:52 +0100
> From: Markus Armbruster <armbru@redhat.com>
> Subject: Re: [PATCH] hw/intc: Handle the error of
>  IOAPICCommonClass.realize()
> 
> Zhao Liu <zhao1.liu@linux.intel.com> writes:
> 
> > Hi Philippe,
> >
> > On Wed, Jan 31, 2024 at 05:48:24PM +0100, Philippe Mathieu-Daudé wrote:
> >> Date: Wed, 31 Jan 2024 17:48:24 +0100
> >> From: Philippe Mathieu-Daudé <philmd@linaro.org>
> >> Subject: Re: [PATCH] hw/intc: Handle the error of  IOAPICCommonClass.realize()
> >> 
> >> 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;
> >> > +    }
> 
> This is wrong, although it'll work in practice.
> 
> It's wrong, because dereferencing @errp requires ERRP_GUARD().
> qapi/error.h:
> 
>  * = Why, when and how to use ERRP_GUARD() =
>  *
>  * Without ERRP_GUARD(), use of the @errp parameter is restricted:
>  * - It must not be dereferenced, because it may be null.
>  * - It should not be passed to error_prepend() or
>  *   error_append_hint(), because that doesn't work with &error_fatal.
>  * ERRP_GUARD() lifts these restrictions.
>  *
>  * To use ERRP_GUARD(), add it right at the beginning of the function.
>  * @errp can then be used without worrying about the argument being
>  * NULL or &error_fatal.
>  *
>  * Using it when it's not needed is safe, but please avoid cluttering
>  * the source with useless code.
> 
> It'll work anyway, because the caller never passes null.
> 
> Obvious fix:
> 
> diff --git a/hw/intc/ioapic_common.c b/hw/intc/ioapic_common.c
> index cb9bf62146..280404cba5 100644
> --- a/hw/intc/ioapic_common.c
> +++ b/hw/intc/ioapic_common.c
> @@ -152,6 +152,7 @@ static int ioapic_dispatch_post_load(void *opaque, int version_id)
>  
>  static void ioapic_common_realize(DeviceState *dev, Error **errp)
>  {
> +    ERRP_GUARD();
>      IOAPICCommonState *s = IOAPIC_COMMON(dev);
>      IOAPICCommonClass *info;

Thanks for explaining and educating me!

>  
> >> 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);
> 
> qapi.error.h advises:
> 
>  * - Whenever practical, also return a value that indicates success /
>  *   failure.  This can make the error checking more concise, and can
>  *   avoid useless error object creation and destruction.  Note that
>  *   we still have many functions returning void.  We recommend
>  *   • bool-valued functions return true on success / false on failure,
>  *   • pointer-valued functions return non-null / null pointer, and
>  *   • integer-valued functions return non-negative / negative.
> 
> The patch then becomes
> 
>           info = IOAPIC_COMMON_GET_CLASS(s);
>      -    info->realize(dev, errp);
>      +    if (!info->realize(dev, errp) {
>      +        return;
>      +    }
> 
> DeviceClass and BusClass callbacks realize, unrealize ignore this
> advice: they return void.  Why?
> 
> Following the advice makes calls easier to read, but the callees have to
> do a tiny bit of extra work: return something.  Good trade when we have
> at least as many callers as callees.
> 
> But these callbacks have many more callees: many devices implement them,
> but only a few places call.  Changing them to return something looked
> like more trouble than it's worth, so we didn't.

Thanks! Got it.

> 
> > What about I change the name of this interface?
> >
> > Maybe ioapic_realize(), to distinguish it from DeviceClass.realize().
> 
> I wouldn't bother.

;-)

> 
> >>      DeviceUnrealize unrealize;
> >
> > Additionally, if I change the pattern of realize(), should I also avoid
> > the DeviceUnrealize macro for symmetry's sake and just declare a similar
> > function pointer as you said?
> >
> > Further, do you think it's necessary to introduce InternalRealize and
> > InternalUnrealize macros for qdev
> 
> You mean typedefs?

Yes!

> 
> >                          for qdev to wrap these special realize/unrealize
> > to differentiate them from normal DeviceRealize/DeviceUnrealize?
> >
> > Because I found that this pattern of realize() (i.e. registering the
> > realize() of the child class in the parent class instead of DeviceClass,
> > and then calling the registered realize() in parent realize()) is also
> > widely used in many cases:
> >
> > * xen_block_realize()
> > * virtser_port_device_realize()
> > * x86_iommu_realize()
> > * virtio_input_device_realize()
> > * apic_common_realize()
> > * pc_dimm_realize()
> > * virtio_device_realize()
> > ...
> 
> Yes.
> 
> When a subtype overrides a supertype's method, it often makes sense to
> have the subtype's method call the supertype's method.

Thanks! I can consider a simple RFC to introduce a new typedef.

> 
> > I'm not quite sure if this is a generic way to use it, although it looks
> > like it could easily be confused with DeviceClass.realize().
> 
> Did I answer your question?  I'm not sure :)

Of course you did. :-)

> 
> >> 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?
> >
> > I'm OK with the change here, but not sure if the return of private
> > realize() should be changed elsewhere as well.
> 
> I think I'd add the missing ERRP_GUARD() and call it a day.
> 

Okay, let me add the missing ERRP_GUARD(). I'll also check and clean up
other place where the ERRP_GUARD() is also missing.

Regards,
Zhao

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

Posted by Zhao Liu 9 months, 3 weeks ago

Ping Philippe & Markus,

Do you have furthur comment on such private realize()? ;-)

Thanks,
Zhao

On Thu, Feb 01, 2024 at 11:25:56AM +0800, Zhao Liu wrote:
> Date: Thu, 1 Feb 2024 11:25:56 +0800
> From: Zhao Liu <zhao1.liu@linux.intel.com>
> Subject: Re: [PATCH] hw/intc: Handle the error of
>  IOAPICCommonClass.realize()
> 
> Hi Philippe,
> 
> On Wed, Jan 31, 2024 at 05:48:24PM +0100, Philippe Mathieu-Daud? wrote:
> > Date: Wed, 31 Jan 2024 17:48:24 +0100
> > From: Philippe Mathieu-Daud? <philmd@linaro.org>
> > Subject: Re: [PATCH] hw/intc: Handle the error of
> >  IOAPICCommonClass.realize()
> > 
> > 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);
> 
> What about I change the name of this interface?
> 
> Maybe ioapic_realize(), to distinguish it from DeviceClass.realize().
> 
> >      DeviceUnrealize unrealize;
> 
> Additionally, if I change the pattern of realize(), should I also avoid
> the DeviceUnrealize macro for symmetry's sake and just declare a similar
> function pointer as you said?
> 
> Further, do you think it's necessary to introduce InternalRealize and
> InternalUnrealize macros for qdev to wrap these special realize/unrealize
> to differentiate them from normal DeviceRealize/DeviceUnrealize?
> 
> Because I found that this pattern of realize() (i.e. registering the
> realize() of the child class in the parent class instead of DeviceClass,
> and then calling the registered realize() in parent realize()) is also
> widely used in many cases:
> 
> * xen_block_realize()
> * virtser_port_device_realize()
> * x86_iommu_realize()
> * virtio_input_device_realize()
> * apic_common_realize()
> * pc_dimm_realize()
> * virtio_device_realize()
> ...
> 
> I'm not quite sure if this is a generic way to use it, although it looks
> like it could easily be confused with DeviceClass.realize().
> 
> > 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?
> 
> I'm OK with the change here, but not sure if the return of private
> realize() should be changed elsewhere as well.
> 
> Thanks,
> Zhao
> 
>