[Qemu-devel] [PATCH 1/2] vhost-scsi: Call virtio_scsi_common_unrealize() when device realize failed

[Qemu-devel] [PATCH 1/2] vhost-scsi: Call virtio_scsi_common_unrealize() when device realize failed

[elohimes]

From: Xie Yongji <xieyongji@baidu.com>

This avoids memory leak when device hotplug is failed.

Signed-off-by: Xie Yongji <xieyongji@baidu.com>
---
 hw/scsi/vhost-scsi.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/hw/scsi/vhost-scsi.c b/hw/scsi/vhost-scsi.c
index 4090f99ee4..db4a090576 100644
--- a/hw/scsi/vhost-scsi.c
+++ b/hw/scsi/vhost-scsi.c
@@ -210,7 +210,7 @@ static void vhost_scsi_realize(DeviceState *dev, Error **errp)
         if (err) {
             error_propagate(errp, err);
             error_free(vsc->migration_blocker);
-            goto close_fd;
+            goto free_virtio;
         }
     }
 
@@ -240,6 +240,8 @@ static void vhost_scsi_realize(DeviceState *dev, Error **errp)
         migrate_del_blocker(vsc->migration_blocker);
     }
     g_free(vsc->dev.vqs);
+ free_virtio:
+    virtio_scsi_common_unrealize(dev, errp);
  close_fd:
     close(vhostfd);
     return;
-- 
2.17.1

[Qemu-devel] [PATCH 2/2] vhost-user-scsi: Call virtio_scsi_common_unrealize() when device realize failed

[elohimes]

From: Xie Yongji <xieyongji@baidu.com>

This avoids memory leak when device hotplug is failed.

Signed-off-by: Xie Yongji <xieyongji@baidu.com>
---
 hw/scsi/vhost-user-scsi.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/hw/scsi/vhost-user-scsi.c b/hw/scsi/vhost-user-scsi.c
index a9fd8ea305..a79653df46 100644
--- a/hw/scsi/vhost-user-scsi.c
+++ b/hw/scsi/vhost-user-scsi.c
@@ -87,7 +87,7 @@ static void vhost_user_scsi_realize(DeviceState *dev, Error **errp)
     }
 
     if (!vhost_user_init(&s->vhost_user, &vs->conf.chardev, errp)) {
-        return;
+        goto free_virtio;
     }
 
     vsc->dev.nvqs = 2 + vs->conf.num_queues;
@@ -101,15 +101,21 @@ static void vhost_user_scsi_realize(DeviceState *dev, Error **errp)
     if (ret < 0) {
         error_setg(errp, "vhost-user-scsi: vhost initialization failed: %s",
                    strerror(-ret));
-        vhost_user_cleanup(&s->vhost_user);
-        g_free(vqs);
-        return;
+        goto free_vhost;
     }
 
     /* Channel and lun both are 0 for bootable vhost-user-scsi disk */
     vsc->channel = 0;
     vsc->lun = 0;
     vsc->target = vs->conf.boot_tpgt;
+
+    return;
+
+free_vhost:
+    vhost_user_cleanup(&s->vhost_user);
+    g_free(vqs);
+free_virtio:
+    virtio_scsi_common_unrealize(dev, errp);
 }
 
 static void vhost_user_scsi_unrealize(DeviceState *dev, Error **errp)
-- 
2.17.1

Re: [Qemu-devel] [PATCH 1/2] vhost-scsi: Call virtio_scsi_common_unrealize() when device realize failed

[Stefan Hajnoczi]

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

