Re: [PATCH] qga/commands-posix: Make ga_wait_child() return boolean

Re: [PATCH] qga/commands-posix: Make ga_wait_child() return boolean

[Philippe Mathieu-Daudé]

On 15/7/24 11:59, Zhao Liu wrote:
> As the comment in qapi/error, dereferencing @errp requires
> ERRP_GUARD():
> 
> * = Why, when and how to use ERRP_GUARD() =
> *
> * Without ERRP_GUARD(), use of the @errp parameter is restricted:
> * - It must not be dereferenced, because it may be null.
> ...
> * 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.
> *
> * Using it when it's not needed is safe, but please avoid cluttering
> * the source with useless code.
> 
> Though currently ga_run_command() only gets &local_err instead of NULL
> @errp, it's still better to follow the requirement to add the
> ERRP_GUARD().
> 
> But as error.h suggested, the best practice for callee is to return
> something to indicate success / failure.
> 
> So make ga_wait_child() return boolean and check the returned boolean in
> ga_run_command() instead of dereferencing @errp, which eliminates the
> need of ERRP_GUARD().

I'd avoid mentioning ERRP_GUARD and just describe:

   Make ga_wait_child() return boolean and check the returned boolean
   in ga_run_command() instead of dereferencing @errp.

For the code change:

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

