| Message ID | 20240229143914.1977550-6-zhao1.liu@linux.intel.com |
|---|---|
| State | New |
| Headers | show |
| Series | Cleanup up to fix missing ERRP_GUARD() for error_prepend() | expand |
On 29/02/2024 15.39, Zhao Liu wrote: > From: Zhao Liu <zhao1.liu@intel.com> > > As the comment in qapi/error, passing @errp to error_prepend() requires > ERRP_GUARD(): > > * = Why, when and how to use ERRP_GUARD() = > * > * Without ERRP_GUARD(), use of the @errp parameter is restricted: > ... > * - It should not be passed to error_prepend(), error_vprepend() 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. > > ERRP_GUARD() could avoid the case when @errp is the pointer of > error_fatal, the user can't see this additional information, because > exit() happens in error_setg earlier than information is added [1]. > > The vfio_ap_realize() passes @errp to error_prepend(), and as a > DeviceClass.realize method, its @errp is so widely sourced that it is > necessary to protect it with ERRP_GUARD(). > > To avoid the issue like [1] said, add missing ERRP_GUARD() at the > beginning of this function. > > [1]: Issue description in the commit message of commit ae7c80a7bd73 > ("error: New macro ERRP_GUARD()"). > > Cc: Alex Williamson <alex.williamson@redhat.com> > Cc: "Cédric Le Goater" <clg@redhat.com> > Cc: Thomas Huth <thuth@redhat.com> > Cc: Tony Krowiak <akrowiak@linux.ibm.com> > Cc: Halil Pasic <pasic@linux.ibm.com> > Cc: Jason Herne <jjherne@linux.ibm.com> > Signed-off-by: Zhao Liu <zhao1.liu@intel.com> > --- > hw/vfio/ap.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/hw/vfio/ap.c b/hw/vfio/ap.c > index e157aa1ff79c..7c4caa593863 100644 > --- a/hw/vfio/ap.c > +++ b/hw/vfio/ap.c > @@ -155,6 +155,7 @@ static void vfio_ap_unregister_irq_notifier(VFIOAPDevice *vapdev, > > static void vfio_ap_realize(DeviceState *dev, Error **errp) > { > + ERRP_GUARD(); > int ret; > Error *err = NULL; Now this function looks like we need both, ERRP_GUARD and the local "err" variable? ... patch looks ok to me, but maybe Markus has an idea how this could be done in a nicer way? Thomas
On 2/29/24 12:30 PM, Thomas Huth wrote: > On 29/02/2024 15.39, Zhao Liu wrote: >> From: Zhao Liu <zhao1.liu@intel.com> >> >> As the comment in qapi/error, passing @errp to error_prepend() requires >> ERRP_GUARD(): >> >> * = Why, when and how to use ERRP_GUARD() = >> * >> * Without ERRP_GUARD(), use of the @errp parameter is restricted: >> ... >> * - It should not be passed to error_prepend(), error_vprepend() 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. >> >> ERRP_GUARD() could avoid the case when @errp is the pointer of >> error_fatal, the user can't see this additional information, because >> exit() happens in error_setg earlier than information is added [1]. >> >> The vfio_ap_realize() passes @errp to error_prepend(), and as a >> DeviceClass.realize method, its @errp is so widely sourced that it is >> necessary to protect it with ERRP_GUARD(). >> >> To avoid the issue like [1] said, add missing ERRP_GUARD() at the >> beginning of this function. >> >> [1]: Issue description in the commit message of commit ae7c80a7bd73 >> ("error: New macro ERRP_GUARD()"). >> >> Cc: Alex Williamson <alex.williamson@redhat.com> >> Cc: "Cédric Le Goater" <clg@redhat.com> >> Cc: Thomas Huth <thuth@redhat.com> >> Cc: Tony Krowiak <akrowiak@linux.ibm.com> >> Cc: Halil Pasic <pasic@linux.ibm.com> >> Cc: Jason Herne <jjherne@linux.ibm.com> >> Signed-off-by: Zhao Liu <zhao1.liu@intel.com> >> --- >> hw/vfio/ap.c | 1 + >> 1 file changed, 1 insertion(+) >> >> diff --git a/hw/vfio/ap.c b/hw/vfio/ap.c >> index e157aa1ff79c..7c4caa593863 100644 >> --- a/hw/vfio/ap.c >> +++ b/hw/vfio/ap.c >> @@ -155,6 +155,7 @@ static void >> vfio_ap_unregister_irq_notifier(VFIOAPDevice *vapdev, >> static void vfio_ap_realize(DeviceState *dev, Error **errp) >> { >> + ERRP_GUARD(); >> int ret; >> Error *err = NULL; > > Now this function looks like we need both, ERRP_GUARD and the local > "err" variable? ... patch looks ok to me, but maybe Markus has an idea > how this could be done in a nicer way? Correct me if I'm wrong, but my understanding from reading the prologue in error.h is that errp is used to pass errors back to the caller. The 'err' variable is used to report errors set by a call to the vfio_ap_register_irq_notification function after which this function returns cleanly. It does seem, however, that this function should return a value (possibly a boolean?) for the cases where errp is passed to a function that sets an error to be propagated to the caller. > > Thomas >
On 04/03/2024 16.12, Anthony Krowiak wrote: > > On 2/29/24 12:30 PM, Thomas Huth wrote: >> On 29/02/2024 15.39, Zhao Liu wrote: >>> From: Zhao Liu <zhao1.liu@intel.com> >>> >>> As the comment in qapi/error, passing @errp to error_prepend() requires >>> ERRP_GUARD(): >>> >>> * = Why, when and how to use ERRP_GUARD() = >>> * >>> * Without ERRP_GUARD(), use of the @errp parameter is restricted: >>> ... >>> * - It should not be passed to error_prepend(), error_vprepend() 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. >>> >>> ERRP_GUARD() could avoid the case when @errp is the pointer of >>> error_fatal, the user can't see this additional information, because >>> exit() happens in error_setg earlier than information is added [1]. >>> >>> The vfio_ap_realize() passes @errp to error_prepend(), and as a >>> DeviceClass.realize method, its @errp is so widely sourced that it is >>> necessary to protect it with ERRP_GUARD(). >>> >>> To avoid the issue like [1] said, add missing ERRP_GUARD() at the >>> beginning of this function. >>> >>> [1]: Issue description in the commit message of commit ae7c80a7bd73 >>> ("error: New macro ERRP_GUARD()"). >>> >>> Cc: Alex Williamson <alex.williamson@redhat.com> >>> Cc: "Cédric Le Goater" <clg@redhat.com> >>> Cc: Thomas Huth <thuth@redhat.com> >>> Cc: Tony Krowiak <akrowiak@linux.ibm.com> >>> Cc: Halil Pasic <pasic@linux.ibm.com> >>> Cc: Jason Herne <jjherne@linux.ibm.com> >>> Signed-off-by: Zhao Liu <zhao1.liu@intel.com> >>> --- >>> hw/vfio/ap.c | 1 + >>> 1 file changed, 1 insertion(+) >>> >>> diff --git a/hw/vfio/ap.c b/hw/vfio/ap.c >>> index e157aa1ff79c..7c4caa593863 100644 >>> --- a/hw/vfio/ap.c >>> +++ b/hw/vfio/ap.c >>> @@ -155,6 +155,7 @@ static void >>> vfio_ap_unregister_irq_notifier(VFIOAPDevice *vapdev, >>> static void vfio_ap_realize(DeviceState *dev, Error **errp) >>> { >>> + ERRP_GUARD(); >>> int ret; >>> Error *err = NULL; >> >> Now this function looks like we need both, ERRP_GUARD and the local "err" >> variable? ... patch looks ok to me, but maybe Markus has an idea how this >> could be done in a nicer way? > > > Correct me if I'm wrong, but my understanding from reading the prologue in > error.h is that errp is used to pass errors back to the caller. The 'err' > variable is used to report errors set by a call to the > vfio_ap_register_irq_notification function after which this function returns > cleanly. Right, no objections, that's what I meant with "this function looks like we need both" ... But having both, "err" and "errp" in one function also looks somewhat confusing at a first glance. No clue how this could be done much better though, maybe rename "err" to "local_err" to make it clear that the two variables are used independently? Thomas
Hi Thomas, (and also ping Markus) ;-) > > > > vfio_ap_unregister_irq_notifier(VFIOAPDevice *vapdev, > > > > static void vfio_ap_realize(DeviceState *dev, Error **errp) > > > > { > > > > + ERRP_GUARD(); > > > > int ret; > > > > Error *err = NULL; > > > > > > Now this function looks like we need both, ERRP_GUARD and the local > > > "err" variable? ... patch looks ok to me, but maybe Markus has an > > > idea how this could be done in a nicer way? > > > > > > Correct me if I'm wrong, but my understanding from reading the prologue > > in error.h is that errp is used to pass errors back to the caller. The > > 'err' variable is used to report errors set by a call to the > > vfio_ap_register_irq_notification function after which this function > > returns cleanly. > > Right, no objections, that's what I meant with "this function looks like we > need both" ... > But having both, "err" and "errp" in one function also looks somewhat > confusing at a first glance. No clue how this could be done much better > though, maybe rename "err" to "local_err" to make it clear that the two > variables are used independently? > I agree, the "local_err" is a better name and it seems we can't get rid of this local varibale. (I can also cleanup this in the follow-up series). From other use cases, @err is usually used to play with @errp and @local_err is (sometimes) used for local error handling. I'm also unsure if this variable naming is a convention of error handling though... Markus, what do you think about this understanding? Thanks, Zhao
diff --git a/hw/vfio/ap.c b/hw/vfio/ap.c index e157aa1ff79c..7c4caa593863 100644 --- a/hw/vfio/ap.c +++ b/hw/vfio/ap.c @@ -155,6 +155,7 @@ static void vfio_ap_unregister_irq_notifier(VFIOAPDevice *vapdev, static void vfio_ap_realize(DeviceState *dev, Error **errp) { + ERRP_GUARD(); int ret; Error *err = NULL; VFIOAPDevice *vapdev = VFIO_AP_DEVICE(dev);