Re: [PATCH v2 1/2] vhost-user-blk: use different event handlers on initialization

[Raphael Norwitz]

Couple commit message NITs but otherwise I'm happy with this.

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

On Wed, Mar 24, 2021 at 12:38:28PM +0300, Denis Plotnikov wrote:
> It is useful to use different connect/disconnect event handlers
> on device initialization and operation as seen from the further
> commit fixing a bug on device initialization.
> 
> The patch refactor the code to make use of them: we don't rely any

s/The/This and s/refactor/refactors

> more on the VM state for choosing how to cleanup the device, instead
> we explicitly use the proper event handler dependping on whether

s/dependping/depending

> the device has been initialized.
> 
> Signed-off-by: Denis Plotnikov <den-plotnikov@yandex-team.ru>
> ---
>  hw/block/vhost-user-blk.c | 31 ++++++++++++++++++++++++-------
>  1 file changed, 24 insertions(+), 7 deletions(-)
> 
> diff --git a/hw/block/vhost-user-blk.c b/hw/block/vhost-user-blk.c
> index b870a50e6b20..1af95ec6aae7 100644
> --- a/hw/block/vhost-user-blk.c
> +++ b/hw/block/vhost-user-blk.c
> @@ -362,7 +362,18 @@ static void vhost_user_blk_disconnect(DeviceState *dev)
>      vhost_dev_cleanup(&s->dev);
>  }
>  
> -static void vhost_user_blk_event(void *opaque, QEMUChrEvent event);
> +static void vhost_user_blk_event(void *opaque, QEMUChrEvent event,
> +                                 bool realized);
> +
> +static void vhost_user_blk_event_realize(void *opaque, QEMUChrEvent event)
> +{
> +    vhost_user_blk_event(opaque, event, false);
> +}
> +
> +static void vhost_user_blk_event_oper(void *opaque, QEMUChrEvent event)
> +{
> +    vhost_user_blk_event(opaque, event, true);
> +}
>  
>  static void vhost_user_blk_chr_closed_bh(void *opaque)
>  {
> @@ -371,11 +382,12 @@ static void vhost_user_blk_chr_closed_bh(void *opaque)
>      VHostUserBlk *s = VHOST_USER_BLK(vdev);
>  
>      vhost_user_blk_disconnect(dev);
> -    qemu_chr_fe_set_handlers(&s->chardev, NULL, NULL, vhost_user_blk_event,
> -            NULL, opaque, NULL, true);
> +    qemu_chr_fe_set_handlers(&s->chardev, NULL, NULL,
> +            vhost_user_blk_event_oper, NULL, opaque, NULL, true);
>  }
>  
> -static void vhost_user_blk_event(void *opaque, QEMUChrEvent event)
> +static void vhost_user_blk_event(void *opaque, QEMUChrEvent event,
> +                                 bool realized)
>  {
>      DeviceState *dev = opaque;
>      VirtIODevice *vdev = VIRTIO_DEVICE(dev);
> @@ -406,7 +418,7 @@ static void vhost_user_blk_event(void *opaque, 
> QEMUChrEvent event)
>           * TODO: maybe it is a good idea to make the same fix
>           * for other vhost-user devices.
>           */
> -        if (runstate_is_running()) {
> +        if (realized) {
>              AioContext *ctx = qemu_get_current_aio_context();
>  
>              qemu_chr_fe_set_handlers(&s->chardev, NULL, NULL, NULL, NULL,
> @@ -473,8 +485,9 @@ static void vhost_user_blk_device_realize(DeviceState 
> *dev, Error **errp)
>      s->vhost_vqs = g_new0(struct vhost_virtqueue, s->num_queues);
>      s->connected = false;
>  
> -    qemu_chr_fe_set_handlers(&s->chardev,  NULL, NULL, vhost_user_blk_event,
> -                             NULL, (void *)dev, NULL, true);
> +    qemu_chr_fe_set_handlers(&s->chardev,  NULL, NULL,
> +                             vhost_user_blk_event_realize, NULL, (void *)dev,
> +                             NULL, true);
>  
>  reconnect:
>      if (qemu_chr_fe_wait_connected(&s->chardev, &err) < 0) {
> @@ -494,6 +507,10 @@ reconnect:
>          goto reconnect;
>      }
>  
> +    /* we're fully initialized, now we can operate, so change the handler */
> +    qemu_chr_fe_set_handlers(&s->chardev,  NULL, NULL,
> +                             vhost_user_blk_event_oper, NULL, (void *)dev,
> +                             NULL, true);
>      return;
>  
>  virtio_err:
> -- 
> 2.25.1
>