* [PATCH] hw/intc: Handle the error of IOAPICCommonClass.realize() @ 2024-01-31 14:29 Zhao Liu 2024-01-31 16:48 ` Philippe Mathieu-Daudé 0 siblings, 1 reply; 6+ messages in thread From: Zhao Liu @ 2024-01-31 14:29 UTC (permalink / raw) To: Michael S . Tsirkin, Paolo Bonzini; +Cc: qemu-devel, qemu-trivial, Zhao Liu 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 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] hw/intc: Handle the error of IOAPICCommonClass.realize() 2024-01-31 14:29 [PATCH] hw/intc: Handle the error of IOAPICCommonClass.realize() Zhao Liu @ 2024-01-31 16:48 ` Philippe Mathieu-Daudé 2024-02-01 3:25 ` Zhao Liu 0 siblings, 1 reply; 6+ messages in thread From: Philippe Mathieu-Daudé @ 2024-01-31 16:48 UTC (permalink / raw) To: Zhao Liu, Michael S . Tsirkin, Paolo Bonzini Cc: qemu-devel, qemu-trivial, Zhao Liu, Markus Armbruster 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? ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] hw/intc: Handle the error of IOAPICCommonClass.realize() 2024-01-31 16:48 ` Philippe Mathieu-Daudé @ 2024-02-01 3:25 ` Zhao Liu 2024-02-06 16:08 ` Zhao Liu 2024-02-07 6:51 ` Markus Armbruster 0 siblings, 2 replies; 6+ messages in thread From: Zhao Liu @ 2024-02-01 3:25 UTC (permalink / raw) To: Philippe Mathieu-Daudé Cc: Michael S . Tsirkin, Paolo Bonzini, qemu-devel, qemu-trivial, Zhao Liu, Markus Armbruster 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] hw/intc: Handle the error of IOAPICCommonClass.realize() 2024-02-01 3:25 ` Zhao Liu @ 2024-02-06 16:08 ` Zhao Liu 2024-02-07 6:51 ` Markus Armbruster 1 sibling, 0 replies; 6+ messages in thread From: Zhao Liu @ 2024-02-06 16:08 UTC (permalink / raw) To: Philippe Mathieu-Daud� Cc: Michael S . Tsirkin, Paolo Bonzini, qemu-devel, qemu-trivial, Zhao Liu, Markus Armbruster 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 > > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] hw/intc: Handle the error of IOAPICCommonClass.realize() 2024-02-01 3:25 ` Zhao Liu 2024-02-06 16:08 ` Zhao Liu @ 2024-02-07 6:51 ` Markus Armbruster 2024-02-07 16:37 ` Zhao Liu 1 sibling, 1 reply; 6+ messages in thread From: Markus Armbruster @ 2024-02-07 6:51 UTC (permalink / raw) To: Zhao Liu Cc: Philippe Mathieu-Daudé, Michael S . Tsirkin, Paolo Bonzini, qemu-devel, qemu-trivial, Zhao Liu 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. ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] hw/intc: Handle the error of IOAPICCommonClass.realize() 2024-02-07 6:51 ` Markus Armbruster @ 2024-02-07 16:37 ` Zhao Liu 0 siblings, 0 replies; 6+ messages in thread From: Zhao Liu @ 2024-02-07 16:37 UTC (permalink / raw) To: Markus Armbruster Cc: Philippe Mathieu-Daudé, Michael S . Tsirkin, Paolo Bonzini, qemu-devel, qemu-trivial, Zhao Liu 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2024-02-07 16:24 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2024-01-31 14:29 [PATCH] hw/intc: Handle the error of IOAPICCommonClass.realize() Zhao Liu 2024-01-31 16:48 ` Philippe Mathieu-Daudé 2024-02-01 3:25 ` Zhao Liu 2024-02-06 16:08 ` Zhao Liu 2024-02-07 6:51 ` Markus Armbruster 2024-02-07 16:37 ` Zhao Liu
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).