[PATCH 05/17] hw/vfio/ap: Fix missing ERRP_GUARD() for error_prepend()

Zhao Liu posted 17 patches 9 months ago
Maintainers: Stefano Stabellini <sstabellini@kernel.org>, Anthony Perard <anthony.perard@citrix.com>, Paul Durrant <paul@xen.org>, Jason Wang <jasowang@redhat.com>, Elena Ufimtseva <elena.ufimtseva@oracle.com>, Jagannathan Raman <jag.raman@oracle.com>, "Michael S. Tsirkin" <mst@redhat.com>, Paolo Bonzini <pbonzini@redhat.com>, Fam Zheng <fam@euphon.net>, Alex Williamson <alex.williamson@redhat.com>, "Cédric Le Goater" <clg@redhat.com>, Tony Krowiak <akrowiak@linux.ibm.com>, Halil Pasic <pasic@linux.ibm.com>, Jason Herne <jjherne@linux.ibm.com>, Thomas Huth <thuth@redhat.com>, Peter Xu <peterx@redhat.com>, Fabiano Rosas <farosas@suse.de>, Marcelo Tosatti <mtosatti@redhat.com>, David Hildenbrand <david@redhat.com>, Richard Henderson <richard.henderson@linaro.org>, Ilya Leoshkevich <iii@linux.ibm.com>
There is a newer version of this series
[PATCH 05/17] hw/vfio/ap: Fix missing ERRP_GUARD() for error_prepend()
Posted by Zhao Liu 9 months ago
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;
     VFIOAPDevice *vapdev = VFIO_AP_DEVICE(dev);
-- 
2.34.1


Re: [PATCH 05/17] hw/vfio/ap: Fix missing ERRP_GUARD() for error_prepend()
Posted by Thomas Huth 9 months ago
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


Re: [PATCH 05/17] hw/vfio/ap: Fix missing ERRP_GUARD() for error_prepend()
Posted by Anthony Krowiak 8 months, 3 weeks ago
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
>

Re: [PATCH 05/17] hw/vfio/ap: Fix missing ERRP_GUARD() for error_prepend()
Posted by Thomas Huth 8 months, 3 weeks ago
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


Re: [PATCH 05/17] hw/vfio/ap: Fix missing ERRP_GUARD() for error_prepend()
Posted by Zhao Liu 8 months, 3 weeks ago
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