Re: [PATCH v4 2/3] vhost-user-blk: split vhost_user_blk_sync_config()

[Markus Armbruster <armbru@redhat.com>]

Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> writes:

> On 30.04.24 11:15, Markus Armbruster wrote:
>> Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> writes:
>> 
>>> Split vhost_user_blk_sync_config() out from
>>> vhost_user_blk_handle_config_change(), to be reused in the following
>>> commit.
>>>
>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
>>> ---
>>>   hw/block/vhost-user-blk.c | 26 +++++++++++++++++++-------
>>>   1 file changed, 19 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/hw/block/vhost-user-blk.c b/hw/block/vhost-user-blk.c
>>> index 9e6bbc6950..091d2c6acf 100644
>>> --- a/hw/block/vhost-user-blk.c
>>> +++ b/hw/block/vhost-user-blk.c
>>> @@ -88,27 +88,39 @@ static void vhost_user_blk_set_config(VirtIODevice *vdev, const uint8_t *config)
>>>       s->blkcfg.wce = blkcfg->wce;
>>>   }
>>>   +static int vhost_user_blk_sync_config(DeviceState *dev, Error **errp)
>>> +{
>>> +    int ret;
>>> +    VirtIODevice *vdev = VIRTIO_DEVICE(dev);
>>
>> Note for later: all this function does with paramter @dev is cast it to
>> VirtIODevice *.
>> 
>>> +    VHostUserBlk *s = VHOST_USER_BLK(vdev);
>>> +
>>> +    ret = vhost_dev_get_config(&s->dev, (uint8_t *)&s->blkcfg,
>>> +                               vdev->config_len, errp);
>>> +    if (ret < 0) {
>>> +        return ret;
>>> +    }
>>> +
>>> +    memcpy(vdev->config, &s->blkcfg, vdev->config_len);
>>> +    virtio_notify_config(vdev);
>>> +
>>> +    return 0;
>>> +}
>>> +
>>>  static int vhost_user_blk_handle_config_change(struct vhost_dev *dev)
>>>  {
>>>      int ret;
>>> -    VirtIODevice *vdev = dev->vdev;
>>> -    VHostUserBlk *s = VHOST_USER_BLK(dev->vdev);
>>>      Error *local_err = NULL;
>>>
>>>      if (!dev->started) {
>>>          return 0;
>>>      }
>>>
>>> -    ret = vhost_dev_get_config(dev, (uint8_t *)&s->blkcfg,
>>> -                               vdev->config_len, &local_err);
>>> +    ret = vhost_user_blk_sync_config(DEVICE(dev->vdev), &local_err);
>>
>> dev->vdev is a VirtIODevice *.  You cast it to DeviceState * for
>> vhost_user_blk_sync_config(), which casts it right back.
>> Could you simply pass it as is instead?
>
> vhost_user_blk_sync_config() is generic handler, which will be used as ->sync_config() in the following commit, so it's good and convenient for it to have DeviceState* argument.

Ah, that's what I missed.

>>>      if (ret < 0) {
>>>          error_report_err(local_err);
>>>          return ret;
>>>      }
>>>
>>> -    memcpy(dev->vdev->config, &s->blkcfg, vdev->config_len);
>>> -    virtio_notify_config(dev->vdev);
>>> -
>>>      return 0;
>>>  }