[PATCH 00/17 Part 2] Cleanup up to fix missing ERRP_GUARD() for error_prepend()

[PATCH 00/17 Part 2] Cleanup up to fix missing ERRP_GUARD() for error_prepend()

[Zhao Liu]

From: Zhao Liu <zhao1.liu@intel.com>

Hi list,

This series is the second part to add missing ERRP_GUARD() for
error_prepend(), which is based on my first part:
<20240228163723.1775791-1-zhao1.liu@linux.intel.com> [1]

The @errp's second restriction (in qapi/error) said:

* Without ERRP_GUARD(), use of the @errp parameter is restricted:
...
* - 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.

With this series and the part 1 [1], all error_prepend() will follow the
above usage rules.

I think it's particularly noteworthy that two cases of error_prepend()
with &error_fatal are recognized in this series (patch 14 & patch 15). 

In fact, error_append_hint() also need ERRP_GUARD(), but very
unfortunately, I realized that in most usage scenarios in QEMU, it
doesn't work with ERRP_GUARD() ;-( ... Maybe need to clean it up in the
future and add ERRP_GUARD() to error_append_hint() as well.

The cleanup looks very trivial and welcome your feedback!

[1]: https://lore.kernel.org/qemu-devel/20240228163723.1775791-1-zhao1.liu@linux.intel.com/

Thanks and Best Regards,
Zhao
---
Zhao Liu (17):
  hw/misc/ivshmem: Fix missing ERRP_GUARD() for error_prepend()
  hw/net/xen_nic: Fix missing ERRP_GUARD() for error_prepend()
  hw/remote/remote-obj: hw/misc/ivshmem: Fix missing ERRP_GUARD() for
    error_prepend()
  hw/scsi/vhost-scsi: Fix missing ERRP_GUARD() for error_prepend()
  hw/vfio/ap: Fix missing ERRP_GUARD() for error_prepend()
  hw/vfio/container: Fix missing ERRP_GUARD() for error_prepend()
  hw/vfio/helpers: Fix missing ERRP_GUARD() for error_prepend()
  hw/vfio/iommufd: Fix missing ERRP_GUARD() for error_prepend()
  hw/vfio/pci-quirks: Fix missing ERRP_GUARD() for error_prepend()
  hw/vfio/pci: Fix missing ERRP_GUARD() for error_prepend()
  hw/vfio/platform: Fix missing ERRP_GUARD() for error_prepend()
  hw/virtio/vhost-vsock: Fix missing ERRP_GUARD() for error_prepend()
  hw/virtio/vhost: Fix missing ERRP_GUARD() for error_prepend()
  migration/option: Fix missing ERRP_GUARD() for error_prepend()
  net/vhost-vdpa: Fix missing ERRP_GUARD() for error_prepend()
  target/i386/sev: Fix missing ERRP_GUARD() for error_prepend()
  target/s390x/cpu_models: Fix missing ERRP_GUARD() for error_prepend()

 hw/misc/ivshmem.c         | 1 +
 hw/net/xen_nic.c          | 1 +
 hw/remote/remote-obj.c    | 1 +
 hw/scsi/vhost-scsi.c      | 1 +
 hw/vfio/ap.c              | 1 +
 hw/vfio/container.c       | 1 +
 hw/vfio/helpers.c         | 3 +++
 hw/vfio/iommufd.c         | 1 +
 hw/vfio/pci-quirks.c      | 2 ++
 hw/vfio/pci.c             | 2 ++
 hw/vfio/platform.c        | 1 +
 hw/virtio/vhost-vsock.c   | 1 +
 hw/virtio/vhost.c         | 2 ++
 migration/options.c       | 2 ++
 net/vhost-vdpa.c          | 1 +
 target/i386/sev.c         | 1 +
 target/s390x/cpu_models.c | 2 ++
 17 files changed, 24 insertions(+)

-- 
2.34.1

[PATCH 01/17] hw/misc/ivshmem: Fix missing ERRP_GUARD() for error_prepend()

[Zhao Liu]

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 ivshmem_common_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: Juan Quintela <quintela@trasno.org>
Cc: Steve Sistare <steven.sistare@oracle.com>
Cc: "Philippe Mathieu-Daudé" <philmd@linaro.org>
Cc: Michael Galaxy <mgalaxy@akamai.com>
Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
---
 hw/misc/ivshmem.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/misc/ivshmem.c b/hw/misc/ivshmem.c
