Zhao Liu <zhao1....@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é <phi...@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....@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....@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.
