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

2014-11-30 Thread Jason Wang 



On Fri, Nov 28, 2014 at 5:26 PM, arei.gong...@huawei.com wrote:

From: Gonglei arei.gong...@huawei.com

Signed-off-by: Gonglei arei.gong...@huawei.com
---
 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).

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

2014-11-30 Thread Gonglei 
On 2014/12/1 13:03, Jason Wang wrote:

 
 
 On Fri, Nov 28, 2014 at 5:26 PM, arei.gong...@huawei.com wrote:
 From: Gonglei arei.gong...@huawei.com

 Signed-off-by: Gonglei arei.gong...@huawei.com
 ---
  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.

Regards,
-Gonglei

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

2014-11-30 Thread Jason Wang 



On Mon, Dec 1, 2014 at 2:27 PM, Gonglei arei.gong...@huawei.com wrote:

On 2014/12/1 13:03, Jason Wang wrote:

 
 
 On Fri, Nov 28, 2014 at 5:26 PM, arei.gong...@huawei.com wrote:

 From: Gonglei arei.gong...@huawei.com

 Signed-off-by: Gonglei arei.gong...@huawei.com
 ---
  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 jasow...@redhat.com

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

2014-11-28 Thread arei.gonglei 
From: Gonglei arei.gong...@huawei.com

Signed-off-by: Gonglei arei.gong...@huawei.com
---
 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;
 }
 
-- 
1.7.12.4