index a2fd0bc36544..de49d1b8a826 100644
--- a/hw/misc/ivshmem.c
+++ b/hw/misc/ivshmem.c
@@ -832,6 +832,7 @@ static void ivshmem_write_config(PCIDevice *pdev, uint32_t address,
 
 static void ivshmem_common_realize(PCIDevice *dev, Error **errp)
 {
+    ERRP_GUARD();
     IVShmemState *s = IVSHMEM_COMMON(dev);
     Error *err = NULL;
     uint8_t *pci_conf;
-- 
2.34.1

Re: [PATCH 01/17] hw/misc/ivshmem: Fix missing ERRP_GUARD() for error_prepend()

[Thomas Huth]

On 29/02/2024 15.38, 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 ivshmem_common_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: Juan Quintela <quintela@trasno.org>
> Cc: Steve Sistare <steven.sistare@oracle.com>
> Cc: "Philippe Mathieu-Daudé" <philmd@linaro.org>
> Cc: Michael Galaxy <mgalaxy@akamai.com>
> Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
> ---
>   hw/misc/ivshmem.c | 1 +
>   1 file changed, 1 insertion(+)
> 
> diff --git a/hw/misc/ivshmem.c b/hw/misc/ivshmem.c
> index a2fd0bc36544..de49d1b8a826 100644
> --- a/hw/misc/ivshmem.c
> +++ b/hw/misc/ivshmem.c
> @@ -832,6 +832,7 @@ static void ivshmem_write_config(PCIDevice *pdev, uint32_t address,
>   
>   static void ivshmem_common_realize(PCIDevice *dev, Error **errp)
>   {
> +    ERRP_GUARD();
>       IVShmemState *s = IVSHMEM_COMMON(dev);
>       Error *err = NULL;

Please remove "err" and the error_propagate in here now.

  Thomas

Re: [PATCH 01/17] hw/misc/ivshmem: Fix missing ERRP_GUARD() for error_prepend()

[Zhao Liu]

> > @@ -832,6 +832,7 @@ static void ivshmem_write_config(PCIDevice *pdev, uint32_t address,
> >   static void ivshmem_common_realize(PCIDevice *dev, Error **errp)
> >   {
> > +    ERRP_GUARD();
> >       IVShmemState *s = IVSHMEM_COMMON(dev);
> >       Error *err = NULL;
> 
> Please remove "err" and the error_propagate in here now.

OK, I hadn't thought of that. Thanks.

Regards,
Zhao

[PATCH 02/17] hw/net/xen_nic: Fix missing ERRP_GUARD() for error_prepend()

[Zhao Liu]

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 xen_netdev_connect() passes @errp to error_prepend(), and its @errp
parameter is from xen_device_frontend_changed().

Though its @errp points to @local_err of xen_device_frontend_changed(),
to follow the requirement of @errp, 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: Stefano Stabellini <sstabellini@kernel.org>
Cc: Anthony Perard <anthony.perard@citrix.com>
Cc: Paul Durrant <paul@xen.org>
Cc: Jason Wang <jasowang@redhat.com>
Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
---
 hw/net/xen_nic.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/net/xen_nic.c b/hw/net/xen_nic.c
index 453fdb981983..89487b49baf9 100644
--- a/hw/net/xen_nic.c
+++ b/hw/net/xen_nic.c
@@ -351,6 +351,7 @@ static bool net_event(void *_xendev)
 
 static bool xen_netdev_connect(XenDevice *xendev, Error **errp)
 {
+    ERRP_GUARD();
     XenNetDev *netdev = XEN_NET_DEVICE(xendev);
     unsigned int port, rx_copy;
 
-- 
2.34.1

Re: [PATCH 02/17] hw/net/xen_nic: Fix missing ERRP_GUARD() for error_prepend()

[Thomas Huth]

On 29/02/2024 15.38, 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 xen_netdev_connect() passes @errp to error_prepend(), and its @errp
> parameter is from xen_device_frontend_changed().
> 
> Though its @errp points to @local_err of xen_device_frontend_changed(),
> to follow the requirement of @errp, 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: Stefano Stabellini <sstabellini@kernel.org>
> Cc: Anthony Perard <anthony.perard@citrix.com>
> Cc: Paul Durrant <paul@xen.org>
> Cc: Jason Wang <jasowang@redhat.com>
> Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
> ---
>   hw/net/xen_nic.c | 1 +
>   1 file changed, 1 insertion(+)
> 
> diff --git a/hw/net/xen_nic.c b/hw/net/xen_nic.c
> index 453fdb981983..89487b49baf9 100644
> --- a/hw/net/xen_nic.c
> +++ b/hw/net/xen_nic.c
> @@ -351,6 +351,7 @@ static bool net_event(void *_xendev)
>   
>   static bool xen_netdev_connect(XenDevice *xendev, Error **errp)
>   {
> +    ERRP_GUARD();
>       XenNetDev *netdev = XEN_NET_DEVICE(xendev);
>       unsigned int port, rx_copy;
>   

Reviewed-by: Thomas Huth <thuth@redhat.com>

Re: [PATCH 02/17] hw/net/xen_nic: Fix missing ERRP_GUARD() for error_prepend()

[Anthony PERARD]

On Thu, Feb 29, 2024 at 06:25:40PM +0100, Thomas Huth wrote:
> On 29/02/2024 15.38, 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 xen_netdev_connect() passes @errp to error_prepend(), and its @errp
> > parameter is from xen_device_frontend_changed().
> > 
> > Though its @errp points to @local_err of xen_device_frontend_changed(),
> > to follow the requirement of @errp, 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: Stefano Stabellini <sstabellini@kernel.org>
> > Cc: Anthony Perard <anthony.perard@citrix.com>
> > Cc: Paul Durrant <paul@xen.org>
> > Cc: Jason Wang <jasowang@redhat.com>
> > Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
> > ---
> >   hw/net/xen_nic.c | 1 +
> >   1 file changed, 1 insertion(+)
> > 
> > diff --git a/hw/net/xen_nic.c b/hw/net/xen_nic.c
> > index 453fdb981983..89487b49baf9 100644
> > --- a/hw/net/xen_nic.c
> > +++ b/hw/net/xen_nic.c
> > @@ -351,6 +351,7 @@ static bool net_event(void *_xendev)
> >   static bool xen_netdev_connect(XenDevice *xendev, Error **errp)
> >   {
> > +    ERRP_GUARD();
> >       XenNetDev *netdev = XEN_NET_DEVICE(xendev);
> >       unsigned int port, rx_copy;
> 
> Reviewed-by: Thomas Huth <thuth@redhat.com>

Acked-by: Anthony PERARD <anthony.perard@citrix.com>

Thanks,

-- 
Anthony PERARD

[PATCH 03/17] hw/remote/remote-obj: hw/misc/ivshmem: Fix missing ERRP_GUARD() for error_prepend()

[Zhao Liu]

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 remote_object_set_fd() passes @errp to error_prepend(), and as a
PropertyInfo.set 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: Elena Ufimtseva <elena.ufimtseva@oracle.com>
Cc: Jagannathan Raman <jag.raman@oracle.com>
Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
---
 hw/remote/remote-obj.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/remote/remote-obj.c b/hw/remote/remote-obj.c
index 65b6f7cc863f..dc27cc8da1f3 100644
--- a/hw/remote/remote-obj.c
+++ b/hw/remote/remote-obj.c
@@ -49,6 +49,7 @@ struct RemoteObject {
 
 static void remote_object_set_fd(Object *obj, const char *str, Error **errp)
 {
+    ERRP_GUARD();
     RemoteObject *o = REMOTE_OBJECT(obj);
     int fd = -1;
 
-- 
2.34.1

Re: [PATCH 03/17] hw/remote/remote-obj: hw/misc/ivshmem: Fix missing ERRP_GUARD() for error_prepend()

[Thomas Huth]

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 remote_object_set_fd() passes @errp to error_prepend(), and as a
> PropertyInfo.set 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: Elena Ufimtseva <elena.ufimtseva@oracle.com>
> Cc: Jagannathan Raman <jag.raman@oracle.com>
> Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
> ---
>   hw/remote/remote-obj.c | 1 +
>   1 file changed, 1 insertion(+)
> 
> diff --git a/hw/remote/remote-obj.c b/hw/remote/remote-obj.c
> index 65b6f7cc863f..dc27cc8da1f3 100644
> --- a/hw/remote/remote-obj.c
> +++ b/hw/remote/remote-obj.c
> @@ -49,6 +49,7 @@ struct RemoteObject {
>   
>   static void remote_object_set_fd(Object *obj, const char *str, Error **errp)
>   {
> +    ERRP_GUARD();
>       RemoteObject *o = REMOTE_OBJECT(obj);
>       int fd = -1;
>   

Reviewed-by: Thomas Huth <thuth@redhat.com>

[PATCH 04/17] hw/scsi/vhost-scsi: Fix missing ERRP_GUARD() for error_prepend()

[Zhao Liu]

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 vhost_scsi_realize() passes @errp to error_prepend(), and as a
VirtioDeviceClass.realize method, its @errp is from DeviceClass.realize
so that there is no guarantee that the @errp won't point to
@error_fatal.

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: "Michael S. Tsirkin" <mst@redhat.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Fam Zheng <fam@euphon.net>
Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
---
 hw/scsi/vhost-scsi.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/scsi/vhost-scsi.c b/hw/scsi/vhost-scsi.c
index 58a00336c2db..ae26bc19a457 100644
--- a/hw/scsi/vhost-scsi.c
+++ b/hw/scsi/vhost-scsi.c
@@ -220,6 +220,7 @@ static int vhost_scsi_set_workers(VHostSCSICommon *vsc, bool per_virtqueue)
 
 static void vhost_scsi_realize(DeviceState *dev, Error **errp)
 {
+    ERRP_GUARD();
     VirtIOSCSICommon *vs = VIRTIO_SCSI_COMMON(dev);
     VHostSCSICommon *vsc = VHOST_SCSI_COMMON(dev);
     Error *err = NULL;
-- 
2.34.1

Re: [PATCH 04/17] hw/scsi/vhost-scsi: Fix missing ERRP_GUARD() for error_prepend()

[Thomas Huth]

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 vhost_scsi_realize() passes @errp to error_prepend(), and as a
> VirtioDeviceClass.realize method, its @errp is from DeviceClass.realize
> so that there is no guarantee that the @errp won't point to
> @error_fatal.
> 
> 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: "Michael S. Tsirkin" <mst@redhat.com>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Cc: Fam Zheng <fam@euphon.net>
> Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
> ---
>   hw/scsi/vhost-scsi.c | 1 +
>   1 file changed, 1 insertion(+)
> 
> diff --git a/hw/scsi/vhost-scsi.c b/hw/scsi/vhost-scsi.c
> index 58a00336c2db..ae26bc19a457 100644
> --- a/hw/scsi/vhost-scsi.c
> +++ b/hw/scsi/vhost-scsi.c
> @@ -220,6 +220,7 @@ static int vhost_scsi_set_workers(VHostSCSICommon *vsc, bool per_virtqueue)
>   
>   static void vhost_scsi_realize(DeviceState *dev, Error **errp)
>   {
> +    ERRP_GUARD();
>       VirtIOSCSICommon *vs = VIRTIO_SCSI_COMMON(dev);
>       VHostSCSICommon *vsc = VHOST_SCSI_COMMON(dev);
>       Error *err = NULL;

I think you could remove the "err" variable and the error_propagate stuff in 
here now.

  Thomas

Re: [PATCH 04/17] hw/scsi/vhost-scsi: Fix missing ERRP_GUARD() for error_prepend()

[Zhao Liu]

> > @@ -220,6 +220,7 @@ static int vhost_scsi_set_workers(VHostSCSICommon *vsc, bool per_virtqueue)
> >   static void vhost_scsi_realize(DeviceState *dev, Error **errp)
> >   {
> > +    ERRP_GUARD();
> >       VirtIOSCSICommon *vs = VIRTIO_SCSI_COMMON(dev);
> >       VHostSCSICommon *vsc = VHOST_SCSI_COMMON(dev);
> >       Error *err = NULL;
> 
> I think you could remove the "err" variable and the error_propagate stuff in
> here now.
>

OK, I'll. Thanks.

-Zhao

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

[Zhao Liu]

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()

[Thomas Huth]

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()

[Anthony Krowiak]

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()

[Thomas Huth]

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()

[Zhao Liu]

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

[PATCH 06/17] hw/vfio/container: Fix missing ERRP_GUARD() for error_prepend()

[Zhao Liu]

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_get_group() passes @errp to error_prepend(). Its @errp is
from vfio_attach_device(), which @errp parameter 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>
Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
---
 hw/vfio/container.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/vfio/container.c b/hw/vfio/container.c
index bd25b9fbad2e..f66bb01f5b18 100644
--- a/hw/vfio/container.c
+++ b/hw/vfio/container.c
@@ -719,6 +719,7 @@ static void vfio_disconnect_container(VFIOGroup *group)
 
 static VFIOGroup *vfio_get_group(int groupid, AddressSpace *as, Error **errp)
 {
+    ERRP_GUARD();
     VFIOGroup *group;
     char path[32];
     struct vfio_group_status status = { .argsz = sizeof(status) };
-- 
2.34.1

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

[Thomas Huth]

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_get_group() passes @errp to error_prepend(). Its @errp is
> from vfio_attach_device(), which @errp parameter 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>
> Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
> ---
>   hw/vfio/container.c | 1 +
>   1 file changed, 1 insertion(+)
> 
> diff --git a/hw/vfio/container.c b/hw/vfio/container.c
> index bd25b9fbad2e..f66bb01f5b18 100644
> --- a/hw/vfio/container.c
> +++ b/hw/vfio/container.c
> @@ -719,6 +719,7 @@ static void vfio_disconnect_container(VFIOGroup *group)
>   
>   static VFIOGroup *vfio_get_group(int groupid, AddressSpace *as, Error **errp)
>   {
> +    ERRP_GUARD();
>       VFIOGroup *group;
>       char path[32];
>       struct vfio_group_status status = { .argsz = sizeof(status) };

Reviewed-by: Thomas Huth <thuth@redhat.com>

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

[Cédric Le Goater]

Hello,

On 2/29/24 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_get_group() passes @errp to error_prepend(). Its @errp is
> from vfio_attach_device(), which @errp parameter 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>
> Signed-off-by: Zhao Liu <zhao1.liu@intel.com>


Reviewed-by: Cédric Le Goater <clg@redhat.com>

Thanks,

C.

[PATCH 07/17] hw/vfio/helpers: Fix missing ERRP_GUARD() for error_prepend()

[Zhao Liu]

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].

In hw/vfio/helpers.c, there're 3 functions passing @errp to
error_prepend() without ERRP_GUARD():
 - vfio_set_irq_signaling()
 - vfio_device_get_name()
 - vfio_device_set_fd()

As the widely used helpers, their @errp parameters are so widely sourced
that it is necessary to protect them with ERRP_GUARD().

To avoid the issue like [1] said, add missing ERRP_GUARD() at their
beginning.

[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>
Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
---
 hw/vfio/helpers.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/hw/vfio/helpers.c b/hw/vfio/helpers.c
index 678987080228..47b4096c05ee 100644
--- a/hw/vfio/helpers.c
+++ b/hw/vfio/helpers.c
@@ -110,6 +110,7 @@ static const char *index_to_str(VFIODevice *vbasedev, int index)
 int vfio_set_irq_signaling(VFIODevice *vbasedev, int index, int subindex,
                            int action, int fd, Error **errp)
 {
+    ERRP_GUARD();
     struct vfio_irq_set *irq_set;
     int argsz, ret = 0;
     const char *name;
@@ -613,6 +614,7 @@ bool vfio_has_region_cap(VFIODevice *vbasedev, int region, uint16_t cap_type)
 
 int vfio_device_get_name(VFIODevice *vbasedev, Error **errp)
 {
+    ERRP_GUARD();
     struct stat st;
 
     if (vbasedev->fd < 0) {
@@ -644,6 +646,7 @@ int vfio_device_get_name(VFIODevice *vbasedev, Error **errp)
 
 void vfio_device_set_fd(VFIODevice *vbasedev, const char *str, Error **errp)
 {
+    ERRP_GUARD();
     int fd = monitor_fd_param(monitor_cur(), str, errp);
 
     if (fd < 0) {
-- 
2.34.1

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

[Cédric Le Goater]

On 2/29/24 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].
> 
> In hw/vfio/helpers.c, there're 3 functions passing @errp to
> error_prepend() without ERRP_GUARD():
>   - vfio_set_irq_signaling()
>   - vfio_device_get_name()
>   - vfio_device_set_fd()
> 
> As the widely used helpers, their @errp parameters are so widely sourced
> that it is necessary to protect them with ERRP_GUARD().
> 
> To avoid the issue like [1] said, add missing ERRP_GUARD() at their
> beginning.
> 
> [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>
> Signed-off-by: Zhao Liu <zhao1.liu@intel.com>


Reviewed-by: Cédric Le Goater <clg@redhat.com>

Thanks,

C.


> ---
>   hw/vfio/helpers.c | 3 +++
>   1 file changed, 3 insertions(+)
> 
> diff --git a/hw/vfio/helpers.c b/hw/vfio/helpers.c
> index 678987080228..47b4096c05ee 100644
> --- a/hw/vfio/helpers.c
> +++ b/hw/vfio/helpers.c
> @@ -110,6 +110,7 @@ static const char *index_to_str(VFIODevice *vbasedev, int index)
>   int vfio_set_irq_signaling(VFIODevice *vbasedev, int index, int subindex,
>                              int action, int fd, Error **errp)
>   {
> +    ERRP_GUARD();
>       struct vfio_irq_set *irq_set;
>       int argsz, ret = 0;
>       const char *name;
> @@ -613,6 +614,7 @@ bool vfio_has_region_cap(VFIODevice *vbasedev, int region, uint16_t cap_type)
>   
>   int vfio_device_get_name(VFIODevice *vbasedev, Error **errp)
>   {
> +    ERRP_GUARD();
>       struct stat st;
>   
>       if (vbasedev->fd < 0) {
> @@ -644,6 +646,7 @@ int vfio_device_get_name(VFIODevice *vbasedev, Error **errp)
>   
>   void vfio_device_set_fd(VFIODevice *vbasedev, const char *str, Error **errp)
>   {
> +    ERRP_GUARD();
>       int fd = monitor_fd_param(monitor_cur(), str, errp);
>   
>       if (fd < 0) {

[PATCH 08/17] hw/vfio/iommufd: Fix missing ERRP_GUARD() for error_prepend()

[Zhao Liu]

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 iommufd_cdev_getfd() passes @errp to error_prepend(). Its @errp is
from vfio_attach_device(), which @errp parameter 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>
Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
---
 hw/vfio/iommufd.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/vfio/iommufd.c b/hw/vfio/iommufd.c
index 9bfddc136089..7baf49e6ee9e 100644
--- a/hw/vfio/iommufd.c
+++ b/hw/vfio/iommufd.c
@@ -116,6 +116,7 @@ static void iommufd_cdev_unbind_and_disconnect(VFIODevice *vbasedev)
 
 static int iommufd_cdev_getfd(const char *sysfs_path, Error **errp)
 {
+    ERRP_GUARD();
     long int ret = -ENOTTY;
     char *path, *vfio_dev_path = NULL, *vfio_path = NULL;
     DIR *dir = NULL;
-- 
2.34.1

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

[Cédric Le Goater]

On 2/29/24 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 iommufd_cdev_getfd() passes @errp to error_prepend(). Its @errp is
> from vfio_attach_device(), which @errp parameter 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>
> Signed-off-by: Zhao Liu <zhao1.liu@intel.com>


Reviewed-by: Cédric Le Goater <clg@redhat.com>

Thanks,

C.


> ---
>   hw/vfio/iommufd.c | 1 +
>   1 file changed, 1 insertion(+)
> 
> diff --git a/hw/vfio/iommufd.c b/hw/vfio/iommufd.c
> index 9bfddc136089..7baf49e6ee9e 100644
> --- a/hw/vfio/iommufd.c
> +++ b/hw/vfio/iommufd.c
> @@ -116,6 +116,7 @@ static void iommufd_cdev_unbind_and_disconnect(VFIODevice *vbasedev)
>   
>   static int iommufd_cdev_getfd(const char *sysfs_path, Error **errp)
>   {
> +    ERRP_GUARD();
>       long int ret = -ENOTTY;
>       char *path, *vfio_dev_path = NULL, *vfio_path = NULL;
>       DIR *dir = NULL;

[PATCH 09/17] hw/vfio/pci-quirks: Fix missing ERRP_GUARD() for error_prepend()

[Zhao Liu]

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].

In hw/vfio/pci-quirks.c, there're 2 functions passing @errp to
error_prepend() without ERRP_GUARD():
- vfio_add_nv_gpudirect_cap()
- vfio_add_vmd_shadow_cap()

Their @errp parameters are so widely sourced that it is necessary to
protect them 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>
Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
---
 hw/vfio/pci-quirks.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/hw/vfio/pci-quirks.c b/hw/vfio/pci-quirks.c
index 84b1a7b9485c..496fd1ee86bd 100644
--- a/hw/vfio/pci-quirks.c
+++ b/hw/vfio/pci-quirks.c
@@ -1538,6 +1538,7 @@ static bool is_valid_std_cap_offset(uint8_t pos)
 
 static int vfio_add_nv_gpudirect_cap(VFIOPCIDevice *vdev, Error **errp)
 {
+    ERRP_GUARD();
     PCIDevice *pdev = &vdev->pdev;
     int ret, pos;
     bool c8_conflict = false, d4_conflict = false;
@@ -1630,6 +1631,7 @@ static int vfio_add_nv_gpudirect_cap(VFIOPCIDevice *vdev, Error **errp)
 #define VMD_SHADOW_CAP_LEN 24
 static int vfio_add_vmd_shadow_cap(VFIOPCIDevice *vdev, Error **errp)
 {
+    ERRP_GUARD();
     uint8_t membar_phys[16];
     int ret, pos = 0xE8;
 
-- 
2.34.1

Re: [PATCH 09/17] hw/vfio/pci-quirks: Fix missing ERRP_GUARD() for error_prepend()

[Cédric Le Goater]

On 2/29/24 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].
> 
> In hw/vfio/pci-quirks.c, there're 2 functions passing @errp to
> error_prepend() without ERRP_GUARD():
> - vfio_add_nv_gpudirect_cap()
> - vfio_add_vmd_shadow_cap()
> 
> Their @errp parameters are so widely sourced that it is necessary to
> protect them 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>
> Signed-off-by: Zhao Liu <zhao1.liu@intel.com>


Reviewed-by: Cédric Le Goater <clg@redhat.com>

Thanks,

C.


> ---
>   hw/vfio/pci-quirks.c | 2 ++
>   1 file changed, 2 insertions(+)
> 
> diff --git a/hw/vfio/pci-quirks.c b/hw/vfio/pci-quirks.c
> index 84b1a7b9485c..496fd1ee86bd 100644
> --- a/hw/vfio/pci-quirks.c
> +++ b/hw/vfio/pci-quirks.c
> @@ -1538,6 +1538,7 @@ static bool is_valid_std_cap_offset(uint8_t pos)
>   
>   static int vfio_add_nv_gpudirect_cap(VFIOPCIDevice *vdev, Error **errp)
>   {
> +    ERRP_GUARD();
>       PCIDevice *pdev = &vdev->pdev;
>       int ret, pos;
>       bool c8_conflict = false, d4_conflict = false;
> @@ -1630,6 +1631,7 @@ static int vfio_add_nv_gpudirect_cap(VFIOPCIDevice *vdev, Error **errp)
>   #define VMD_SHADOW_CAP_LEN 24
>   static int vfio_add_vmd_shadow_cap(VFIOPCIDevice *vdev, Error **errp)
>   {
> +    ERRP_GUARD();
>       uint8_t membar_phys[16];
>       int ret, pos = 0xE8;
>   

[PATCH 10/17] hw/vfio/pci: Fix missing ERRP_GUARD() for error_prepend()

[Zhao Liu]

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].

In hw/vfio/pci.c, there're 2 functions passing @errp to error_prepend()
without ERRP_GUARD():
- vfio_add_std_cap()
- vfio_realize()

The @errp of vfio_add_std_cap() is also from vfio_realize(). And
vfio_realize(), as a PCIDeviceClass.realize method, its @errp is from
DeviceClass.realize so that there is no guarantee that the @errp won't
point to @error_fatal.

To avoid the issue like [1] said, add missing ERRP_GUARD() at their
beginning.

[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>
Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
---
 hw/vfio/pci.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
index 4fa387f0430d..e70efde11913 100644
--- a/hw/vfio/pci.c
+++ b/hw/vfio/pci.c
@@ -2136,6 +2136,7 @@ static void vfio_check_af_flr(VFIOPCIDevice *vdev, uint8_t pos)
 
 static int vfio_add_std_cap(VFIOPCIDevice *vdev, uint8_t pos, Error **errp)
 {
+    ERRP_GUARD();
     PCIDevice *pdev = &vdev->pdev;
     uint8_t cap_id, next, size;
     int ret;
@@ -2942,6 +2943,7 @@ static void vfio_unregister_req_notifier(VFIOPCIDevice *vdev)
 
 static void vfio_realize(PCIDevice *pdev, Error **errp)
 {
+    ERRP_GUARD();
     VFIOPCIDevice *vdev = VFIO_PCI(pdev);
     VFIODevice *vbasedev = &vdev->vbasedev;
     char *tmp, *subsys;
-- 
2.34.1

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

[Cédric Le Goater]

On 2/29/24 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].
> 
> In hw/vfio/pci.c, there're 2 functions passing @errp to error_prepend()
> without ERRP_GUARD():
> - vfio_add_std_cap()
> - vfio_realize()
> 
> The @errp of vfio_add_std_cap() is also from vfio_realize(). And
> vfio_realize(), as a PCIDeviceClass.realize method, its @errp is from
> DeviceClass.realize so that there is no guarantee that the @errp won't
> point to @error_fatal.
> 
> To avoid the issue like [1] said, add missing ERRP_GUARD() at their
> beginning.
> 
> [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>
> Signed-off-by: Zhao Liu <zhao1.liu@intel.com>


Reviewed-by: Cédric Le Goater <clg@redhat.com>

Thanks,

C.

[PATCH 11/17] hw/vfio/platform: Fix missing ERRP_GUARD() for error_prepend()

[Zhao Liu]

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_platform_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>
Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
---
 hw/vfio/platform.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/vfio/platform.c b/hw/vfio/platform.c
index a8d9b7da633e..dcd2365fb353 100644
--- a/hw/vfio/platform.c
+++ b/hw/vfio/platform.c
@@ -576,6 +576,7 @@ static int vfio_base_device_init(VFIODevice *vbasedev, Error **errp)
  */
 static void vfio_platform_realize(DeviceState *dev, Error **errp)
 {
+    ERRP_GUARD();
     VFIOPlatformDevice *vdev = VFIO_PLATFORM_DEVICE(dev);
     SysBusDevice *sbdev = SYS_BUS_DEVICE(dev);
     VFIODevice *vbasedev = &vdev->vbasedev;
-- 
2.34.1

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

[Cédric Le Goater]

On 2/29/24 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_platform_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>
> Signed-off-by: Zhao Liu <zhao1.liu@intel.com>


Reviewed-by: Cédric Le Goater <clg@redhat.com>

Thanks,

C.


> ---
>   hw/vfio/platform.c | 1 +
>   1 file changed, 1 insertion(+)
> 
> diff --git a/hw/vfio/platform.c b/hw/vfio/platform.c
> index a8d9b7da633e..dcd2365fb353 100644
> --- a/hw/vfio/platform.c
> +++ b/hw/vfio/platform.c
> @@ -576,6 +576,7 @@ static int vfio_base_device_init(VFIODevice *vbasedev, Error **errp)
>    */
>   static void vfio_platform_realize(DeviceState *dev, Error **errp)
>   {
> +    ERRP_GUARD();
>       VFIOPlatformDevice *vdev = VFIO_PLATFORM_DEVICE(dev);
>       SysBusDevice *sbdev = SYS_BUS_DEVICE(dev);
>       VFIODevice *vbasedev = &vdev->vbasedev;

[PATCH 12/17] hw/virtio/vhost-vsock: Fix missing ERRP_GUARD() for error_prepend()

[Zhao Liu]

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 vhost_vsock_device_realize() passes @errp to error_prepend(), and as
a VirtioDeviceClass.realize method, its @errp is from
DeviceClass.realize so that there is no guarantee that the @errp won't
point to @error_fatal.

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: "Michael S. Tsirkin" <mst@redhat.com>
Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
---
 hw/virtio/vhost-vsock.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/virtio/vhost-vsock.c b/hw/virtio/vhost-vsock.c
index d5ca0b5a1055..3d4a5a97f484 100644
--- a/hw/virtio/vhost-vsock.c
+++ b/hw/virtio/vhost-vsock.c
@@ -121,6 +121,7 @@ static const VMStateDescription vmstate_virtio_vhost_vsock = {
 
 static void vhost_vsock_device_realize(DeviceState *dev, Error **errp)
 {
+    ERRP_GUARD();
     VHostVSockCommon *vvc = VHOST_VSOCK_COMMON(dev);
     VirtIODevice *vdev = VIRTIO_DEVICE(dev);
     VHostVSock *vsock = VHOST_VSOCK(dev);
-- 
2.34.1

[PATCH 13/17] hw/virtio/vhost: Fix missing ERRP_GUARD() for error_prepend()

[Zhao Liu]

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].

In hw/virtio/vhost.c, there're 2 functions passing @errp to
error_prepend() without ERRP_GUARD():
- vhost_save_backend_state()
- vhost_load_backend_state()

Their @errp both points to callers' @local_err. However, as the APIs
defined in include/hw/virtio/vhost.h, it is necessary to protect their
@errp with ERRP_GUARD().

To follow the requirement of @errp, add missing ERRP_GUARD() at their
beginning.

[1]: Issue description in the commit message of commit ae7c80a7bd73
     ("error: New macro ERRP_GUARD()").

Cc: "Michael S. Tsirkin" <mst@redhat.com>
Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
---
 hw/virtio/vhost.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
index 2c9ac794680e..2e4e040db87a 100644
--- a/hw/virtio/vhost.c
+++ b/hw/virtio/vhost.c
@@ -2199,6 +2199,7 @@ int vhost_check_device_state(struct vhost_dev *dev, Error **errp)
 
 int vhost_save_backend_state(struct vhost_dev *dev, QEMUFile *f, Error **errp)
 {
+    ERRP_GUARD();
     /* Maximum chunk size in which to transfer the state */
     const size_t chunk_size = 1 * 1024 * 1024;
     g_autofree void *transfer_buf = NULL;
@@ -2291,6 +2292,7 @@ fail:
 
 int vhost_load_backend_state(struct vhost_dev *dev, QEMUFile *f, Error **errp)
 {
+    ERRP_GUARD();
     size_t transfer_buf_size = 0;
     g_autofree void *transfer_buf = NULL;
     g_autoptr(GError) g_err = NULL;
-- 
2.34.1

[PATCH 14/17] migration/option: Fix missing ERRP_GUARD() for error_prepend()

[Zhao Liu]

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 migrate_params_check() passes @errp to error_prepend() without
ERRP_GUARD(), and it could be called from migration_object_init(),
where the passed @errp points to @error_fatal.

Therefore, the error message echoed in error_prepend() will be lost
because of the above issue.

To fix this, 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: Peter Xu <peterx@redhat.com>
Cc: Fabiano Rosas <farosas@suse.de>
Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
---
 migration/options.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/migration/options.c b/migration/options.c
index 3e3e0b93b439..b98cd639979e 100644
--- a/migration/options.c
+++ b/migration/options.c
@@ -1057,6 +1057,8 @@ void migrate_params_init(MigrationParameters *params)
  */
 bool migrate_params_check(MigrationParameters *params, Error **errp)
 {
+    ERRP_GUARD();
+
     if (params->has_compress_level &&
         (params->compress_level > 9)) {
         error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "compress_level",
-- 
2.34.1

Re: [PATCH 14/17] migration/option: Fix missing ERRP_GUARD() for error_prepend()

[Fabiano Rosas]

Zhao Liu <zhao1.liu@linux.intel.com> writes:

> 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 migrate_params_check() passes @errp to error_prepend() without
> ERRP_GUARD(), and it could be called from migration_object_init(),
> where the passed @errp points to @error_fatal.
>
> Therefore, the error message echoed in error_prepend() will be lost
> because of the above issue.
>
> To fix this, 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: Peter Xu <peterx@redhat.com>
> Cc: Fabiano Rosas <farosas@suse.de>
> Signed-off-by: Zhao Liu <zhao1.liu@intel.com>

Reviewed-by: Fabiano Rosas <farosas@suse.de>

Re: [PATCH 14/17] migration/option: Fix missing ERRP_GUARD() for error_prepend()

[Peter Xu]

On Thu, Feb 29, 2024 at 10:39:11PM +0800, 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 migrate_params_check() passes @errp to error_prepend() without
> ERRP_GUARD(), and it could be called from migration_object_init(),
> where the passed @errp points to @error_fatal.
> 
> Therefore, the error message echoed in error_prepend() will be lost
> because of the above issue.
> 
> To fix this, 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: Peter Xu <peterx@redhat.com>
> Cc: Fabiano Rosas <farosas@suse.de>
> Signed-off-by: Zhao Liu <zhao1.liu@intel.com>

Acked-by: Peter Xu <peterx@redhat.com>

-- 
Peter Xu

[PATCH 15/17] net/vhost-vdpa: Fix missing ERRP_GUARD() for error_prepend()

[Zhao Liu]

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 net_init_vhost_vdpa() passes @errp to error_prepend(), and as a
member of net_client_init_fun[], it's called in net_client_init1() and
gets @errp from this caller.

But because netdev_init_modern() passes &error_fatal to
net_client_init1(), then @errp parameter of net_init_vhost_vdpa() would
point to @error_fatal. This causes the error message in error_prepend()
to be lost because of the above issue.

To fix this, 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: Jason Wang <jasowang@redhat.com>
Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
---
 net/vhost-vdpa.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c
index 3726ee5d6763..6c1fa396aae1 100644
--- a/net/vhost-vdpa.c
+++ b/net/vhost-vdpa.c
@@ -1751,6 +1751,7 @@ static int vhost_vdpa_get_max_queue_pairs(int fd, uint64_t features,
 int net_init_vhost_vdpa(const Netdev *netdev, const char *name,
                         NetClientState *peer, Error **errp)
 {
+    ERRP_GUARD();
     const NetdevVhostVDPAOptions *opts;
     uint64_t features;
     int vdpa_device_fd;
-- 
2.34.1

Re: [PATCH 15/17] net/vhost-vdpa: Fix missing ERRP_GUARD() for error_prepend()

[Thomas Huth]

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 net_init_vhost_vdpa() passes @errp to error_prepend(), and as a
> member of net_client_init_fun[], it's called in net_client_init1() and
> gets @errp from this caller.
> 
> But because netdev_init_modern() passes &error_fatal to
> net_client_init1(), then @errp parameter of net_init_vhost_vdpa() would
> point to @error_fatal. This causes the error message in error_prepend()
> to be lost because of the above issue.
> 
> To fix this, 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: Jason Wang <jasowang@redhat.com>
> Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
> ---
>   net/vhost-vdpa.c | 1 +
>   1 file changed, 1 insertion(+)
> 
> diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c
> index 3726ee5d6763..6c1fa396aae1 100644
> --- a/net/vhost-vdpa.c
> +++ b/net/vhost-vdpa.c
> @@ -1751,6 +1751,7 @@ static int vhost_vdpa_get_max_queue_pairs(int fd, uint64_t features,
>   int net_init_vhost_vdpa(const Netdev *netdev, const char *name,
>                           NetClientState *peer, Error **errp)
>   {
> +    ERRP_GUARD();
>       const NetdevVhostVDPAOptions *opts;
>       uint64_t features;
>       int vdpa_device_fd;

Reviewed-by: Thomas Huth <thuth@redhat.com>

[PATCH 16/17] target/i386/sev: Fix missing ERRP_GUARD() for error_prepend()

[Zhao Liu]

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 sev_inject_launch_secret() passes @errp to error_prepend(), and as
an APIs defined in target/i386/sev.h, it is necessary to protect its
@errp 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: Paolo Bonzini <pbonzini@redhat.com>
Cc: Marcelo Tosatti <mtosatti@redhat.com>
Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
---
 target/i386/sev.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/target/i386/sev.c b/target/i386/sev.c
index 173de91afe7d..72930ff0dcc5 100644
--- a/target/i386/sev.c
+++ b/target/i386/sev.c
@@ -1044,6 +1044,7 @@ sev_encrypt_flash(uint8_t *ptr, uint64_t len, Error **errp)
 int sev_inject_launch_secret(const char *packet_hdr, const char *secret,
                              uint64_t gpa, Error **errp)
 {
+    ERRP_GUARD();
     struct kvm_sev_launch_secret input;
     g_autofree guchar *data = NULL, *hdr = NULL;
     int error, ret = 1;
-- 
2.34.1

Re: [PATCH 16/17] target/i386/sev: Fix missing ERRP_GUARD() for error_prepend()

[Thomas Huth]

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 sev_inject_launch_secret() passes @errp to error_prepend(), and as
> an APIs defined in target/i386/sev.h, it is necessary to protect its
> @errp 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: Paolo Bonzini <pbonzini@redhat.com>
> Cc: Marcelo Tosatti <mtosatti@redhat.com>
> Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
> ---
>   target/i386/sev.c | 1 +
>   1 file changed, 1 insertion(+)
> 
> diff --git a/target/i386/sev.c b/target/i386/sev.c
> index 173de91afe7d..72930ff0dcc5 100644
> --- a/target/i386/sev.c
> +++ b/target/i386/sev.c
> @@ -1044,6 +1044,7 @@ sev_encrypt_flash(uint8_t *ptr, uint64_t len, Error **errp)
>   int sev_inject_launch_secret(const char *packet_hdr, const char *secret,
>                                uint64_t gpa, Error **errp)
>   {
> +    ERRP_GUARD();
>       struct kvm_sev_launch_secret input;
>       g_autofree guchar *data = NULL, *hdr = NULL;
>       int error, ret = 1;

Reviewed-by: Thomas Huth <thuth@redhat.com>

[PATCH 17/17] target/s390x/cpu_models: Fix missing ERRP_GUARD() for error_prepend()

[Zhao Liu]

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].

In target/s390x/cpu_models.c, there're 2 functions passing @errp to
error_prepend() without ERRP_GUARD():
- check_compatibility()
- s390_realize_cpu_model()

Though both their @errp parameters point to their callers' local @err
virables and don't cause the issue as [1] said, to follow the
requirement of @errp, also add missing ERRP_GUARD() at their beginning.

[1]: Issue description in the commit message of commit ae7c80a7bd73
     ("error: New macro ERRP_GUARD()").

Cc: David Hildenbrand <david@redhat.com>
Cc: Richard Henderson <richard.henderson@linaro.org>
Cc: Ilya Leoshkevich <iii@linux.ibm.com>
Cc: Thomas Huth <thuth@redhat.com>
Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
---
 target/s390x/cpu_models.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/target/s390x/cpu_models.c b/target/s390x/cpu_models.c
index a63d990e4e8e..1a1c09612271 100644
--- a/target/s390x/cpu_models.c
+++ b/target/s390x/cpu_models.c
@@ -503,6 +503,7 @@ static void error_prepend_missing_feat(const char *name, void *opaque)
 static void check_compatibility(const S390CPUModel *max_model,
                                 const S390CPUModel *model, Error **errp)
 {
+    ERRP_GUARD();
     S390FeatBitmap missing;
 
     if (model->def->gen > max_model->def->gen) {
@@ -566,6 +567,7 @@ S390CPUModel *get_max_cpu_model(Error **errp)
 
 void s390_realize_cpu_model(CPUState *cs, Error **errp)
 {
+    ERRP_GUARD();
     Error *err = NULL;
     S390CPUClass *xcc = S390_CPU_GET_CLASS(cs);
     S390CPU *cpu = S390_CPU(cs);
-- 
2.34.1

Re: [PATCH 17/17] target/s390x/cpu_models: Fix missing ERRP_GUARD() for error_prepend()

[Thomas Huth]

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].
> 
> In target/s390x/cpu_models.c, there're 2 functions passing @errp to
> error_prepend() without ERRP_GUARD():
> - check_compatibility()
> - s390_realize_cpu_model()
> 
> Though both their @errp parameters point to their callers' local @err
> virables and don't cause the issue as [1] said, to follow the
> requirement of @errp, also add missing ERRP_GUARD() at their beginning.
> 
> [1]: Issue description in the commit message of commit ae7c80a7bd73
>       ("error: New macro ERRP_GUARD()").
> 
> Cc: David Hildenbrand <david@redhat.com>
> Cc: Richard Henderson <richard.henderson@linaro.org>
> Cc: Ilya Leoshkevich <iii@linux.ibm.com>
> Cc: Thomas Huth <thuth@redhat.com>
> Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
> ---
>   target/s390x/cpu_models.c | 2 ++
>   1 file changed, 2 insertions(+)
> 
> diff --git a/target/s390x/cpu_models.c b/target/s390x/cpu_models.c
> index a63d990e4e8e..1a1c09612271 100644
> --- a/target/s390x/cpu_models.c
> +++ b/target/s390x/cpu_models.c
> @@ -503,6 +503,7 @@ static void error_prepend_missing_feat(const char *name, void *opaque)
>   static void check_compatibility(const S390CPUModel *max_model,
>                                   const S390CPUModel *model, Error **errp)
>   {
> +    ERRP_GUARD();
>       S390FeatBitmap missing;
>   
>       if (model->def->gen > max_model->def->gen) {
> @@ -566,6 +567,7 @@ S390CPUModel *get_max_cpu_model(Error **errp)
>   
>   void s390_realize_cpu_model(CPUState *cs, Error **errp)
>   {
> +    ERRP_GUARD();
>       Error *err = NULL;

I think that function could use an additional clean-up now: Remove the local 
"err" variable and use "errp" only instead.

  Thomas


>       S390CPUClass *xcc = S390_CPU_GET_CLASS(cs);
>       S390CPU *cpu = S390_CPU(cs);

Re: [PATCH 17/17] target/s390x/cpu_models: Fix missing ERRP_GUARD() for error_prepend()

[Zhao Liu]

> > diff --git a/target/s390x/cpu_models.c b/target/s390x/cpu_models.c
> > index a63d990e4e8e..1a1c09612271 100644
> > --- a/target/s390x/cpu_models.c
> > +++ b/target/s390x/cpu_models.c
> > @@ -503,6 +503,7 @@ static void error_prepend_missing_feat(const char *name, void *opaque)
> >   static void check_compatibility(const S390CPUModel *max_model,
> >                                   const S390CPUModel *model, Error **errp)
> >   {
> > +    ERRP_GUARD();
> >       S390FeatBitmap missing;
> >       if (model->def->gen > max_model->def->gen) {
> > @@ -566,6 +567,7 @@ S390CPUModel *get_max_cpu_model(Error **errp)
> >   void s390_realize_cpu_model(CPUState *cs, Error **errp)
> >   {
> > +    ERRP_GUARD();
> >       Error *err = NULL;
> 
> I think that function could use an additional clean-up now: Remove the local
> "err" variable and use "errp" only instead.
>

Thanks! It's cleaner. Will do this.

Regards,
Zhao

Re: [PATCH 17/17] target/s390x/cpu_models: Fix missing ERRP_GUARD() for error_prepend()

[Zhao Liu]

Hi Thomas,

> >   void s390_realize_cpu_model(CPUState *cs, Error **errp)
> >   {
> > +    ERRP_GUARD();
> >       Error *err = NULL;
> 
> I think that function could use an additional clean-up now: Remove the local
> "err" variable and use "errp" only instead.
>

Since many cases check @err to determine if the callee function fails,
it's better also make those functions reture something like boolean
instead of void. Then we could avoid checking @err/@errp.

Thus, the remaining cleanups include:
* related conversion of local @err to @errp and
* make the callee function return someting,

I can do these centrally in another series, and this series (part 1 +
part 2, 33 patches in total) will only be used for adding ERRP_GUARD(),
which makes it easier to review as well as easier to merge.

Do you agree? ;-)

Thanks,
Zhao

Re: [PATCH 17/17] target/s390x/cpu_models: Fix missing ERRP_GUARD() for error_prepend()

[Thomas Huth]

On 08/03/2024 11.12, Zhao Liu wrote:
> Hi Thomas,
> 
>>>    void s390_realize_cpu_model(CPUState *cs, Error **errp)
>>>    {
>>> +    ERRP_GUARD();
>>>        Error *err = NULL;
>>
>> I think that function could use an additional clean-up now: Remove the local
>> "err" variable and use "errp" only instead.
>>
> 
> Since many cases check @err to determine if the callee function fails,
> it's better also make those functions reture something like boolean
> instead of void. Then we could avoid checking @err/@errp.
> 
> Thus, the remaining cleanups include:
> * related conversion of local @err to @errp and
> * make the callee function return someting,
> 
> I can do these centrally in another series, and this series (part 1 +
> part 2, 33 patches in total) will only be used for adding ERRP_GUARD(),
> which makes it easier to review as well as easier to merge.
> 
> Do you agree? ;-)

Fine for me.

  Thomas