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 <address@hidden> wrote:
>
> On Mon, Jul 15, 2019 at 06:23:25PM +0800, address@hidden wrote:
> > From: Xie Yongji <address@hidden>
> >
> > This avoids memory leak when device hotplug is failed.
> >
> > Signed-off-by: Xie Yongji <address@hidden>
> > ---
> >  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