Re: [PATCH v2 12/25] vhost-user-blk: move first vhost_user_blk_init() to _realize()

[Raphael Norwitz <raphael.s.norwitz@gmail.com>]

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()?

> >> +            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