On 21.10.25 21:06, Raphael Norwitz wrote:
> ACK on your point. One more question about setting s->connected = false.
>
> On Tue, Oct 21, 2025 at 12:29 PM Vladimir Sementsov-Ogievskiy
> <vsementsov@yandex-team.ru> wrote:
>>
>> On 21.10.25 02:35, Raphael Norwitz wrote:
>>> On Thu, Oct 16, 2025 at 7:48 AM Vladimir Sementsov-Ogievskiy
>>> <vsementsov@yandex-team.ru> wrote:
>>>>
>>>> We'll need to postpone further connecting/reconnecting logic to the
>>>> later point to support backend-transfer migration for vhost-user-blk.
>>>> For now, move first call to vhost_user_blk_init() to _realize() (this
>>>> call will not be postponed). To support this, we also have to move
>>>> re-initialization to vhost_user_blk_realize_connect_loop().
>>>>
>>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
>>>> ---
>>>> hw/block/vhost-user-blk.c | 17 ++++++++++++++---
>>>> 1 file changed, 14 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/hw/block/vhost-user-blk.c b/hw/block/vhost-user-blk.c
>>>> index 36e32229ad..af4a97b8e4 100644
>>>> --- a/hw/block/vhost-user-blk.c
>>>> +++ b/hw/block/vhost-user-blk.c
>>>> @@ -464,14 +464,12 @@ static int vhost_user_blk_realize_connect(VHostUserBlk *s, Error **errp)
>>>> DeviceState *dev = DEVICE(s);
>>>> int ret;
>>>>
>>>> - s->connected = false;
>>>> -
>>>> ret = qemu_chr_fe_wait_connected(&s->chardev, errp);
>>>> if (ret < 0) {
>>>> return ret;
>>>> }
>>>>
>>>> - ret = vhost_user_blk_init(dev, true, errp);
>>>> + ret = vhost_user_blk_connect(dev, errp);
>>>> if (ret < 0) {
>>>> qemu_chr_fe_disconnect(&s->chardev);
>>>> return ret;
>>>> @@ -501,7 +499,16 @@ static int vhost_user_blk_realize_connect_loop(VHostUserBlk *s, Error **errp)
>>>> error_prepend(errp, "Reconnecting after error: ");
>>>> error_report_err(*errp);
>>>> *errp = NULL;
>>>> +
>
> Having removed setting s->connected = false from
> vhost_user_blk_realize_connect() we will now only set s->connected =
> false here in the if (*errp) {} error path. Shouldn't we also set
> s->connected = false outside the error path here or in
> vhost_user_blk_device_realize()?
>
On success path we are calling vhost_user_blk_realize_connect() for the first
time, so s->connected was never set to true at this time, it must be false
(due to zero-initialization of the structure). We can add an assertion.
>>>> + s->connected = false;
>>>> +
>>>> + ret = vhost_user_blk_init(dev, false, errp);
>>>> + if (ret < 0) {
>>>> + /* No reason to retry initialization */
>>>> + return ret;
>>>> + }
>>>> }
>>>> +
>>>> ret = vhost_user_blk_realize_connect(s, errp);
>>>> } while (ret < 0 && retries--);
>>>>
>>>> @@ -566,6 +573,10 @@ 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);
>>>>
>>>
>>> Why call vhost_user_blk_init() here if we call it in
>>> host_user_blk_realize_connect_loop()?
>>
>> To be able to postpone the whole realize-connect-loop to the later
>> point (not in realize) in further commits.
>>
>> So this first init will stay in realize, for early initialization of the device.
>>
>
> Makes sense - I missed that vhost_user_blk_init() is only called in
> the error path.
>
>>>
>>>> + if (vhost_user_blk_init(dev, false, errp) < 0) {
>>>> + goto fail;
>>>> + }
>>>> +
>>>> if (vhost_user_blk_realize_connect_loop(s, errp) < 0) {
>>>> goto fail;
>>>> }
>>>> --
>>>> 2.48.1
>>>>
>>>>
>>
>>
>> --
>> Best regards,
>> Vladimir
--
Best regards,
Vladimir