[1/2] vhost-scsi: Call virtio_scsi_common_unrealize() when device realize failed
diff mbox series

Message ID 20190715102326.2805-1-xieyongji@baidu.com
State New
Headers show
Series
  • [1/2] vhost-scsi: Call virtio_scsi_common_unrealize() when device realize failed
Related show

Commit Message

Yongji Xie July 15, 2019, 10:23 a.m. UTC
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(-)

Comments

Stefan Hajnoczi July 16, 2019, 2:42 p.m. UTC | #1
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);
Yongji Xie July 17, 2019, 12:15 a.m. UTC | #2
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

Patch
diff mbox series

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;