Re: [PATCH v4 2/3] vhost-user-blk: split vhost_user_blk_sync_config()
Vladimir Sementsov-Ogievskiy writes:
> On 30.04.24 11:15, Markus Armbruster wrote:
>> Vladimir Sementsov-Ogievskiy 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
>>> ---
>>> 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(>dev, (uint8_t *)>blkcfg,
>>> + vdev->config_len, errp);
>>> +if (ret < 0) {
>>> +return ret;
>>> +}
>>> +
>>> +memcpy(vdev->config, >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 *)>blkcfg,
>>> - vdev->config_len, _err);
>>> +ret = vhost_user_blk_sync_config(DEVICE(dev->vdev), _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, >blkcfg, vdev->config_len);
>>> -virtio_notify_config(dev->vdev);
>>> -
>>> return 0;
>>> }
Re: [PATCH v4 2/3] vhost-user-blk: split vhost_user_blk_sync_config()
On 30.04.24 11:15, Markus Armbruster wrote:
Vladimir Sementsov-Ogievskiy 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
---
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(>dev, (uint8_t *)>blkcfg,
+ vdev->config_len, errp);
+if (ret < 0) {
+return ret;
+}
+
+memcpy(vdev->config, >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 *)>blkcfg,
- vdev->config_len, _err);
+ret = vhost_user_blk_sync_config(DEVICE(dev->vdev), _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.
if (ret < 0) {
error_report_err(local_err);
return ret;
}
-memcpy(dev->vdev->config, >blkcfg, vdev->config_len);
-virtio_notify_config(dev->vdev);
-
return 0;
}
--
Best regards,
Vladimir
Re: [PATCH v4 2/3] vhost-user-blk: split vhost_user_blk_sync_config()
Vladimir Sementsov-Ogievskiy 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
> ---
> 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(>dev, (uint8_t *)>blkcfg,
> + vdev->config_len, errp);
> +if (ret < 0) {
> +return ret;
> +}
> +
> +memcpy(vdev->config, >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 *)>blkcfg,
> - vdev->config_len, _err);
> +ret = vhost_user_blk_sync_config(DEVICE(dev->vdev), _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?
> if (ret < 0) {
> error_report_err(local_err);
> return ret;
> }
>
> -memcpy(dev->vdev->config, >blkcfg, vdev->config_len);
> -virtio_notify_config(dev->vdev);
> -
> return 0;
> }
