ACK on your point. One more question about setting s->connected = false.
On Tue, Oct 21, 2025 at 12:29 PM Vladimir Sementsov-Ogievskiy <[email protected]> wrote: > > On 21.10.25 02:35, Raphael Norwitz wrote: > > On Thu, Oct 16, 2025 at 7:48 AM Vladimir Sementsov-Ogievskiy > > <[email protected]> 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 <[email protected]> > >> --- > >> 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
