On Mon, Jul 10, 2023 at 6:44 PM Alex Bennée <[email protected]> wrote:
> Instead of requiring all the information up front allow the > vhost_dev_init to complete and then see what information we have from > the backend. > > This does change the order around somewhat. > > Signed-off-by: Alex Bennée <[email protected]> > --- > hw/virtio/vhost-user-device.c | 45 +++++++++++++++++++++++++---------- > 1 file changed, 32 insertions(+), 13 deletions(-) > > diff --git a/hw/virtio/vhost-user-device.c b/hw/virtio/vhost-user-device.c > index 0109d4829d..b30b6265fb 100644 > --- a/hw/virtio/vhost-user-device.c > +++ b/hw/virtio/vhost-user-device.c > @@ -243,7 +243,6 @@ static void vub_device_realize(DeviceState *dev, Error > **errp) > { > VirtIODevice *vdev = VIRTIO_DEVICE(dev); > VHostUserBase *vub = VHOST_USER_BASE(dev); > - int ret; > > if (!vub->chardev.chr) { > error_setg(errp, "vhost-user-device: missing chardev"); > @@ -254,13 +253,43 @@ static void vub_device_realize(DeviceState *dev, > Error **errp) > return; > } > > + if (vhost_dev_init(&vub->vhost_dev, &vub->vhost_user, > + VHOST_BACKEND_TYPE_USER, 0, errp)!=0) { > + error_setg(errp, "vhost-user-device: unable to start connection"); > + return; > + } > + > + if (vub->vhost_dev.specs.device_id) { > + if (vub->virtio_id && vub->virtio_id != > vub->vhost_dev.specs.device_id) { > + error_setg(errp, "vhost-user-device: backend id %d doesn't > match cli %d", > + vub->vhost_dev.specs.device_id, vub->virtio_id); > + return; > + } > + vub->virtio_id = vub->vhost_dev.specs.device_id; > + } > + > if (!vub->virtio_id) { > - error_setg(errp, "vhost-user-device: need to define device id"); > + error_setg(errp, "vhost-user-device: need to define or be told > device id"); > return; > } > > + if (vub->vhost_dev.specs.min_vqs) { > + if (vub->num_vqs) { > + if (vub->num_vqs < vub->vhost_dev.specs.min_vqs || > + vub->num_vqs > vub->vhost_dev.specs.max_vqs) { > + error_setg(errp, > + "vhost-user-device: selected nvqs (%d) out of > bounds (%d->%d)", > + vub->num_vqs, > + vub->vhost_dev.specs.min_vqs, > vub->vhost_dev.specs.max_vqs); > + return; > + } > + } else { > + vub->num_vqs = vub->vhost_dev.specs.min_vqs; > + } > + } > + > if (!vub->num_vqs) { > - vub->num_vqs = 1; /* reasonable default? */ > + error_setg(errp, "vhost-user-device: need to define number of > vqs"); > } > > /* > @@ -287,16 +316,6 @@ static void vub_device_realize(DeviceState *dev, > Error **errp) > virtio_add_queue(vdev, 4, vub_handle_output)); > } > > - vub->vhost_dev.nvqs = vub->num_vqs; > Who sets `vub->vhost_dev.nvqs` after removing this line? Why having `vub->num_vqs` in the first place? In vub_start for example we still use `vub->vhost_dev.nvqs`, and we pass `vhost_dev` to other functions that use its `nvqs`, so `num_vqs` is redundant and requires a logic to copy/initialise `vhost_dev.nvqs`. Maybe it would be better to initialse `nvqs` through a function, in the device file, instead of doing: `vub->num_vqs = 2;` We could have: `vub_set_nvqs(vub, 2);` Or something along those lines. And the function will have all the internal logic in this commit, i.e., checking the boundaries, setting the `vhost_dev.nvqs` value, printing the error, etc. So we can save the extra variable, and the logic to copy the value to the device. > - > - /* connect to backend */ > - ret = vhost_dev_init(&vub->vhost_dev, &vub->vhost_user, > - VHOST_BACKEND_TYPE_USER, 0, errp); > - > - if (ret < 0) { > - do_vhost_user_cleanup(vdev, vub); > - } > - > qemu_chr_fe_set_handlers(&vub->chardev, NULL, NULL, vub_event, NULL, > dev, NULL, true); > } > -- > 2.39.2 > > >
