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 <arm...@redhat.com> > Subject: Re: [PATCH] hw/intc: Handle the error of > IOAPICCommonClass.realize() > > 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;
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