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

[Thomas Huth <thuth@redhat.com>]

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