qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Anthony Krowiak
Subject: Re: [PATCH 05/17] hw/vfio/ap: Fix missing ERRP_GUARD() for error_prepend()
Date: Mon, 4 Mar 2024 10:12:38 -0500
User-agent: Mozilla Thunderbird


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




reply via email to

[Prev in Thread] Current Thread [Next in Thread]