Re: [Qemu-devel] [PATCH] vhost: Fix vhostfd leak in error branch

[Jason Wang]

On Mon, Dec 1, 2014 at 2:27 PM, Gonglei <address@hidden> wrote:
> On 2014/12/1 13:03, Jason Wang wrote:
>
> >  
> >  
> >  On Fri, Nov 28, 2014 at 5:26 PM, address@hidden wrote:
> > >  From: Gonglei <address@hidden>
> > >
> > >  Signed-off-by: Gonglei <address@hidden>
> > >  ---
> > >   hw/scsi/vhost-scsi.c | 1 +
> > >   hw/virtio/vhost.c    | 2 ++
> > >   2 files changed, 3 insertions(+)
> > >
> > >  diff --git a/hw/scsi/vhost-scsi.c b/hw/scsi/vhost-scsi.c
> > >  index 308b393..dcb2bc5 100644
> > >  --- a/hw/scsi/vhost-scsi.c
> > >  +++ b/hw/scsi/vhost-scsi.c
> > >  @@ -233,6 +233,7 @@ static void vhost_scsi_realize(DeviceState 
> > > *dev, 
> > >  Error **errp)
> > >                                  vhost_dummy_handle_output);
> > >       if (err != NULL) {
> > >           error_propagate(errp, err);
> > >  +        close(vhostfd);
> > >           return;
> > >       }
> > >   
> > >  diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
> > >  index 5d7c40a..5a12861 100644
> > >  --- a/hw/virtio/vhost.c
> > >  +++ b/hw/virtio/vhost.c
> > >  @@ -817,10 +817,12 @@ int vhost_dev_init(struct vhost_dev *hdev, 
> > > void 
> > >  *opaque,
> > >       int i, r;
> > >   
> > >       if (vhost_set_backend_type(hdev, backend_type) < 0) {
> > >  +        close((uintptr_t)opaque);
> > >           return -1;
> > >       }
> > >   
> > >       if (hdev->vhost_ops->vhost_backend_init(hdev, opaque) < 0) {
> > >  +        close((uintptr_t)opaque);
> > >           return -errno;
> > >       }
> > >   
> >  
> >  Patch looks fine.
> >  
> >  I wonder whether setting errno and goto fail would be better here?
> >  This will let vhost_backend_cleanup() to do the cleanup, e.g 
> > closeing
> >  fd or purging queue (for vhost uesr).
> >  
>
> Hi, Jason
> Actually, vhost_backend_init() can not fail for both vhost-usr
> and vhost-backend-type-kernel  at present. Besides, vhost-usr'
> s vhost_backend_cleanup() just set dev->opaque to 0,
> don't purge queues.


I see, thanks for explaining.

Reviewed-by: Jason Wang <address@hidden>