fix vhost_user_blk_watch crash

[PATCH] fix vhost_user_blk_watch crash

Posted by Li Feng 4 years, 1 month ago

the G_IO_HUP is watched in tcp_chr_connect, and the callback
vhost_user_blk_watch is not needed, because tcp_chr_hup is registered as
callback. And it will close the tcp link.

Signed-off-by: Li Feng <fengli@smartx.com>
---
 hw/block/vhost-user-blk.c          | 19 -------------------
 include/hw/virtio/vhost-user-blk.h |  1 -
 2 files changed, 20 deletions(-)

diff --git a/hw/block/vhost-user-blk.c b/hw/block/vhost-user-blk.c
index 12925a47ec..17df5338e7 100644
--- a/hw/block/vhost-user-blk.c
+++ b/hw/block/vhost-user-blk.c
@@ -349,18 +349,6 @@ static void vhost_user_blk_disconnect(DeviceState *dev)
     vhost_dev_cleanup(&s->dev);
 }
 
-static gboolean vhost_user_blk_watch(GIOChannel *chan, GIOCondition cond,
-                                     void *opaque)
-{
-    DeviceState *dev = opaque;
-    VirtIODevice *vdev = VIRTIO_DEVICE(dev);
-    VHostUserBlk *s = VHOST_USER_BLK(vdev);
-
-    qemu_chr_fe_disconnect(&s->chardev);
-
-    return true;
-}
-
 static void vhost_user_blk_event(void *opaque, QEMUChrEvent event)
 {
     DeviceState *dev = opaque;
@@ -373,15 +361,9 @@ static void vhost_user_blk_event(void *opaque, QEMUChrEvent event)
             qemu_chr_fe_disconnect(&s->chardev);
             return;
         }
-        s->watch = qemu_chr_fe_add_watch(&s->chardev, G_IO_HUP,
-                                         vhost_user_blk_watch, dev);
         break;
     case CHR_EVENT_CLOSED:
         vhost_user_blk_disconnect(dev);
-        if (s->watch) {
-            g_source_remove(s->watch);
-            s->watch = 0;
-        }
         break;
     case CHR_EVENT_BREAK:
     case CHR_EVENT_MUX_IN:
@@ -428,7 +410,6 @@ static void vhost_user_blk_device_realize(DeviceState *dev, Error **errp)
 
     s->inflight = g_new0(struct vhost_inflight, 1);
     s->vhost_vqs = g_new0(struct vhost_virtqueue, s->num_queues);
