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

[PATCH 00/16 Part 1] 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 follow-up of my previous @errp related patch set
[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 requirement, there are many places where ERRP_GUARD() needs to
be added, especially places where there is no way to ensure that the
incoming @errp is pointing to @errp_fatal or not.

To make review easier (and to make sure I'm doing the right thing), I
just post the first part here.

Welcome your feedback!

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

Thanks and Best Regards,
Zhao
---
Zhao Liu (16):
  error: Add error_vprepend() in comment of ERRP_GUARD() rules
  backends/iommufd: Fix missing ERRP_GUARD() for error_prepend()
  block: Fix missing ERRP_GUARD() for error_prepend()
  block/cbw: Fix missing ERRP_GUARD() for error_prepend()
  block/nbd: Fix missing ERRP_GUARD() for error_prepend()
  block/nvme: Fix missing ERRP_GUARD() for error_prepend()
  block/qcow2-bitmap: Fix missing ERRP_GUARD() for error_prepend()
  block/qcow2: Fix missing ERRP_GUARD() for error_prepend()
  block/qed: Fix missing ERRP_GUARD() for error_prepend()
  block/snapshot: Fix missing ERRP_GUARD() for error_prepend()
  block/vdi: Fix missing ERRP_GUARD() for error_prepend()
  block/vmdk: Fix missing ERRP_GUARD() for error_prepend()
  block/virtio-blk: Fix missing ERRP_GUARD() for error_prepend()
  hw/char/xen_console: Fix missing ERRP_GUARD() for error_prepend()
  hw/core/loader-fit: Fix missing ERRP_GUARD() for error_prepend()
  hw/core/qdev-properties-system: Fix missing ERRP_GUARD() for
    error_prepend()

 backends/iommufd.c               | 1 +
 block.c                          | 4 ++++
 block/copy-before-write.c        | 1 +
 block/nbd.c                      | 1 +
 block/nvme.c                     | 3 +++
 block/qcow2-bitmap.c             | 1 +
 block/qcow2.c                    | 2 ++
 block/qed.c                      | 1 +
 block/snapshot.c                 | 2 ++
 block/vdi.c                      | 1 +
 block/vmdk.c                     | 1 +
 hw/block/virtio-blk.c            | 1 +
 hw/char/xen_console.c            | 1 +
 hw/core/loader-fit.c             | 2 ++
 hw/core/qdev-properties-system.c | 1 +
 include/qapi/error.h             | 2 +-
 16 files changed, 24 insertions(+), 1 deletion(-)

-- 
2.34.1

[PATCH 01/16] error: Add error_vprepend() in comment of ERRP_GUARD() rules

[Zhao Liu]

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

The error_vprepend() should use ERRP_GUARD() just as the documentation
of ERRP_GUARD() says:

> It must be used when the function dereferences @errp or passes
> @errp to error_prepend(), error_vprepend(), or error_append_hint().

Considering that error_vprepend() is also an API provided in error.h,
it is necessary to add it to the description of the rules for using
ERRP_GUARD().

Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
---
 include/qapi/error.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/qapi/error.h b/include/qapi/error.h
index f21a231bb1a6..b1b389967f92 100644
--- a/include/qapi/error.h
+++ b/include/qapi/error.h
@@ -207,7 +207,7 @@
  *
  * Without ERRP_GUARD(), use of the @errp parameter is restricted:
  * - It must not be dereferenced, because it may be null.
- * - It should not be passed to error_prepend() or
+ * - 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.
  *
-- 
2.34.1

Re: [PATCH 01/16] error: Add error_vprepend() in comment of ERRP_GUARD() rules

[Markus Armbruster]

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

> From: Zhao Liu <zhao1.liu@intel.com>
>
> The error_vprepend() should use ERRP_GUARD() just as the documentation
> of ERRP_GUARD() says:
>
>> It must be used when the function dereferences @errp or passes
>> @errp to error_prepend(), error_vprepend(), or error_append_hint().
>
> Considering that error_vprepend() is also an API provided in error.h,
> it is necessary to add it to the description of the rules for using
> ERRP_GUARD().
>
> Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
> ---
>  include/qapi/error.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/include/qapi/error.h b/include/qapi/error.h
> index f21a231bb1a6..b1b389967f92 100644
> --- a/include/qapi/error.h
> +++ b/include/qapi/error.h
> @@ -207,7 +207,7 @@
>   *
>   * Without ERRP_GUARD(), use of the @errp parameter is restricted:
>   * - It must not be dereferenced, because it may be null.
> - * - It should not be passed to error_prepend() or
> + * - 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.
>   *

Good catch!

I'd like a comma after error_vprepend().

Reviewed-by: Markus Armbruster <armbru@redhat.com>

Re: [PATCH 01/16] error: Add error_vprepend() in comment of ERRP_GUARD() rules

[Zhao Liu]

> >   * Without ERRP_GUARD(), use of the @errp parameter is restricted:
> >   * - It must not be dereferenced, because it may be null.
> > - * - It should not be passed to error_prepend() or
> > + * - 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.
> >   *
> 
> Good catch!
> 
> I'd like a comma after error_vprepend().
> 
> Reviewed-by: Markus Armbruster <armbru@redhat.com>

Thanks! I'll.

Regards,
Zhao

[PATCH 02/16] backends/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_backend_set_fd() passes @errp to error_prepend(), to avoid
the above issue, 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: Yi Liu <yi.l.liu@intel.com>
Cc: Eric Auger <eric.auger@redhat.com>
Cc: Zhenzhong Duan <zhenzhong.duan@intel.com>
Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
---
 backends/iommufd.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/backends/iommufd.c b/backends/iommufd.c
index 1ef683c7b080..62a79fa6b049 100644
--- a/backends/iommufd.c
+++ b/backends/iommufd.c
@@ -43,6 +43,7 @@ static void iommufd_backend_finalize(Object *obj)
 
 static void iommufd_backend_set_fd(Object *obj, const char *str, Error **errp)
 {
+    ERRP_GUARD();
     IOMMUFDBackend *be = IOMMUFD_BACKEND(obj);
     int fd = -1;
 
-- 
2.34.1

RE: [PATCH 02/16] backends/iommufd: Fix missing ERRP_GUARD() for error_prepend()

[Duan, Zhenzhong]

>-----Original Message-----
>From: Zhao Liu <zhao1.liu@linux.intel.com>
>Subject: [PATCH 02/16] backends/iommufd: Fix missing ERRP_GUARD() for
>error_prepend()
>
>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_backend_set_fd() passes @errp to error_prepend(), to avoid
>the above issue, 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: Yi Liu <yi.l.liu@intel.com>
>Cc: Eric Auger <eric.auger@redhat.com>
>Cc: Zhenzhong Duan <zhenzhong.duan@intel.com>
>Signed-off-by: Zhao Liu <zhao1.liu@intel.com>

Reviewed-by: Zhenzhong Duan <zhenzhong.duan@intel.com>

Thanks
Zhenzhong

>---
> backends/iommufd.c | 1 +
> 1 file changed, 1 insertion(+)
>
>diff --git a/backends/iommufd.c b/backends/iommufd.c
>index 1ef683c7b080..62a79fa6b049 100644
>--- a/backends/iommufd.c
>+++ b/backends/iommufd.c
>@@ -43,6 +43,7 @@ static void iommufd_backend_finalize(Object *obj)
>
> static void iommufd_backend_set_fd(Object *obj, const char *str, Error
>**errp)
> {
>+    ERRP_GUARD();
>     IOMMUFDBackend *be = IOMMUFD_BACKEND(obj);
>     int fd = -1;
>
>--
>2.34.1

[PATCH 03/16] block: 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 block.c, there're 4 functions passing @errp to error_prepend()
without ERRP_GUARD():
 - bdrv_co_create_opts_simple()
 - parse_json_filename()
 - bdrv_open_backing_file()
 - bdrv_append_temp_snapshot()

bdrv_co_create_opts_simple(), as an implementation of
BolckDriver.bdrv_co_create_opts(), its @errp parameter is so widely
sourced that it is necessary to protect it with ERRP_GUARD().

Though the @errp parameters passed to parse_json_filename(),
bdrv_open_backing_file() and bdrv_append_temp_snapshot() points to their
callers' local_err, 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: Kevin Wolf <kwolf@redhat.com>
Cc: Hanna Reitz <hreitz@redhat.com>
Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
---
 block.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/block.c b/block.c
index 1ed9214f66ed..cf66c767a011 100644
--- a/block.c
+++ b/block.c
@@ -633,6 +633,7 @@ int coroutine_fn bdrv_co_create_opts_simple(BlockDriver *drv,
                                             QemuOpts *opts,
                                             Error **errp)
 {
+    ERRP_GUARD();
     BlockBackend *blk;
     QDict *options;
     int64_t size = 0;
@@ -1998,6 +1999,7 @@ fail_opts:
 
 static QDict *parse_json_filename(const char *filename, Error **errp)
 {
+    ERRP_GUARD();
     QObject *options_obj;
     QDict *options;
     int ret;
@@ -3585,6 +3587,7 @@ int bdrv_set_backing_hd(BlockDriverState *bs, BlockDriverState *backing_hd,
 int bdrv_open_backing_file(BlockDriverState *bs, QDict *parent_options,
                            const char *bdref_key, Error **errp)
 {
+    ERRP_GUARD();
     char *backing_filename = NULL;
     char *bdref_key_dot;
     const char *reference = NULL;
@@ -3851,6 +3854,7 @@ static BlockDriverState *bdrv_append_temp_snapshot(BlockDriverState *bs,
                                                    QDict *snapshot_options,
                                                    Error **errp)
 {
+    ERRP_GUARD();
     g_autofree char *tmp_filename = NULL;
     int64_t total_size;
     QemuOpts *opts = NULL;
-- 
2.34.1

Re: [PATCH 03/16] block: Fix missing ERRP_GUARD() for error_prepend()

[Eric Blake]

On Thu, Feb 29, 2024 at 12:37:10AM +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():
> 

> 
> In block.c, there're 4 functions passing @errp to error_prepend()
> without ERRP_GUARD():
>  - bdrv_co_create_opts_simple()
>  - parse_json_filename()
>  - bdrv_open_backing_file()
>  - bdrv_append_temp_snapshot()
> 
> bdrv_co_create_opts_simple(), as an implementation of
> BolckDriver.bdrv_co_create_opts(), its @errp parameter is so widely

BlockDriver

> sourced that it is necessary to protect it with ERRP_GUARD().
> 
> Though the @errp parameters passed to parse_json_filename(),
> bdrv_open_backing_file() and bdrv_append_temp_snapshot() points to their
> callers' local_err, 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: Kevin Wolf <kwolf@redhat.com>
> Cc: Hanna Reitz <hreitz@redhat.com>
> Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
> ---
>  block.c | 4 ++++
>  1 file changed, 4 insertions(+)

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.
Virtualization:  qemu.org | libguestfs.org

Re: [PATCH 03/16] block: Fix missing ERRP_GUARD() for error_prepend()

[Zhao Liu]

On Thu, Feb 29, 2024 at 01:51:16PM -0600, Eric Blake wrote:
> Date: Thu, 29 Feb 2024 13:51:16 -0600
> From: Eric Blake <eblake@redhat.com>
> Subject: Re: [PATCH 03/16] block: Fix missing ERRP_GUARD() for
>  error_prepend()
> 
> On Thu, Feb 29, 2024 at 12:37:10AM +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():
> > 
> 
> > 
> > In block.c, there're 4 functions passing @errp to error_prepend()
> > without ERRP_GUARD():
> >  - bdrv_co_create_opts_simple()
> >  - parse_json_filename()
> >  - bdrv_open_backing_file()
> >  - bdrv_append_temp_snapshot()
> > 
> > bdrv_co_create_opts_simple(), as an implementation of
> > BolckDriver.bdrv_co_create_opts(), its @errp parameter is so widely
> 
> BlockDriver
>

Good catch! Thanks and will fix!

Regards,
Zhao

[PATCH 04/16] block/cbw: 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 cbw_open() passes @errp to error_prepend() without ERRP_GUARD().

Though it is the BlockDriver.bdrv_open() method, and currently its
@errp parameter only points to callers' local_err, 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: John Snow <jsnow@redhat.com>
Cc: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
Cc: Kevin Wolf <kwolf@redhat.com>
Cc: Hanna Reitz <hreitz@redhat.com>
Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
---
 block/copy-before-write.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/block/copy-before-write.c b/block/copy-before-write.c
index 0842a1a6dfbe..8aba27a71d6d 100644
--- a/block/copy-before-write.c
+++ b/block/copy-before-write.c
@@ -407,6 +407,7 @@ out:
 static int cbw_open(BlockDriverState *bs, QDict *options, int flags,
                     Error **errp)
 {
+    ERRP_GUARD();
     BDRVCopyBeforeWriteState *s = bs->opaque;
     BdrvDirtyBitmap *bitmap = NULL;
     int64_t cluster_size;
-- 
2.34.1

Re: [PATCH 04/16] block/cbw: Fix missing ERRP_GUARD() for error_prepend()

[Vladimir Sementsov-Ogievskiy]

On 28.02.24 19:37, 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 cbw_open() passes @errp to error_prepend() without ERRP_GUARD().
> 
> Though it is the BlockDriver.bdrv_open() method, and currently its
> @errp parameter only points to callers' local_err, 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: John Snow<jsnow@redhat.com>
> Cc: Vladimir Sementsov-Ogievskiy<vsementsov@yandex-team.ru>
> Cc: Kevin Wolf<kwolf@redhat.com>
> Cc: Hanna Reitz<hreitz@redhat.com>
> Signed-off-by: Zhao Liu<zhao1.liu@intel.com>

Your actual email is at linux.intel.com, is it all OK?

I'd prefer not to abbreviate copy-before-write in subject, but anyway:
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>

Thanks for fixing!

-- 
Best regards,
Vladimir

Re: [PATCH 04/16] block/cbw: Fix missing ERRP_GUARD() for error_prepend()

[Zhao Liu]

Hi Vladimir,

> > Signed-off-by: Zhao Liu<zhao1.liu@intel.com>
> 
> Your actual email is at linux.intel.com, is it all OK?

Yes, I'm used to sending patches from linux.intel.com, and both
addresses are OK.

> 
> I'd prefer not to abbreviate copy-before-write in subject, but anyway:
> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
>

Thanks! I'll take care of this in the future.

Regards,
Zhao

[PATCH 05/16] block/nbd: 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 nbd_co_do_receive_one_chunk() passes @errp to error_prepend()
without ERRP_GUARD(), and though its @errp parameter points to its
caller's local_err, 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: Eric Blake <eblake@redhat.com>
Cc: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
Cc: Kevin Wolf <kwolf@redhat.com>
Cc: Hanna Reitz <hreitz@redhat.com>
Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
---
 block/nbd.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/block/nbd.c b/block/nbd.c
index b9d4f935e017..ef05f7cdfd65 100644
--- a/block/nbd.c
+++ b/block/nbd.c
@@ -852,6 +852,7 @@ static coroutine_fn int nbd_co_do_receive_one_chunk(
         BDRVNBDState *s, uint64_t cookie, bool only_structured,
         int *request_ret, QEMUIOVector *qiov, void **payload, Error **errp)
 {
+    ERRP_GUARD();
     int ret;
     int i = COOKIE_TO_INDEX(cookie);
     void *local_payload = NULL;
-- 
2.34.1

Re: [PATCH 05/16] block/nbd: Fix missing ERRP_GUARD() for error_prepend()

[Vladimir Sementsov-Ogievskiy]

On 28.02.24 19:37, 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 nbd_co_do_receive_one_chunk() passes @errp to error_prepend()
> without ERRP_GUARD(), and though its @errp parameter points to its
> caller's local_err, 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: Eric Blake<eblake@redhat.com>
> Cc: Vladimir Sementsov-Ogievskiy<vsementsov@yandex-team.ru>
> Cc: Kevin Wolf<kwolf@redhat.com>
> Cc: Hanna Reitz<hreitz@redhat.com>
> Signed-off-by: Zhao Liu<zhao1.liu@intel.com>

Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>

-- 
Best regards,
Vladimir

[PATCH 06/16] block/nvme: 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 nvme.c, there're 3 functions passing @errp to error_prepend()
without ERRP_GUARD():
- nvme_init_queue()
- nvme_create_queue_pair()
- nvme_identify()

All these 3 functions take their @errp parameters from the
nvme_file_open(), which is a BlockDriver.bdrv_nvme() method and its
@errp points to its caller's local_err.

Though these 3 cases haven't trigger the issue like [1] said, 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: Stefan Hajnoczi <stefanha@redhat.com>
Cc: Fam Zheng <fam@euphon.net>
Cc: "Philippe Mathieu-Daudé" <philmd@linaro.org>
Cc: Kevin Wolf <kwolf@redhat.com>
Cc: Hanna Reitz <hreitz@redhat.com>
Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
---
 block/nvme.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/block/nvme.c b/block/nvme.c
index 0a0a0a6b36cd..3a3c6da73d29 100644
--- a/block/nvme.c
+++ b/block/nvme.c
@@ -168,6 +168,7 @@ static QemuOptsList runtime_opts = {
 static bool nvme_init_queue(BDRVNVMeState *s, NVMeQueue *q,
                             unsigned nentries, size_t entry_bytes, Error **errp)
 {
+    ERRP_GUARD();
     size_t bytes;
     int r;
 
@@ -221,6 +222,7 @@ static NVMeQueuePair *nvme_create_queue_pair(BDRVNVMeState *s,
                                              unsigned idx, size_t size,
                                              Error **errp)
 {
+    ERRP_GUARD();
     int i, r;
     NVMeQueuePair *q;
     uint64_t prp_list_iova;
@@ -535,6 +537,7 @@ static int nvme_admin_cmd_sync(BlockDriverState *bs, NvmeCmd *cmd)
 /* Returns true on success, false on failure. */
 static bool nvme_identify(BlockDriverState *bs, int namespace, Error **errp)
 {
+    ERRP_GUARD();
     BDRVNVMeState *s = bs->opaque;
     bool ret = false;
     QEMU_AUTO_VFREE union {
-- 
2.34.1

Re: [PATCH 06/16] block/nvme: Fix missing ERRP_GUARD() for error_prepend()

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 1910 bytes --]

On Thu, Feb 29, 2024 at 12:37:13AM +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].
> 
> In nvme.c, there're 3 functions passing @errp to error_prepend()
> without ERRP_GUARD():
> - nvme_init_queue()
> - nvme_create_queue_pair()
> - nvme_identify()
> 
> All these 3 functions take their @errp parameters from the
> nvme_file_open(), which is a BlockDriver.bdrv_nvme() method and its
> @errp points to its caller's local_err.
> 
> Though these 3 cases haven't trigger the issue like [1] said, 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: Stefan Hajnoczi <stefanha@redhat.com>
> Cc: Fam Zheng <fam@euphon.net>
> Cc: "Philippe Mathieu-Daudé" <philmd@linaro.org>
> Cc: Kevin Wolf <kwolf@redhat.com>
> Cc: Hanna Reitz <hreitz@redhat.com>
> Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
> ---
>  block/nvme.c | 3 +++
>  1 file changed, 3 insertions(+)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[PATCH 07/16] block/qcow2-bitmap: 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 qcow2_co_can_store_new_dirty_bitmap() passes @errp to
error_prepend(). As a BlockDriver.bdrv_co_can_store_new_dirty_bitmap
method, it's called by bdrv_co_can_store_new_dirty_bitmap().

Its caller is not being called anywhere, but as the API in
include/block/block-io.h, we can't ensure what kind of @errp future
users will pass in.

To avoid potential issues as [1] said, add missing ERRP_GUARD() at the
beginning of qcow2_co_can_store_new_dirty_bitmap().

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

Cc: Eric Blake <eblake@redhat.com>
Cc: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
Cc: John Snow <jsnow@redhat.com>
Cc: Kevin Wolf <kwolf@redhat.com>
Cc: Hanna Reitz <hreitz@redhat.com>
Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
---
 block/qcow2-bitmap.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
index 0e567ed588d7..874ea5694851 100644
--- a/block/qcow2-bitmap.c
+++ b/block/qcow2-bitmap.c
@@ -1710,6 +1710,7 @@ bool coroutine_fn qcow2_co_can_store_new_dirty_bitmap(BlockDriverState *bs,
                                                       uint32_t granularity,
                                                       Error **errp)
 {
+    ERRP_GUARD();
     BDRVQcow2State *s = bs->opaque;
     BdrvDirtyBitmap *bitmap;
     uint64_t bitmap_directory_size = 0;
-- 
2.34.1

Re: [PATCH 07/16] block/qcow2-bitmap: Fix missing ERRP_GUARD() for error_prepend()

[Vladimir Sementsov-Ogievskiy]

On 28.02.24 19:37, 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 qcow2_co_can_store_new_dirty_bitmap() passes @errp to
> error_prepend(). As a BlockDriver.bdrv_co_can_store_new_dirty_bitmap
> method, it's called by bdrv_co_can_store_new_dirty_bitmap().
> 
> Its caller is not being called anywhere, but as the API in
> include/block/block-io.h, we can't ensure what kind of @errp future
> users will pass in.
> 
> To avoid potential issues as [1] said, add missing ERRP_GUARD() at the
> beginning of qcow2_co_can_store_new_dirty_bitmap().
> 
> [1]: Issue description in the commit message of commit ae7c80a7bd73
>       ("error: New macro ERRP_GUARD()").
> 
> Cc: Eric Blake<eblake@redhat.com>
> Cc: Vladimir Sementsov-Ogievskiy<vsementsov@yandex-team.ru>
> Cc: John Snow<jsnow@redhat.com>
> Cc: Kevin Wolf<kwolf@redhat.com>
> Cc: Hanna Reitz<hreitz@redhat.com>
> Signed-off-by: Zhao Liu<zhao1.liu@intel.com>

Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>

-- 
Best regards,
Vladimir

[PATCH 08/16] block/qcow2: 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 block/qcow2.c, there're 2 functions passing @errp to error_prepend()
without ERRP_GUARD():
 - qcow2_co_create()
 - qcow2_co_truncate()

Their @errp parameters are so widely sourced that it is necessary to
protect their @errp with ERRP_GUARD().

Therefore, 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: Kevin Wolf <kwolf@redhat.com>
Cc: Hanna Reitz <hreitz@redhat.com>
Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
---
 block/qcow2.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/block/qcow2.c b/block/qcow2.c
index 204f5854cff2..956128b40948 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -3483,6 +3483,7 @@ static uint64_t qcow2_opt_get_refcount_bits_del(QemuOpts *opts, int version,
 static int coroutine_fn GRAPH_UNLOCKED
 qcow2_co_create(BlockdevCreateOptions *create_options, Error **errp)
 {
+    ERRP_GUARD();
     BlockdevCreateOptionsQcow2 *qcow2_opts;
     QDict *options;
 
@@ -4283,6 +4284,7 @@ static int coroutine_fn GRAPH_RDLOCK
 qcow2_co_truncate(BlockDriverState *bs, int64_t offset, bool exact,
                   PreallocMode prealloc, BdrvRequestFlags flags, Error **errp)
 {
+    ERRP_GUARD();
     BDRVQcow2State *s = bs->opaque;
     uint64_t old_length;
     int64_t new_l1_size;
-- 
2.34.1

Re: [PATCH 08/16] block/qcow2: Fix missing ERRP_GUARD() for error_prepend()

[Eric Blake]

On Thu, Feb 29, 2024 at 12:37:15AM +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():
> 

> 
> In block/qcow2.c, there're 2 functions passing @errp to error_prepend()

s/there're/there are/

> without ERRP_GUARD():
>  - qcow2_co_create()
>  - qcow2_co_truncate()
> 
> Their @errp parameters are so widely sourced that it is necessary to
> protect their @errp with ERRP_GUARD().
> 
> Therefore, 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: Kevin Wolf <kwolf@redhat.com>
> Cc: Hanna Reitz <hreitz@redhat.com>
> Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
> ---
>  block/qcow2.c | 2 ++
>  1 file changed, 2 insertions(+)

Reviewed-by: Eric Blake <eblake@redhat.com>


-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.
Virtualization:  qemu.org | libguestfs.org

Re: [PATCH 08/16] block/qcow2: Fix missing ERRP_GUARD() for error_prepend()

[Zhao Liu]

On Thu, Feb 29, 2024 at 02:48:44PM -0600, Eric Blake wrote:
> Date: Thu, 29 Feb 2024 14:48:44 -0600
> From: Eric Blake <eblake@redhat.com>
> Subject: Re: [PATCH 08/16] block/qcow2: Fix missing ERRP_GUARD() for
>  error_prepend()
> 
> On Thu, Feb 29, 2024 at 12:37:15AM +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():
> > 
> 
> > 
> > In block/qcow2.c, there're 2 functions passing @errp to error_prepend()
> 
> s/there're/there are/

Thanks! Will fix.

-Zhao

[PATCH 09/16] block/qed: 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 bdrv_qed_co_invalidate_cache() passes @errp to error_prepend()
without ERRP_GUARD().

Though it is a BlockDriver.bdrv_co_invalidate_cache() method, and
currently its @errp parameter only points to callers' local_err, 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: Stefan Hajnoczi <stefanha@redhat.com>
Cc: Kevin Wolf <kwolf@redhat.com>
Cc: Hanna Reitz <hreitz@redhat.com>
Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
---
 block/qed.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/block/qed.c b/block/qed.c
index bc2f0a61c0a9..fa5bc1108552 100644
--- a/block/qed.c
+++ b/block/qed.c
@@ -1579,6 +1579,7 @@ bdrv_qed_co_change_backing_file(BlockDriverState *bs, const char *backing_file,
 static void coroutine_fn GRAPH_RDLOCK
 bdrv_qed_co_invalidate_cache(BlockDriverState *bs, Error **errp)
 {
+    ERRP_GUARD();
     BDRVQEDState *s = bs->opaque;
     int ret;
 
-- 
2.34.1

Re: [PATCH 09/16] block/qed: Fix missing ERRP_GUARD() for error_prepend()

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 1655 bytes --]

On Thu, Feb 29, 2024 at 12:37:16AM +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 bdrv_qed_co_invalidate_cache() passes @errp to error_prepend()
> without ERRP_GUARD().
> 
> Though it is a BlockDriver.bdrv_co_invalidate_cache() method, and
> currently its @errp parameter only points to callers' local_err, 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: Stefan Hajnoczi <stefanha@redhat.com>
> Cc: Kevin Wolf <kwolf@redhat.com>
> Cc: Hanna Reitz <hreitz@redhat.com>
> Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
> ---
>  block/qed.c | 1 +
>  1 file changed, 1 insertion(+)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[PATCH 10/16] block/snapshot: 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 block/snapshot.c, there're 2 functions passing @errp to
error_prepend() without ERRP_GUARD():
 - bdrv_all_delete_snapshot()
 - bdrv_all_goto_snapshot()

As the APIs exposed in include/block/snapshot.h, they could be called
by other modules.

To avoid potential issues as [1] said, add missing ERRP_GUARD() at the
beginning of these 2 functions.

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

Cc: Kevin Wolf <kwolf@redhat.com>
Cc: Hanna Reitz <hreitz@redhat.com>
Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
---
 block/snapshot.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/block/snapshot.c b/block/snapshot.c
index 8694fc0a3eba..8242b4abac41 100644
--- a/block/snapshot.c
+++ b/block/snapshot.c
@@ -566,6 +566,7 @@ int bdrv_all_delete_snapshot(const char *name,
                              bool has_devices, strList *devices,
                              Error **errp)
 {
+    ERRP_GUARD();
     g_autoptr(GList) bdrvs = NULL;
     GList *iterbdrvs;
 
@@ -605,6 +606,7 @@ int bdrv_all_goto_snapshot(const char *name,
                            bool has_devices, strList *devices,
                            Error **errp)
 {
+    ERRP_GUARD();
     g_autoptr(GList) bdrvs = NULL;
     GList *iterbdrvs;
     int ret;
-- 
2.34.1

[PATCH 11/16] block/vdi: 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 vdi_co_do_create() passes @errp to error_prepend() without
ERRP_GUARD(), and its @errp parameter is so widely sourced that it is
necessary to protect it with ERRP_GUARD().

To avoid the potential issues as [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: Stefan Weil <sw@weilnetz.de>
Cc: Kevin Wolf <kwolf@redhat.com>
Cc: Hanna Reitz <hreitz@redhat.com>
Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
---
 block/vdi.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/block/vdi.c b/block/vdi.c
index 3b57becb9fe0..6363da08cee9 100644
--- a/block/vdi.c
+++ b/block/vdi.c
@@ -738,6 +738,7 @@ static int coroutine_fn GRAPH_UNLOCKED
 vdi_co_do_create(BlockdevCreateOptions *create_options, size_t block_size,
                  Error **errp)
 {
+    ERRP_GUARD();
     BlockdevCreateOptionsVdi *vdi_opts;
     int ret = 0;
     uint64_t bytes = 0;
-- 
2.34.1

[PATCH 12/16] block/vmdk: 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 vmdk_parse_extents() passes @errp to error_prepend(), and its @errp
is from vmdk_open().

Though, vmdk_open(), as a BlockDriver.bdrv_open(), gets the @errp
parameter which is pointer of its caller's local_err, 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: Fam Zheng <fam@euphon.net>
Cc: Kevin Wolf <kwolf@redhat.com>
Cc: Hanna Reitz <hreitz@redhat.com>
Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
---
 block/vmdk.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/block/vmdk.c b/block/vmdk.c
index bf78e1238351..3b82979fdf42 100644
--- a/block/vmdk.c
+++ b/block/vmdk.c
@@ -1147,6 +1147,7 @@ static int GRAPH_RDLOCK
 vmdk_parse_extents(const char *desc, BlockDriverState *bs, QDict *options,
                    Error **errp)
 {
+    ERRP_GUARD();
     int ret;
     int matches;
     char access[11];
-- 
2.34.1

[PATCH 13/16] block/virtio-blk: 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 virtio_blk_vq_aio_context_init() passes @errp to error_prepend().

Though its @errp points its caller's local @err variable, to follow the
requirement of @errp, add missing ERRP_GUARD() at the beginning of
virtio_blk_vq_aio_context_init().

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

Cc: "Michael S. Tsirkin" <mst@redhat.com>
Cc: Stefan Hajnoczi <stefanha@redhat.com>
Cc: Kevin Wolf <kwolf@redhat.com>
Cc: Hanna Reitz <hreitz@redhat.com>
Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
---
 hw/block/virtio-blk.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
index 738cb2ac367d..92de315f17f7 100644
--- a/hw/block/virtio-blk.c
+++ b/hw/block/virtio-blk.c
@@ -1682,6 +1682,7 @@ static bool apply_iothread_vq_mapping(
 /* Context: BQL held */
 static bool virtio_blk_vq_aio_context_init(VirtIOBlock *s, Error **errp)
 {
+    ERRP_GUARD();
     VirtIODevice *vdev = VIRTIO_DEVICE(s);
     VirtIOBlkConf *conf = &s->conf;
     BusState *qbus = BUS(qdev_get_parent_bus(DEVICE(vdev)));
-- 
2.34.1

Re: [PATCH 13/16] block/virtio-blk: Fix missing ERRP_GUARD() for error_prepend()

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 1631 bytes --]

On Thu, Feb 29, 2024 at 12:37:20AM +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 virtio_blk_vq_aio_context_init() passes @errp to error_prepend().
> 
> Though its @errp points its caller's local @err variable, to follow the
> requirement of @errp, add missing ERRP_GUARD() at the beginning of
> virtio_blk_vq_aio_context_init().
> 
> [1]: Issue description in the commit message of commit ae7c80a7bd73
>      ("error: New macro ERRP_GUARD()").
> 
> Cc: "Michael S. Tsirkin" <mst@redhat.com>
> Cc: Stefan Hajnoczi <stefanha@redhat.com>
> Cc: Kevin Wolf <kwolf@redhat.com>
> Cc: Hanna Reitz <hreitz@redhat.com>
> Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
> ---
>  hw/block/virtio-blk.c | 1 +
>  1 file changed, 1 insertion(+)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 13/16] block/virtio-blk: Fix missing ERRP_GUARD() for error_prepend()

[Michael S. Tsirkin]

On Thu, Feb 29, 2024 at 12:37:20AM +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 virtio_blk_vq_aio_context_init() passes @errp to error_prepend().
> 
> Though its @errp points its caller's local @err variable, to follow the
> requirement of @errp, add missing ERRP_GUARD() at the beginning of
> virtio_blk_vq_aio_context_init().
> 
> [1]: Issue description in the commit message of commit ae7c80a7bd73
>      ("error: New macro ERRP_GUARD()").
> 
> Cc: "Michael S. Tsirkin" <mst@redhat.com>
> Cc: Stefan Hajnoczi <stefanha@redhat.com>
> Cc: Kevin Wolf <kwolf@redhat.com>
> Cc: Hanna Reitz <hreitz@redhat.com>
> Signed-off-by: Zhao Liu <zhao1.liu@intel.com>


Acked-by: Michael S. Tsirkin <mst@redhat.com>

merge with other things please.

> ---
>  hw/block/virtio-blk.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
> index 738cb2ac367d..92de315f17f7 100644
> --- a/hw/block/virtio-blk.c
> +++ b/hw/block/virtio-blk.c
> @@ -1682,6 +1682,7 @@ static bool apply_iothread_vq_mapping(
>  /* Context: BQL held */
>  static bool virtio_blk_vq_aio_context_init(VirtIOBlock *s, Error **errp)
>  {
> +    ERRP_GUARD();
>      VirtIODevice *vdev = VIRTIO_DEVICE(s);
>      VirtIOBlkConf *conf = &s->conf;
>      BusState *qbus = BUS(qdev_get_parent_bus(DEVICE(vdev)));
> -- 
> 2.34.1

[PATCH 14/16] hw/char/xen_console: 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_console_connect() passes @errp to error_prepend() without
ERRP_GUARD().

There're 2 places will call xen_console_connect():
 - xen_console_realize(): the @errp is from DeviceClass.realize()'s
			  parameter.
 - xen_console_frontend_changed(): the @errp points its caller's
                                   @local_err.

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

[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: "Marc-André Lureau" <marcandre.lureau@redhat.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
---
 hw/char/xen_console.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/char/xen_console.c b/hw/char/xen_console.c
index 5cbee2f184d8..683c92aca1ce 100644
--- a/hw/char/xen_console.c
+++ b/hw/char/xen_console.c
@@ -206,6 +206,7 @@ static bool con_event(void *_xendev)
 
 static bool xen_console_connect(XenDevice *xendev, Error **errp)
 {
+    ERRP_GUARD();
     XenConsole *con = XEN_CONSOLE_DEVICE(xendev);
     unsigned int port, limit;
 
-- 
2.34.1

Re: [PATCH 14/16] hw/char/xen_console: Fix missing ERRP_GUARD() for error_prepend()

[Anthony PERARD]

On Thu, Feb 29, 2024 at 12:37:21AM +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 xen_console_connect() passes @errp to error_prepend() without
> ERRP_GUARD().
> 
> There're 2 places will call xen_console_connect():
>  - xen_console_realize(): the @errp is from DeviceClass.realize()'s
> 			  parameter.
>  - xen_console_frontend_changed(): the @errp points its caller's
>                                    @local_err.
> 
> To avoid the issue like [1] said, add missing ERRP_GUARD() at the
> beginning of xen_console_connect().
> 
> [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: "Marc-André Lureau" <marcandre.lureau@redhat.com>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Signed-off-by: Zhao Liu <zhao1.liu@intel.com>

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

Thanks,

-- 
Anthony PERARD

[PATCH 15/16] hw/core/loader-fit: 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/core/loader-fit.c, there're 2 functions passing @errp to
error_prepend() without ERRP_GUARD():
 - fit_load_kernel()
 - fit_load_fdt()

Their @errp parameters are both the pointers of the local @err virable
in load_fit().

Though they don't cause the issue like [1] said, 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: Paul Burton <paulburton@kernel.org>
Cc: Aleksandar Rikalo <aleksandar.rikalo@syrmia.com>
Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
---
 hw/core/loader-fit.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/hw/core/loader-fit.c b/hw/core/loader-fit.c
index b7c7b3ba94d4..9f20007dbb51 100644
--- a/hw/core/loader-fit.c
+++ b/hw/core/loader-fit.c
@@ -120,6 +120,7 @@ static int fit_load_kernel(const struct fit_loader *ldr, const void *itb,
                            int cfg, void *opaque, hwaddr *pend,
                            Error **errp)
 {
+    ERRP_GUARD();
     const char *name;
     const void *data;
     const void *load_data;
@@ -178,6 +179,7 @@ static int fit_load_fdt(const struct fit_loader *ldr, const void *itb,
                         int cfg, void *opaque, const void *match_data,
                         hwaddr kernel_end, Error **errp)
 {
+    ERRP_GUARD();
     Error *err = NULL;
     const char *name;
     const void *data;
-- 
2.34.1

[PATCH 16/16] hw/core/qdev-properties-system: 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 set_chr() passes @errp to error_prepend() without ERRP_GUARD().

As a PropertyInfo.set method, the @errp passed to set_chr() 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: Paolo Bonzini <pbonzini@redhat.com>
Cc: "Daniel P. Berrangé" <berrange@redhat.com>
Cc: Eduardo Habkost <eduardo@habkost.net>
Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
---
 hw/core/qdev-properties-system.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/core/qdev-properties-system.c b/hw/core/qdev-properties-system.c
index 1a396521d51f..545c3ceff7c9 100644
--- a/hw/core/qdev-properties-system.c
+++ b/hw/core/qdev-properties-system.c
@@ -242,6 +242,7 @@ static void get_chr(Object *obj, Visitor *v, const char *name, void *opaque,
 static void set_chr(Object *obj, Visitor *v, const char *name, void *opaque,
                     Error **errp)
 {
+    ERRP_GUARD();
     Property *prop = opaque;
     CharBackend *be = object_field_prop_ptr(obj, prop);
     Chardev *s;
-- 
2.34.1

Re: [PATCH 16/16] hw/core/qdev-properties-system: Fix missing ERRP_GUARD() for error_prepend()

[Markus Armbruster]

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

Suggest "when @errp is &error_fatal"

> exit() happens in error_setg earlier than information is added [1].
>
> The set_chr() passes @errp to error_prepend() without ERRP_GUARD().
>
> As a PropertyInfo.set method, the @errp passed to set_chr() is so widely
> sourced that it is necessary to protect it with ERRP_GUARD().

"sourced"?  Do you mean "used"?

Are you trying to say something like "there are too many possible
callers for me to check the impact of this defect; it may or may not be
harmless."

> 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: "Daniel P. Berrangé" <berrange@redhat.com>
> Cc: Eduardo Habkost <eduardo@habkost.net>
> Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
> ---
>  hw/core/qdev-properties-system.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/hw/core/qdev-properties-system.c b/hw/core/qdev-properties-system.c
> index 1a396521d51f..545c3ceff7c9 100644
> --- a/hw/core/qdev-properties-system.c
> +++ b/hw/core/qdev-properties-system.c
> @@ -242,6 +242,7 @@ static void get_chr(Object *obj, Visitor *v, const char *name, void *opaque,
>  static void set_chr(Object *obj, Visitor *v, const char *name, void *opaque,
>                      Error **errp)
>  {
> +    ERRP_GUARD();
>      Property *prop = opaque;
>      CharBackend *be = object_field_prop_ptr(obj, prop);
>      Chardev *s;

Commit message could use a bit of polish.  Regardless
Reviewed-by: Markus Armbruster <armbru@redhat.com>

Re: [PATCH 16/16] hw/core/qdev-properties-system: Fix missing ERRP_GUARD() for error_prepend()

[Zhao Liu]

Hi Markus,

> > ERRP_GUARD() could avoid the case when @errp is the pointer of
> > error_fatal, the user can't see this additional information, because
> 
> Suggest "when @errp is &error_fatal"

Sure! It's clearer.

> > exit() happens in error_setg earlier than information is added [1].
> >
> > The set_chr() passes @errp to error_prepend() without ERRP_GUARD().
> >
> > As a PropertyInfo.set method, the @errp passed to set_chr() is so widely
> > sourced that it is necessary to protect it with ERRP_GUARD().
> 
> "sourced"?  Do you mean "used"?
> 
> Are you trying to say something like "there are too many possible
> callers for me to check the impact of this defect; it may or may not be
> harmless."

Yes! Very well expressed. Thanks for your words.

> > To avoid the issue like [1] said, add missing ERRP_GUARD() at the
> > beginning of this function.
> >

[snip]

> Commit message could use a bit of polish.  Regardless
> Reviewed-by: Markus Armbruster <armbru@redhat.com>
> 

Thanks!

-Zhao

Re: [PATCH 16/16] hw/core/qdev-properties-system: Fix missing ERRP_GUARD() for error_prepend()

[Markus Armbruster]

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

> Hi Markus,
>
>> > ERRP_GUARD() could avoid the case when @errp is the pointer of
>> > error_fatal, the user can't see this additional information, because
>> 
>> Suggest "when @errp is &error_fatal"
>
> Sure! It's clearer.
>
>> > exit() happens in error_setg earlier than information is added [1].
>> >
>> > The set_chr() passes @errp to error_prepend() without ERRP_GUARD().
>> >
>> > As a PropertyInfo.set method, the @errp passed to set_chr() is so widely
>> > sourced that it is necessary to protect it with ERRP_GUARD().
>> 
>> "sourced"?  Do you mean "used"?
>> 
>> Are you trying to say something like "there are too many possible
>> callers for me to check the impact of this defect; it may or may not be
>> harmless."
>
> Yes! Very well expressed. Thanks for your words.

You're welcome!  Go ahead and replace your sentence with it.

>> > To avoid the issue like [1] said, add missing ERRP_GUARD() at the
>> > beginning of this function.
>> >
>
> [snip]
>
>> Commit message could use a bit of polish.  Regardless
>> Reviewed-by: Markus Armbruster <armbru@redhat.com>
>> 
>
> Thanks!
>
> -Zhao