On Mon, Jul 15, 2019 at 06:23:25PM +0800, elohimes@gmail.com wrote:
> From: Xie Yongji <xieyongji@baidu.com>
> 
> This avoids memory leak when device hotplug is failed.
> 
> Signed-off-by: Xie Yongji <xieyongji@baidu.com>
> ---
>  hw/scsi/vhost-scsi.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/scsi/vhost-scsi.c b/hw/scsi/vhost-scsi.c
> index 4090f99ee4..db4a090576 100644
> --- a/hw/scsi/vhost-scsi.c
> +++ b/hw/scsi/vhost-scsi.c
> @@ -210,7 +210,7 @@ static void vhost_scsi_realize(DeviceState *dev, Error **errp)
>          if (err) {
>              error_propagate(errp, err);
>              error_free(vsc->migration_blocker);
> -            goto close_fd;
> +            goto free_virtio;
>          }
>      }
>  
> @@ -240,6 +240,8 @@ static void vhost_scsi_realize(DeviceState *dev, Error **errp)
>          migrate_del_blocker(vsc->migration_blocker);
>      }
>      g_free(vsc->dev.vqs);
> + free_virtio:
> +    virtio_scsi_common_unrealize(dev, errp);

error_set*() requires that *errp == NULL:

  static void error_setv(Error **errp, ...
  ...
      assert(*errp == NULL);

Today virtio_scsi_common_unrealize() doesn't use the errp argument but
if it ever uses it then QEMU will hit an assertion failure.

Please do this instead:

  virtio_scsi_common_unrealize(dev, &error_abort);

If virtio_scsi_common_unrealize() ever produces an error there will be
an message explaining that errors are unexpected.

This also applies to Patch 2.

Alternatively you could do this to handle all cases and propagate the
error:

  Error *local_err = NULL;
  virtio_scsi_common_unrealize(dev, &local_err);
  error_propagate(errp, local_err);

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

Re: [Qemu-devel] [PATCH 1/2] vhost-scsi: Call virtio_scsi_common_unrealize() when device realize failed

[Yongji Xie]

On Tue, 16 Jul 2019 at 22:42, Stefan Hajnoczi <stefanha@gmail.com> wrote:
>
> On Mon, Jul 15, 2019 at 06:23:25PM +0800, elohimes@gmail.com wrote:
> > From: Xie Yongji <xieyongji@baidu.com>
> >
> > This avoids memory leak when device hotplug is failed.
> >
> > Signed-off-by: Xie Yongji <xieyongji@baidu.com>
> > ---
> >  hw/scsi/vhost-scsi.c | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> >
> > diff --git a/hw/scsi/vhost-scsi.c b/hw/scsi/vhost-scsi.c
> > index 4090f99ee4..db4a090576 100644
> > --- a/hw/scsi/vhost-scsi.c
> > +++ b/hw/scsi/vhost-scsi.c
> > @@ -210,7 +210,7 @@ static void vhost_scsi_realize(DeviceState *dev, Error **errp)
> >          if (err) {
> >              error_propagate(errp, err);
> >              error_free(vsc->migration_blocker);
> > -            goto close_fd;
> > +            goto free_virtio;
> >          }
> >      }
> >
> > @@ -240,6 +240,8 @@ static void vhost_scsi_realize(DeviceState *dev, Error **errp)
> >          migrate_del_blocker(vsc->migration_blocker);
> >      }
> >      g_free(vsc->dev.vqs);
> > + free_virtio:
> > +    virtio_scsi_common_unrealize(dev, errp);
>
> error_set*() requires that *errp == NULL:
>
>   static void error_setv(Error **errp, ...
>   ...
>       assert(*errp == NULL);
>
> Today virtio_scsi_common_unrealize() doesn't use the errp argument but
> if it ever uses it then QEMU will hit an assertion failure.
>
> Please do this instead:
>
>   virtio_scsi_common_unrealize(dev, &error_abort);
>
> If virtio_scsi_common_unrealize() ever produces an error there will be
> an message explaining that errors are unexpected.
>
> This also applies to Patch 2.
>
> Alternatively you could do this to handle all cases and propagate the
> error:
>
>   Error *local_err = NULL;
>   virtio_scsi_common_unrealize(dev, &local_err);
>   error_propagate(errp, local_err);

Will fix it in v2. Thank you.

Thanks,
Yongji