Re: [PATCH 1/4] vhost-user-blk: simplify and fix vhost_user_blk_handle_config_change

[Raphael Norwitz]

I don’t understand the “valid for resize only” comment. Looks like this is zero 
day behavior and I can’t tell why it was added. Does anyone know?

With that, reasoning and code looks good:

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

> On Oct 6, 2023, at 4:20 PM, Vladimir Sementsov-Ogievskiy 
> <vsementsov@yandex-team.ru> wrote:
> 
> Let's not care about what was changed and update the whole config,
> reasons:
> 
> 1. config->geometry should be updated together with capacity, so we fix
>   a bug.
> 
> 2. Vhost-user protocol doesn't say anything about config change
>   limitation. Silent ignore of changes doesn't seem to be correct.
> 
> 3. vhost-user-vsock reads the whole config
> 
> 4. on realize we don't do any checks on retrieved config, so no reason
>   to care here
> 
> Also, let's notify guest unconditionally:
> 
> 1. So does vhost-user-vsock
> 
> 2. We are going to reuse the functionality in new cases when we do want
>   to notify the guest unconditionally. So, no reason to create extra
>   branches in the logic.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
> ---
> hw/block/vhost-user-blk.c | 11 +++--------
> 1 file changed, 3 insertions(+), 8 deletions(-)
> 
> diff --git a/hw/block/vhost-user-blk.c b/hw/block/vhost-user-blk.c
> index eecf3f7a81..1ee05b46ee 100644
> --- a/hw/block/vhost-user-blk.c
> +++ b/hw/block/vhost-user-blk.c
> @@ -93,7 +93,6 @@ static void vhost_user_blk_set_config(VirtIODevice *vdev, 
> const uint8_t *config)
> static int vhost_user_blk_handle_config_change(struct vhost_dev *dev)
> {
>     int ret;
> -    struct virtio_blk_config blkcfg;
>     VirtIODevice *vdev = dev->vdev;
>     VHostUserBlk *s = VHOST_USER_BLK(dev->vdev);
>     Error *local_err = NULL;
> @@ -102,19 +101,15 @@ static int vhost_user_blk_handle_config_change(struct 
> vhost_dev *dev)
>         return 0;
>     }
> 
> -    ret = vhost_dev_get_config(dev, (uint8_t *)&blkcfg,
> +    ret = vhost_dev_get_config(dev, (uint8_t *)&s->blkcfg,
>                                vdev->config_len, &local_err);
>     if (ret < 0) {
>         error_report_err(local_err);
>         return ret;
>     }
> 
> -    /* valid for resize only */
> -    if (blkcfg.capacity != s->blkcfg.capacity) {
> -        s->blkcfg.capacity = blkcfg.capacity;
> -        memcpy(dev->vdev->config, &s->blkcfg, vdev->config_len);
> -        virtio_notify_config(dev->vdev);
> -    }
> +    memcpy(dev->vdev->config, &s->blkcfg, vdev->config_len);
> +    virtio_notify_config(dev->vdev);
> 
>     return 0;
> }
> -- 
> 2.34.1
>