> Cc: Michael Roth <michael.roth@amd.com>
> Cc: Konstantin Kostiuk <kkostiuk@redhat.com>
> Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
> ---
>   qga/commands-posix.c | 8 ++++----
>   1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/qga/commands-posix.c b/qga/commands-posix.c
> index 7f05996495a2..64bb0be94479 100644
> --- a/qga/commands-posix.c
> +++ b/qga/commands-posix.c
> @@ -59,7 +59,7 @@
>   #endif
>   #endif
>   
> -static void ga_wait_child(pid_t pid, int *status, Error **errp)
> +static bool ga_wait_child(pid_t pid, int *status, Error **errp)
>   {
>       pid_t rpid;
>   
> @@ -70,10 +70,11 @@ static void ga_wait_child(pid_t pid, int *status, Error **errp)
>       if (rpid == -1) {
>           error_setg_errno(errp, errno, "failed to wait for child (pid: %d)",
>                            pid);
> -        return;
> +        return false;
>       }
>   
>       g_assert(rpid == pid);
> +    return true;
>   }
>   
>   static ssize_t ga_pipe_read_str(int fd[2], char **str)
> @@ -178,8 +179,7 @@ static int ga_run_command(const char *argv[], const char *in_str,
>           goto out;
>       }
>   
> -    ga_wait_child(pid, &status, errp);
> -    if (*errp) {
> +    if (!ga_wait_child(pid, &status, errp)) {
>           goto out;
>       }
>   

[PATCH] hw/nubus/nubus-virtio-mmio: Fix missing ERRP_GUARD() in nubus_virtio_mmio_realize()

[Zhao Liu]

As the comment in qapi/error, dereferencing @errp requires
ERRP_GUARD():

* = Why, when and how to use ERRP_GUARD() =
*
* Without ERRP_GUARD(), use of the @errp parameter is restricted:
* - It must not be dereferenced, because it may be null.
...
* 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.
*
* Using it when it's not needed is safe, but please avoid cluttering
* the source with useless code.

But in nubus_virtio_mmio_realize(), @errp is dereferenced without
ERRP_GUARD().

Although nubus_virtio_mmio_realize() - as a DeviceClass.realize()
method - doesn't get the NULL @errp parameter, it hasn't triggered the
bug that dereferencing the NULL @errp. It's still necessary to follow
the requirement of @errp, so add missing ERRP_GUARD() in
nubus_virtio_mmio_realize().

Cc: Laurent Vivier <laurent@vivier.eu>
Cc: Philippe Mathieu-Daudé <philmd@linaro.org>
Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
---
 hw/nubus/nubus-virtio-mmio.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/nubus/nubus-virtio-mmio.c b/hw/nubus/nubus-virtio-mmio.c
index 58a63c84d0be..a5558d3ec28b 100644
--- a/hw/nubus/nubus-virtio-mmio.c
+++ b/hw/nubus/nubus-virtio-mmio.c
@@ -23,6 +23,7 @@ static void nubus_virtio_mmio_set_input_irq(void *opaque, int n, int level)
 
 static void nubus_virtio_mmio_realize(DeviceState *dev, Error **errp)
 {
+    ERRP_GUARD();
     NubusVirtioMMIODeviceClass *nvmdc = NUBUS_VIRTIO_MMIO_GET_CLASS(dev);
     NubusVirtioMMIO *s = NUBUS_VIRTIO_MMIO(dev);
     NubusDevice *nd = NUBUS_DEVICE(dev);
-- 
2.34.1

[PATCH] hw/virtio/vdpa-dev: Check returned value instead of dereferencing @errp

[Zhao Liu]

As the comment in qapi/error, dereferencing @errp requires
ERRP_GUARD():

* = Why, when and how to use ERRP_GUARD() =
*
* Without ERRP_GUARD(), use of the @errp parameter is restricted:
* - It must not be dereferenced, because it may be null.
...
* 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.
*
* Using it when it's not needed is safe, but please avoid cluttering
* the source with useless code.

Though vhost_vdpa_device_realize() is called at DeviceClass.realize()
context and won't get NULL @errp, it's still better to follow the
requirement to add the ERRP_GUARD().

But qemu_open() and vhost_vdpa_device_get_u32()'s return values can
distinguish between successful and unsuccessful calls, so check the
return values directly without dereferencing @errp, which eliminates
the need of ERRP_GUARD().

Cc: "Michael S. Tsirkin" <mst@redhat.com>
Cc: "Eugenio Pérez" <eperezma@redhat.com>
Cc: Jason Wang <jasowang@redhat.com>
Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
---
 hw/virtio/vdpa-dev.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/hw/virtio/vdpa-dev.c b/hw/virtio/vdpa-dev.c
index 64b96b226c39..7b439efdc1d3 100644
--- a/hw/virtio/vdpa-dev.c
+++ b/hw/virtio/vdpa-dev.c
@@ -50,6 +50,7 @@ vhost_vdpa_device_get_u32(int fd, unsigned long int cmd, Error **errp)
 
 static void vhost_vdpa_device_realize(DeviceState *dev, Error **errp)
 {
+    ERRP_GUARD();
     VirtIODevice *vdev = VIRTIO_DEVICE(dev);
     VhostVdpaDevice *v = VHOST_VDPA_DEVICE(vdev);
     struct vhost_vdpa_iova_range iova_range;
@@ -63,19 +64,19 @@ static void vhost_vdpa_device_realize(DeviceState *dev, Error **errp)
     }
 
     v->vhostfd = qemu_open(v->vhostdev, O_RDWR, errp);
-    if (*errp) {
+    if (v->vhostfd < 0) {
         return;
     }
 
     v->vdev_id = vhost_vdpa_device_get_u32(v->vhostfd,
                                            VHOST_VDPA_GET_DEVICE_ID, errp);
-    if (*errp) {
+    if (v->vdev_id < 0) {
         goto out;
     }
 
     max_queue_size = vhost_vdpa_device_get_u32(v->vhostfd,
                                                VHOST_VDPA_GET_VRING_NUM, errp);
-    if (*errp) {
+    if (max_queue_size < 0) {
         goto out;
     }
 
@@ -89,7 +90,7 @@ static void vhost_vdpa_device_realize(DeviceState *dev, Error **errp)
 
     v->num_queues = vhost_vdpa_device_get_u32(v->vhostfd,
                                               VHOST_VDPA_GET_VQS_COUNT, errp);
-    if (*errp) {
+    if (v->num_queues < 0) {
         goto out;
     }
 
@@ -127,7 +128,7 @@ static void vhost_vdpa_device_realize(DeviceState *dev, Error **errp)
     v->config_size = vhost_vdpa_device_get_u32(v->vhostfd,
                                                VHOST_VDPA_GET_CONFIG_SIZE,
                                                errp);
-    if (*errp) {
+    if (v->config_size < 0) {
         goto vhost_cleanup;
     }
 
-- 
2.34.1

[PATCH] qga/commands-posix: Make ga_wait_child() return boolean

[Zhao Liu]

As the comment in qapi/error, dereferencing @errp requires
ERRP_GUARD():

* = Why, when and how to use ERRP_GUARD() =
*
* Without ERRP_GUARD(), use of the @errp parameter is restricted:
* - It must not be dereferenced, because it may be null.
...
* 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.
*
* Using it when it's not needed is safe, but please avoid cluttering
* the source with useless code.

Though currently ga_run_command() only gets &local_err instead of NULL
@errp, it's still better to follow the requirement to add the
ERRP_GUARD().

But as error.h suggested, the best practice for callee is to return
something to indicate success / failure.

So make ga_wait_child() return boolean and check the returned boolean in
ga_run_command() instead of dereferencing @errp, which eliminates the
need of ERRP_GUARD().

Cc: Michael Roth <michael.roth@amd.com>
Cc: Konstantin Kostiuk <kkostiuk@redhat.com>
Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
---
 qga/commands-posix.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/qga/commands-posix.c b/qga/commands-posix.c
index 7f05996495a2..64bb0be94479 100644
--- a/qga/commands-posix.c
+++ b/qga/commands-posix.c
@@ -59,7 +59,7 @@
 #endif
 #endif
 
-static void ga_wait_child(pid_t pid, int *status, Error **errp)
+static bool ga_wait_child(pid_t pid, int *status, Error **errp)
 {
     pid_t rpid;
 
@@ -70,10 +70,11 @@ static void ga_wait_child(pid_t pid, int *status, Error **errp)
     if (rpid == -1) {
         error_setg_errno(errp, errno, "failed to wait for child (pid: %d)",
                          pid);
-        return;
+        return false;
     }
 
     g_assert(rpid == pid);
+    return true;
 }
 
 static ssize_t ga_pipe_read_str(int fd[2], char **str)
@@ -178,8 +179,7 @@ static int ga_run_command(const char *argv[], const char *in_str,
         goto out;
     }
 
-    ga_wait_child(pid, &status, errp);
-    if (*errp) {
+    if (!ga_wait_child(pid, &status, errp)) {
         goto out;
     }
 
-- 
2.34.1

Re: [PATCH] qga/commands-posix: Make ga_wait_child() return boolean

[Zhao Liu]

On Mon, Jul 15, 2024 at 11:59:29AM +0200, Philippe Mathieu-Daudé wrote:
> Date: Mon, 15 Jul 2024 11:59:29 +0200
> From: Philippe Mathieu-Daudé <philmd@linaro.org>
> Subject: Re: [PATCH] qga/commands-posix: Make ga_wait_child() return boolean
> 
> On 15/7/24 11:59, Zhao Liu wrote:
> > As the comment in qapi/error, dereferencing @errp requires
> > ERRP_GUARD():
> > 
> > * = Why, when and how to use ERRP_GUARD() =
> > *
> > * Without ERRP_GUARD(), use of the @errp parameter is restricted:
> > * - It must not be dereferenced, because it may be null.
> > ...
> > * 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.
> > *
> > * Using it when it's not needed is safe, but please avoid cluttering
> > * the source with useless code.
> > 
> > Though currently ga_run_command() only gets &local_err instead of NULL
> > @errp, it's still better to follow the requirement to add the
> > ERRP_GUARD().
> > 
> > But as error.h suggested, the best practice for callee is to return
> > something to indicate success / failure.
> > 
> > So make ga_wait_child() return boolean and check the returned boolean in
> > ga_run_command() instead of dereferencing @errp, which eliminates the
> > need of ERRP_GUARD().
> 
> I'd avoid mentioning ERRP_GUARD and just describe:
> 
>   Make ga_wait_child() return boolean and check the returned boolean
>   in ga_run_command() instead of dereferencing @errp.
> 
> For the code change:
> 
> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Many thanks for your words and review!

Will use your words in the next version.

Re: [PATCH] hw/virtio/vdpa-dev: Check returned value instead of dereferencing @errp

[Eugenio Perez Martin]

On Mon, Jul 15, 2024 at 11:45 AM Zhao Liu <zhao1.liu@intel.com> wrote:
>
> As the comment in qapi/error, dereferencing @errp requires
> ERRP_GUARD():
>
> * = Why, when and how to use ERRP_GUARD() =
> *
> * Without ERRP_GUARD(), use of the @errp parameter is restricted:
> * - It must not be dereferenced, because it may be null.
> ...
> * 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.
> *
> * Using it when it's not needed is safe, but please avoid cluttering
> * the source with useless code.
>
> Though vhost_vdpa_device_realize() is called at DeviceClass.realize()
> context and won't get NULL @errp, it's still better to follow the
> requirement to add the ERRP_GUARD().
>
> But qemu_open() and vhost_vdpa_device_get_u32()'s return values can
> distinguish between successful and unsuccessful calls, so check the
> return values directly without dereferencing @errp, which eliminates
> the need of ERRP_GUARD().
>
> Cc: "Michael S. Tsirkin" <mst@redhat.com>
> Cc: "Eugenio Pérez" <eperezma@redhat.com>
> Cc: Jason Wang <jasowang@redhat.com>
> Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
> ---
>  hw/virtio/vdpa-dev.c | 11 ++++++-----
>  1 file changed, 6 insertions(+), 5 deletions(-)
>
> diff --git a/hw/virtio/vdpa-dev.c b/hw/virtio/vdpa-dev.c
> index 64b96b226c39..7b439efdc1d3 100644
> --- a/hw/virtio/vdpa-dev.c
> +++ b/hw/virtio/vdpa-dev.c
> @@ -50,6 +50,7 @@ vhost_vdpa_device_get_u32(int fd, unsigned long int cmd, Error **errp)
>
>  static void vhost_vdpa_device_realize(DeviceState *dev, Error **errp)
>  {
> +    ERRP_GUARD();

Good catch, thank you! But removing the err dereferencing eliminates
the need for ERRP_GUARD(), doesn't it?

Thanks!

>      VirtIODevice *vdev = VIRTIO_DEVICE(dev);
>      VhostVdpaDevice *v = VHOST_VDPA_DEVICE(vdev);
>      struct vhost_vdpa_iova_range iova_range;
> @@ -63,19 +64,19 @@ static void vhost_vdpa_device_realize(DeviceState *dev, Error **errp)
>      }
>
>      v->vhostfd = qemu_open(v->vhostdev, O_RDWR, errp);
> -    if (*errp) {
> +    if (v->vhostfd < 0) {
>          return;
>      }
>
>      v->vdev_id = vhost_vdpa_device_get_u32(v->vhostfd,
>                                             VHOST_VDPA_GET_DEVICE_ID, errp);
> -    if (*errp) {
> +    if (v->vdev_id < 0) {
>          goto out;
>      }
>
>      max_queue_size = vhost_vdpa_device_get_u32(v->vhostfd,
>                                                 VHOST_VDPA_GET_VRING_NUM, errp);
> -    if (*errp) {
> +    if (max_queue_size < 0) {
>          goto out;
>      }
>
> @@ -89,7 +90,7 @@ static void vhost_vdpa_device_realize(DeviceState *dev, Error **errp)
>
>      v->num_queues = vhost_vdpa_device_get_u32(v->vhostfd,
>                                                VHOST_VDPA_GET_VQS_COUNT, errp);
> -    if (*errp) {
> +    if (v->num_queues < 0) {
>          goto out;
>      }
>
> @@ -127,7 +128,7 @@ static void vhost_vdpa_device_realize(DeviceState *dev, Error **errp)
>      v->config_size = vhost_vdpa_device_get_u32(v->vhostfd,
>                                                 VHOST_VDPA_GET_CONFIG_SIZE,
>                                                 errp);
> -    if (*errp) {
> +    if (v->config_size < 0) {
>          goto vhost_cleanup;
>      }
>
> --
> 2.34.1
>

Re: [PATCH] hw/virtio/vdpa-dev: Check returned value instead of dereferencing @errp

[Zhao Liu]

On Mon, Jul 15, 2024 at 11:01:08PM +0200, Eugenio Perez Martin wrote:
> Date: Mon, 15 Jul 2024 23:01:08 +0200
> From: Eugenio Perez Martin <eperezma@redhat.com>
> Subject: Re: [PATCH] hw/virtio/vdpa-dev: Check returned value instead of
>  dereferencing @errp
> 
> On Mon, Jul 15, 2024 at 11:45 AM Zhao Liu <zhao1.liu@intel.com> wrote:
> >
> > As the comment in qapi/error, dereferencing @errp requires
> > ERRP_GUARD():
> >
> > * = Why, when and how to use ERRP_GUARD() =
> > *
> > * Without ERRP_GUARD(), use of the @errp parameter is restricted:
> > * - It must not be dereferenced, because it may be null.
> > ...
> > * 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.
> > *
> > * Using it when it's not needed is safe, but please avoid cluttering
> > * the source with useless code.
> >
> > Though vhost_vdpa_device_realize() is called at DeviceClass.realize()
> > context and won't get NULL @errp, it's still better to follow the
> > requirement to add the ERRP_GUARD().
> >
> > But qemu_open() and vhost_vdpa_device_get_u32()'s return values can
> > distinguish between successful and unsuccessful calls, so check the
> > return values directly without dereferencing @errp, which eliminates
> > the need of ERRP_GUARD().
> >
> > Cc: "Michael S. Tsirkin" <mst@redhat.com>
> > Cc: "Eugenio Pérez" <eperezma@redhat.com>
> > Cc: Jason Wang <jasowang@redhat.com>
> > Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
> > ---
> >  hw/virtio/vdpa-dev.c | 11 ++++++-----
> >  1 file changed, 6 insertions(+), 5 deletions(-)
> >
> > diff --git a/hw/virtio/vdpa-dev.c b/hw/virtio/vdpa-dev.c
> > index 64b96b226c39..7b439efdc1d3 100644
> > --- a/hw/virtio/vdpa-dev.c
> > +++ b/hw/virtio/vdpa-dev.c
> > @@ -50,6 +50,7 @@ vhost_vdpa_device_get_u32(int fd, unsigned long int cmd, Error **errp)
> >
> >  static void vhost_vdpa_device_realize(DeviceState *dev, Error **errp)
> >  {
> > +    ERRP_GUARD();
> 
> Good catch, thank you! But removing the err dereferencing eliminates
> the need for ERRP_GUARD(), doesn't it?
>

Thanks Eugenio! You're right and I forgot to delete it. I'll post a new
version.

Re: [PATCH] hw/virtio/vdpa-dev: Check returned value instead of dereferencing @errp

[Eugenio Perez Martin]

On Tue, Jul 16, 2024 at 5:05 AM Zhao Liu <zhao1.liu@intel.com> wrote:
>
> On Mon, Jul 15, 2024 at 11:01:08PM +0200, Eugenio Perez Martin wrote:
> > Date: Mon, 15 Jul 2024 23:01:08 +0200
> > From: Eugenio Perez Martin <eperezma@redhat.com>
> > Subject: Re: [PATCH] hw/virtio/vdpa-dev: Check returned value instead of
> >  dereferencing @errp
> >
> > On Mon, Jul 15, 2024 at 11:45 AM Zhao Liu <zhao1.liu@intel.com> wrote:
> > >
> > > As the comment in qapi/error, dereferencing @errp requires
> > > ERRP_GUARD():
> > >
> > > * = Why, when and how to use ERRP_GUARD() =
> > > *
> > > * Without ERRP_GUARD(), use of the @errp parameter is restricted:
> > > * - It must not be dereferenced, because it may be null.
> > > ...
> > > * 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.
> > > *
> > > * Using it when it's not needed is safe, but please avoid cluttering
> > > * the source with useless code.
> > >
> > > Though vhost_vdpa_device_realize() is called at DeviceClass.realize()
> > > context and won't get NULL @errp, it's still better to follow the
> > > requirement to add the ERRP_GUARD().
> > >
> > > But qemu_open() and vhost_vdpa_device_get_u32()'s return values can
> > > distinguish between successful and unsuccessful calls, so check the
> > > return values directly without dereferencing @errp, which eliminates
> > > the need of ERRP_GUARD().
> > >
> > > Cc: "Michael S. Tsirkin" <mst@redhat.com>
> > > Cc: "Eugenio Pérez" <eperezma@redhat.com>
> > > Cc: Jason Wang <jasowang@redhat.com>
> > > Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
> > > ---
> > >  hw/virtio/vdpa-dev.c | 11 ++++++-----
> > >  1 file changed, 6 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/hw/virtio/vdpa-dev.c b/hw/virtio/vdpa-dev.c
> > > index 64b96b226c39..7b439efdc1d3 100644
> > > --- a/hw/virtio/vdpa-dev.c
> > > +++ b/hw/virtio/vdpa-dev.c
> > > @@ -50,6 +50,7 @@ vhost_vdpa_device_get_u32(int fd, unsigned long int cmd, Error **errp)
> > >
> > >  static void vhost_vdpa_device_realize(DeviceState *dev, Error **errp)
> > >  {
> > > +    ERRP_GUARD();
> >
> > Good catch, thank you! But removing the err dereferencing eliminates
> > the need for ERRP_GUARD(), doesn't it?
> >
>
> Thanks Eugenio! You're right and I forgot to delete it. I'll post a new
> version.
>
>

Good! With that removed,

Acked-by: Eugenio Pérez <eperezma@redhat.com>

Thanks!

Re: [PATCH] hw/nubus/nubus-virtio-mmio: Fix missing ERRP_GUARD() in nubus_virtio_mmio_realize()

[Zhao Liu]

Hi Philippe,

If possible, can this one catch a ride with your PULL too?

Many thanks!
Zhao

On Mon, Jul 15, 2024 at 05:59:37PM +0800, Zhao Liu wrote:
> Date: Mon, 15 Jul 2024 17:59:37 +0800
> From: Zhao Liu <zhao1.liu@intel.com>
> Subject: [PATCH] hw/nubus/nubus-virtio-mmio: Fix missing ERRP_GUARD() in
>  nubus_virtio_mmio_realize()
> X-Mailer: git-send-email 2.34.1
> 
> As the comment in qapi/error, dereferencing @errp requires
> ERRP_GUARD():
> 
> * = Why, when and how to use ERRP_GUARD() =
> *
> * Without ERRP_GUARD(), use of the @errp parameter is restricted:
> * - It must not be dereferenced, because it may be null.
> ...
> * 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.
> *
> * Using it when it's not needed is safe, but please avoid cluttering
> * the source with useless code.
> 
> But in nubus_virtio_mmio_realize(), @errp is dereferenced without
> ERRP_GUARD().
> 
> Although nubus_virtio_mmio_realize() - as a DeviceClass.realize()
> method - doesn't get the NULL @errp parameter, it hasn't triggered the
> bug that dereferencing the NULL @errp. It's still necessary to follow
> the requirement of @errp, so add missing ERRP_GUARD() in
> nubus_virtio_mmio_realize().
> 
> Cc: Laurent Vivier <laurent@vivier.eu>
> Cc: Philippe Mathieu-Daudé <philmd@linaro.org>
> Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
> ---
>  hw/nubus/nubus-virtio-mmio.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/hw/nubus/nubus-virtio-mmio.c b/hw/nubus/nubus-virtio-mmio.c
> index 58a63c84d0be..a5558d3ec28b 100644
> --- a/hw/nubus/nubus-virtio-mmio.c
> +++ b/hw/nubus/nubus-virtio-mmio.c
> @@ -23,6 +23,7 @@ static void nubus_virtio_mmio_set_input_irq(void *opaque, int n, int level)
>  
>  static void nubus_virtio_mmio_realize(DeviceState *dev, Error **errp)
>  {
> +    ERRP_GUARD();
>      NubusVirtioMMIODeviceClass *nvmdc = NUBUS_VIRTIO_MMIO_GET_CLASS(dev);
>      NubusVirtioMMIO *s = NUBUS_VIRTIO_MMIO(dev);
>      NubusDevice *nd = NUBUS_DEVICE(dev);
> -- 
> 2.34.1
> 

Re: [PATCH] hw/nubus/nubus-virtio-mmio: Fix missing ERRP_GUARD() in nubus_virtio_mmio_realize()

[Philippe Mathieu-Daudé]

+Markus/Eric for review

On 17/7/24 13:27, Zhao Liu wrote:
> Hi Philippe,
> 
> If possible, can this one catch a ride with your PULL too?
> 
> Many thanks!
> Zhao
> 
> On Mon, Jul 15, 2024 at 05:59:37PM +0800, Zhao Liu wrote:
>> Date: Mon, 15 Jul 2024 17:59:37 +0800
>> From: Zhao Liu <zhao1.liu@intel.com>
>> Subject: [PATCH] hw/nubus/nubus-virtio-mmio: Fix missing ERRP_GUARD() in
>>   nubus_virtio_mmio_realize()
>> X-Mailer: git-send-email 2.34.1
>>
>> As the comment in qapi/error, dereferencing @errp requires
>> ERRP_GUARD():
>>
>> * = Why, when and how to use ERRP_GUARD() =
>> *
>> * Without ERRP_GUARD(), use of the @errp parameter is restricted:
>> * - It must not be dereferenced, because it may be null.
>> ...
>> * 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.
>> *
>> * Using it when it's not needed is safe, but please avoid cluttering
>> * the source with useless code.
>>
>> But in nubus_virtio_mmio_realize(), @errp is dereferenced without
>> ERRP_GUARD().
>>
>> Although nubus_virtio_mmio_realize() - as a DeviceClass.realize()
>> method - doesn't get the NULL @errp parameter, it hasn't triggered the
>> bug that dereferencing the NULL @errp. It's still necessary to follow
>> the requirement of @errp, so add missing ERRP_GUARD() in
>> nubus_virtio_mmio_realize().
>>
>> Cc: Laurent Vivier <laurent@vivier.eu>
>> Cc: Philippe Mathieu-Daudé <philmd@linaro.org>
>> Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
>> ---
>>   hw/nubus/nubus-virtio-mmio.c | 1 +
>>   1 file changed, 1 insertion(+)
>>
>> diff --git a/hw/nubus/nubus-virtio-mmio.c b/hw/nubus/nubus-virtio-mmio.c
>> index 58a63c84d0be..a5558d3ec28b 100644
>> --- a/hw/nubus/nubus-virtio-mmio.c
>> +++ b/hw/nubus/nubus-virtio-mmio.c
>> @@ -23,6 +23,7 @@ static void nubus_virtio_mmio_set_input_irq(void *opaque, int n, int level)
>>   
>>   static void nubus_virtio_mmio_realize(DeviceState *dev, Error **errp)
>>   {
>> +    ERRP_GUARD();
>>       NubusVirtioMMIODeviceClass *nvmdc = NUBUS_VIRTIO_MMIO_GET_CLASS(dev);
>>       NubusVirtioMMIO *s = NUBUS_VIRTIO_MMIO(dev);
>>       NubusDevice *nd = NUBUS_DEVICE(dev);
>> -- 
>> 2.34.1
>>

Re: [PATCH] hw/nubus/nubus-virtio-mmio: Fix missing ERRP_GUARD() in nubus_virtio_mmio_realize()

[Markus Armbruster]

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

> As the comment in qapi/error, dereferencing @errp requires

Suggest "According to the comment in qapi/error.h".

> ERRP_GUARD():
>
> * = Why, when and how to use ERRP_GUARD() =
> *
> * Without ERRP_GUARD(), use of the @errp parameter is restricted:
> * - It must not be dereferenced, because it may be null.
> ...
> * 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.
> *
> * Using it when it's not needed is safe, but please avoid cluttering
> * the source with useless code.
>
> But in nubus_virtio_mmio_realize(), @errp is dereferenced without
> ERRP_GUARD().

Suggest to scratch "But".

> Although nubus_virtio_mmio_realize() - as a DeviceClass.realize()
> method - doesn't get the NULL @errp parameter, it hasn't triggered the
> bug that dereferencing the NULL @errp. It's still necessary to follow
> the requirement of @errp, so add missing ERRP_GUARD() in
> nubus_virtio_mmio_realize().

Suggest

  Although nubus_virtio_mmio_realize() - as a DeviceClass.realize()
  method - is never passed a null @errp argument, it should follow the
  rules on @errp usage.  Add the ERRP_GUARD() there.
  
> Cc: Laurent Vivier <laurent@vivier.eu>
> Cc: Philippe Mathieu-Daudé <philmd@linaro.org>
> Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
> ---
>  hw/nubus/nubus-virtio-mmio.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/hw/nubus/nubus-virtio-mmio.c b/hw/nubus/nubus-virtio-mmio.c
> index 58a63c84d0be..a5558d3ec28b 100644
> --- a/hw/nubus/nubus-virtio-mmio.c
> +++ b/hw/nubus/nubus-virtio-mmio.c
> @@ -23,6 +23,7 @@ static void nubus_virtio_mmio_set_input_irq(void *opaque, int n, int level)
>  
>  static void nubus_virtio_mmio_realize(DeviceState *dev, Error **errp)
>  {
> +    ERRP_GUARD();
>      NubusVirtioMMIODeviceClass *nvmdc = NUBUS_VIRTIO_MMIO_GET_CLASS(dev);
>      NubusVirtioMMIO *s = NUBUS_VIRTIO_MMIO(dev);
>      NubusDevice *nd = NUBUS_DEVICE(dev);
       SysBusDevice *sbd;
       int i, offset;

       nvmdc->parent_realize(dev, errp);

Here's the dereference:

       if (*errp) {
           return;
       }

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

Thanks!

Re: [PATCH] hw/nubus/nubus-virtio-mmio: Fix missing ERRP_GUARD() in nubus_virtio_mmio_realize()

[Zhao Liu]

Hi Markus,

On Tue, Jul 23, 2024 at 12:21:17PM +0200, Markus Armbruster wrote:
> Date: Tue, 23 Jul 2024 12:21:17 +0200
> From: Markus Armbruster <armbru@redhat.com>
> Subject: Re: [PATCH] hw/nubus/nubus-virtio-mmio: Fix missing ERRP_GUARD()
>  in nubus_virtio_mmio_realize()
> 
> Zhao Liu <zhao1.liu@intel.com> writes:
> 
> > As the comment in qapi/error, dereferencing @errp requires
> 
> Suggest "According to the comment in qapi/error.h".
 
Thanks! Good words.

> > ERRP_GUARD():
> >
> > * = Why, when and how to use ERRP_GUARD() =
> > *
> > * Without ERRP_GUARD(), use of the @errp parameter is restricted:
> > * - It must not be dereferenced, because it may be null.
> > ...
> > * 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.
> > *
> > * Using it when it's not needed is safe, but please avoid cluttering
> > * the source with useless code.
> >
> > But in nubus_virtio_mmio_realize(), @errp is dereferenced without
> > ERRP_GUARD().
> 
> Suggest to scratch "But".

No problem, will do.

> > Although nubus_virtio_mmio_realize() - as a DeviceClass.realize()
> > method - doesn't get the NULL @errp parameter, it hasn't triggered the
> > bug that dereferencing the NULL @errp. It's still necessary to follow
> > the requirement of @errp, so add missing ERRP_GUARD() in
> > nubus_virtio_mmio_realize().
> 
> Suggest
> 
>   Although nubus_virtio_mmio_realize() - as a DeviceClass.realize()
>   method - is never passed a null @errp argument, it should follow the
>   rules on @errp usage.  Add the ERRP_GUARD() there.

Thanks for the text! It sounds much more authentic!

> > Cc: Laurent Vivier <laurent@vivier.eu>
> > Cc: Philippe Mathieu-Daudé <philmd@linaro.org>
> > Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
> > ---
> >  hw/nubus/nubus-virtio-mmio.c | 1 +
> >  1 file changed, 1 insertion(+)
> >
> > diff --git a/hw/nubus/nubus-virtio-mmio.c b/hw/nubus/nubus-virtio-mmio.c
> > index 58a63c84d0be..a5558d3ec28b 100644
> > --- a/hw/nubus/nubus-virtio-mmio.c
> > +++ b/hw/nubus/nubus-virtio-mmio.c
> > @@ -23,6 +23,7 @@ static void nubus_virtio_mmio_set_input_irq(void *opaque, int n, int level)
> >  
> >  static void nubus_virtio_mmio_realize(DeviceState *dev, Error **errp)
> >  {
> > +    ERRP_GUARD();
> >      NubusVirtioMMIODeviceClass *nvmdc = NUBUS_VIRTIO_MMIO_GET_CLASS(dev);
> >      NubusVirtioMMIO *s = NUBUS_VIRTIO_MMIO(dev);
> >      NubusDevice *nd = NUBUS_DEVICE(dev);
>        SysBusDevice *sbd;
>        int i, offset;
> 
>        nvmdc->parent_realize(dev, errp);
> 
> Here's the dereference:
> 
>        if (*errp) {
>            return;
>        }
> 
> Reviewed-by: Markus Armbruster <armbru@redhat.com>
> 

Thanks! Will refresh a v2 soon.

Regards,
Zhao