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

[Stefan Hajnoczi]

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);

signature.asc
Description: PGP signature