-    s->watch = 0;
     s->connected = false;
 
     qemu_chr_fe_set_handlers(&s->chardev,  NULL, NULL, vhost_user_blk_event,
diff --git a/include/hw/virtio/vhost-user-blk.h b/include/hw/virtio/vhost-user-blk.h
index 05ea0ad183..34ad6f0c0e 100644
--- a/include/hw/virtio/vhost-user-blk.h
+++ b/include/hw/virtio/vhost-user-blk.h
@@ -38,7 +38,6 @@ typedef struct VHostUserBlk {
     VhostUserState vhost_user;
     struct vhost_virtqueue *vhost_vqs;
     VirtQueue **virtqs;
-    guint watch;
     bool connected;
 } VHostUserBlk;
 
-- 
2.11.0


-- 
The SmartX email address is only for business purpose. Any sent message 
that is not related to the business is not authorized or permitted by 
SmartX.
本邮箱为北京志凌海纳科技有限公司（SmartX）工作邮箱. 如本邮箱发出的邮件与工作无关,该邮件未得到本公司任何的明示或默示的授权.

Re: [PATCH] fix vhost_user_blk_watch crash

Posted by Raphael Norwitz 4 years, 1 month ago

On Mon, Mar 23, 2020 at 01:29:24PM +0800, Li Feng wrote:
> 
> the G_IO_HUP is watched in tcp_chr_connect, and the callback
> vhost_user_blk_watch is not needed, because tcp_chr_hup is registered as
> callback. And it will close the tcp link.
> 
> Signed-off-by: Li Feng <fengli@smartx.com>

Reviewed-by: Raphael Norwitz <raphael.norwitz@nutanix.com>

> ---
>  hw/block/vhost-user-blk.c          | 19 -------------------
>  include/hw/virtio/vhost-user-blk.h |  1 -
>  2 files changed, 20 deletions(-)
> 
> diff --git a/hw/block/vhost-user-blk.c b/hw/block/vhost-user-blk.c
> index 12925a47ec..17df5338e7 100644
> --- a/hw/block/vhost-user-blk.c
> +++ b/hw/block/vhost-user-blk.c
> @@ -349,18 +349,6 @@ static void vhost_user_blk_disconnect(DeviceState *dev)
>      vhost_dev_cleanup(&s->dev);
>  }
>  
> -static gboolean vhost_user_blk_watch(GIOChannel *chan, GIOCondition cond,
> -                                     void *opaque)
> -{
> -    DeviceState *dev = opaque;
> -    VirtIODevice *vdev = VIRTIO_DEVICE(dev);
> -    VHostUserBlk *s = VHOST_USER_BLK(vdev);
> -
> -    qemu_chr_fe_disconnect(&s->chardev);
> -
> -    return true;
> -}
> -
>  static void vhost_user_blk_event(void *opaque, QEMUChrEvent event)
>  {
>      DeviceState *dev = opaque;
> @@ -373,15 +361,9 @@ static void vhost_user_blk_event(void *opaque, QEMUChrEvent event)
>              qemu_chr_fe_disconnect(&s->chardev);
>              return;
>          }
> -        s->watch = qemu_chr_fe_add_watch(&s->chardev, G_IO_HUP,
> -                                         vhost_user_blk_watch, dev);
>          break;
>      case CHR_EVENT_CLOSED:
>          vhost_user_blk_disconnect(dev);
> -        if (s->watch) {
> -            g_source_remove(s->watch);
> -            s->watch = 0;
> -        }
>          break;
>      case CHR_EVENT_BREAK:
>      case CHR_EVENT_MUX_IN:
> @@ -428,7 +410,6 @@ static void vhost_user_blk_device_realize(DeviceState *dev, Error **errp)
>  
>      s->inflight = g_new0(struct vhost_inflight, 1);
>      s->vhost_vqs = g_new0(struct vhost_virtqueue, s->num_queues);
> -    s->watch = 0;
>      s->connected = false;
>  
>      qemu_chr_fe_set_handlers(&s->chardev,  NULL, NULL, vhost_user_blk_event,
> diff --git a/include/hw/virtio/vhost-user-blk.h b/include/hw/virtio/vhost-user-blk.h
> index 05ea0ad183..34ad6f0c0e 100644
> --- a/include/hw/virtio/vhost-user-blk.h
> +++ b/include/hw/virtio/vhost-user-blk.h
> @@ -38,7 +38,6 @@ typedef struct VHostUserBlk {
>      VhostUserState vhost_user;
>      struct vhost_virtqueue *vhost_vqs;
>      VirtQueue **virtqs;
> -    guint watch;
>      bool connected;
>  } VHostUserBlk;
>  
> -- 
> 2.11.0
> 
> 
> -- 
> The SmartX email address is only for business purpose. Any sent message 
> that is not related to the business is not authorized or permitted by 
> SmartX.
> 本邮箱为北京志凌海纳科技有限公司（SmartX）工作邮箱. 如本邮箱发出的邮件与工作无关,该邮件未得到本公司任何的明示或默示的授权.
> 
> 
> 
>

Re: [PATCH] fix vhost_user_blk_watch crash

Posted by Michael S. Tsirkin 4 years ago

On Mon, Mar 23, 2020 at 01:29:24PM +0800, Li Feng wrote:
> the G_IO_HUP is watched in tcp_chr_connect, and the callback
> vhost_user_blk_watch is not needed, because tcp_chr_hup is registered as
> callback. And it will close the tcp link.
> 
> Signed-off-by: Li Feng <fengli@smartx.com>

Raphael would you like to review this?
Also, I think at this point
nutanix is the biggest contributor to vhost user blk.
So if you want to be added to MAINTAINERS on this
one so people Cc you on patcches, that'd be great.

> ---
>  hw/block/vhost-user-blk.c          | 19 -------------------
>  include/hw/virtio/vhost-user-blk.h |  1 -
>  2 files changed, 20 deletions(-)
> 
> diff --git a/hw/block/vhost-user-blk.c b/hw/block/vhost-user-blk.c
> index 12925a47ec..17df5338e7 100644
> --- a/hw/block/vhost-user-blk.c
> +++ b/hw/block/vhost-user-blk.c
> @@ -349,18 +349,6 @@ static void vhost_user_blk_disconnect(DeviceState *dev)
>      vhost_dev_cleanup(&s->dev);
>  }
>  
> -static gboolean vhost_user_blk_watch(GIOChannel *chan, GIOCondition cond,
> -                                     void *opaque)
> -{
> -    DeviceState *dev = opaque;
> -    VirtIODevice *vdev = VIRTIO_DEVICE(dev);
> -    VHostUserBlk *s = VHOST_USER_BLK(vdev);
> -
> -    qemu_chr_fe_disconnect(&s->chardev);
> -
> -    return true;
> -}
> -
>  static void vhost_user_blk_event(void *opaque, QEMUChrEvent event)
>  {
>      DeviceState *dev = opaque;
> @@ -373,15 +361,9 @@ static void vhost_user_blk_event(void *opaque, QEMUChrEvent event)
>              qemu_chr_fe_disconnect(&s->chardev);
>              return;
>          }
> -        s->watch = qemu_chr_fe_add_watch(&s->chardev, G_IO_HUP,
> -                                         vhost_user_blk_watch, dev);
>          break;
>      case CHR_EVENT_CLOSED:
>          vhost_user_blk_disconnect(dev);
> -        if (s->watch) {
> -            g_source_remove(s->watch);
> -            s->watch = 0;
> -        }
>          break;
>      case CHR_EVENT_BREAK:
>      case CHR_EVENT_MUX_IN:
> @@ -428,7 +410,6 @@ static void vhost_user_blk_device_realize(DeviceState *dev, Error **errp)
>  
>      s->inflight = g_new0(struct vhost_inflight, 1);
>      s->vhost_vqs = g_new0(struct vhost_virtqueue, s->num_queues);
> -    s->watch = 0;
>      s->connected = false;
>  
>      qemu_chr_fe_set_handlers(&s->chardev,  NULL, NULL, vhost_user_blk_event,
> diff --git a/include/hw/virtio/vhost-user-blk.h b/include/hw/virtio/vhost-user-blk.h
> index 05ea0ad183..34ad6f0c0e 100644
> --- a/include/hw/virtio/vhost-user-blk.h
> +++ b/include/hw/virtio/vhost-user-blk.h
> @@ -38,7 +38,6 @@ typedef struct VHostUserBlk {
>      VhostUserState vhost_user;
>      struct vhost_virtqueue *vhost_vqs;
>      VirtQueue **virtqs;
> -    guint watch;
>      bool connected;
>  } VHostUserBlk;
>  
> -- 
> 2.11.0
> 
> 
> -- 
> The SmartX email address is only for business purpose. Any sent message 
> that is not related to the business is not authorized or permitted by 
> SmartX.
> 本邮箱为北京志凌海纳科技有限公司（SmartX）工作邮箱. 如本邮箱发出的邮件与工作无关,该邮件未得到本公司任何的明示或默示的授权.
>

Re: [PATCH] fix vhost_user_blk_watch crash

Posted by Raphael Norwitz 4 years, 1 month ago

On Sun, Mar 29, 2020 at 09:30:24AM -0400, Michael S. Tsirkin wrote:
> 
> On Mon, Mar 23, 2020 at 01:29:24PM +0800, Li Feng wrote:
> > the G_IO_HUP is watched in tcp_chr_connect, and the callback
> > vhost_user_blk_watch is not needed, because tcp_chr_hup is registered as
> > callback. And it will close the tcp link.
> > 
> > Signed-off-by: Li Feng <fengli@smartx.com>
> 
> Raphael would you like to review this?

Sure - I'll review it now.

> Also, I think at this point
> nutanix is the biggest contributor to vhost user blk.
> So if you want to be added to MAINTAINERS on this
> one so people Cc you on patcches, that'd be great.
> 

Yes, please add me to MAINTAINERS.

Re: [PATCH] fix vhost_user_blk_watch crash

Posted by Michael S. Tsirkin 4 years ago

On Wed, Mar 25, 2020 at 06:30:08PM -0400, Raphael Norwitz wrote:
> On Sun, Mar 29, 2020 at 09:30:24AM -0400, Michael S. Tsirkin wrote:
> > 
> > On Mon, Mar 23, 2020 at 01:29:24PM +0800, Li Feng wrote:
> > > the G_IO_HUP is watched in tcp_chr_connect, and the callback
> > > vhost_user_blk_watch is not needed, because tcp_chr_hup is registered as
> > > callback. And it will close the tcp link.
> > > 
> > > Signed-off-by: Li Feng <fengli@smartx.com>
> > 
> > Raphael would you like to review this?
> 
> Sure - I'll review it now.
> 
> > Also, I think at this point
> > nutanix is the biggest contributor to vhost user blk.
> > So if you want to be added to MAINTAINERS on this
> > one so people Cc you on patcches, that'd be great.
> > 
> 
> Yes, please add me to MAINTAINERS.

Great, pls send a patch.

>