On Wed, Oct 15, 2025 at 2:57 PM Stefano Garzarella <[email protected]> wrote:
>
> On Wed, Oct 15, 2025 at 02:43:14PM +0200, Albert Esteve wrote:
> >Refactor backend_read() function and add a reply_ack variable
> >to have the option for handlers to force tweak whether they should
> >send a reply or not without depending on VHOST_USER_NEED_REPLY_MASK
> >flag.
> >
> >This fixes an issue with
> >vhost_user_backend_handle_shared_object_lookup() logic, as the
> >error path was not closing the backend channel correctly. So,
> >we can remove the reply call from within the handler, make
> >sure it returns early on errors as other handlers do and
> >set the reply_ack variable on backend_read() to true to ensure
> >that it will send a response, thus keeping the original intent.
> >
> >Fixes: 160947666276c5b7f6bca4d746bcac2966635d79
> >Cc: [email protected]
> >Signed-off-by: Albert Esteve <[email protected]>
> >---
> > hw/virtio/vhost-user.c | 40 +++++++++++++---------------------------
> > 1 file changed, 13 insertions(+), 27 deletions(-)
>
> Thanks! This patch LGTM, so
>
> Reviewed-by: Stefano Garzarella <[email protected]>
>
>
> But I left couple of comments that is not related to this fix and maybe
> should be fixed separately:
>
> >
> >diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
> >index 36c9c2e04d..8a93f1d4b5 100644
> >--- a/hw/virtio/vhost-user.c
> >+++ b/hw/virtio/vhost-user.c
> >@@ -1668,14 +1668,6 @@ static bool vhost_user_send_resp(QIOChannel *ioc,
> >VhostUserHeader *hdr,
> > return !qio_channel_writev_all(ioc, iov, ARRAY_SIZE(iov), errp);
> > }
> >
> >-static bool
> >-vhost_user_backend_send_dmabuf_fd(QIOChannel *ioc, VhostUserHeader *hdr,
> >- VhostUserPayload *payload, Error **errp)
> >-{
> >- hdr->size = sizeof(payload->u64);
> >- return vhost_user_send_resp(ioc, hdr, payload, errp);
> >-}
> >-
> > int vhost_user_get_shared_object(struct vhost_dev *dev, unsigned char *uuid,
> > int *dmabuf_fd)
> > {
> >@@ -1716,19 +1708,15 @@ int vhost_user_get_shared_object(struct vhost_dev
> >*dev, unsigned char *uuid,
> >
> > static int
> > vhost_user_backend_handle_shared_object_lookup(struct vhost_user *u,
> >- QIOChannel *ioc,
> >- VhostUserHeader *hdr,
> >- VhostUserPayload
> >*payload)
> >+ VhostUserShared *object)
> > {
> > QemuUUID uuid;
> > CharBackend *chr = u->user->chr;
> >- Error *local_err = NULL;
> > int dmabuf_fd = -1;
> > int fd_num = 0;
> >
> >- memcpy(uuid.data, payload->object.uuid, sizeof(payload->object.uuid));
> >+ memcpy(uuid.data, object->uuid, sizeof(object->uuid));
> >
> >- payload->u64 = 0;
> > switch (virtio_object_type(&uuid)) {
> > case TYPE_DMABUF:
> > dmabuf_fd = virtio_lookup_dmabuf(&uuid);
> >@@ -1737,18 +1725,16 @@
> >vhost_user_backend_handle_shared_object_lookup(struct vhost_user *u,
> > {
> > struct vhost_dev *dev = virtio_lookup_vhost_device(&uuid);
> > if (dev == NULL) {
> >- payload->u64 = -EINVAL;
> >- break;
> >+ return -EINVAL;
> > }
> > int ret = vhost_user_get_shared_object(dev, uuid.data, &dmabuf_fd);
> > if (ret < 0) {
> >- payload->u64 = ret;
> >+ return ret;
> > }
> > break;
> > }
> > case TYPE_INVALID:
> >- payload->u64 = -EINVAL;
> >- break;
> >+ return -EINVAL;
>
> So, after this patch, we are not going to call
> `qemu_chr_fe_set_msgfds()` when we are returning an error to the
> backend. I guess this is even better than before, right?
Yeah, otherwise it was sending fd_num = 0 which should be safe, but it
is indeed cleaner and clearer to read.
>
> > }
> >
> > if (dmabuf_fd != -1) {
> >@@ -1757,11 +1743,6 @@
> >vhost_user_backend_handle_shared_object_lookup(struct vhost_user *u,
> >
>
> Should we call qemu_chr_fe_set_msgfds() only if fd_num > 0?
> Or should we return an error if `dmabuf_fd` is not a valid fd?
Unless I misunderstood the code, I think if fd_num = 0 the function
will not send any FD through the communication channel. But indeed, it
would be fair to return an error if `dmabuf_fd` is not valid, to let
the backend know something unexpected happened. I will prepare a
follow-up patch soonish with this :)
>
> I guess this is pre-existing and maybe should be fixed in another patch
> if it's a problem.
>
> Thanks,
> Stefano
>
> > if (qemu_chr_fe_set_msgfds(chr, &dmabuf_fd, fd_num) < 0) {
> > error_report("Failed to set msg fds.");
> >- payload->u64 = -EINVAL;
> >- }
> >-
> >- if (!vhost_user_backend_send_dmabuf_fd(ioc, hdr, payload, &local_err)) {
> >- error_report_err(local_err);
> > return -EINVAL;
> > }
> >
> >@@ -1790,6 +1771,7 @@ static gboolean backend_read(QIOChannel *ioc,
> >GIOCondition condition,
> > struct iovec iov;
> > g_autofree int *fd = NULL;
> > size_t fdsize = 0;
> >+ bool reply_ack;
> > int i;
> >
> > /* Read header */
> >@@ -1808,6 +1790,8 @@ static gboolean backend_read(QIOChannel *ioc,
> >GIOCondition condition,
> > goto err;
> > }
> >
> >+ reply_ack = hdr.flags & VHOST_USER_NEED_REPLY_MASK;
> >+
> > /* Read payload */
> > if (qio_channel_read_all(ioc, (char *) &payload, hdr.size, &local_err))
> > {
> > error_report_err(local_err);
> >@@ -1833,8 +1817,10 @@ static gboolean backend_read(QIOChannel *ioc,
> >GIOCondition condition,
> >
> > &payload.object);
> > break;
> > case VHOST_USER_BACKEND_SHARED_OBJECT_LOOKUP:
> >- ret = vhost_user_backend_handle_shared_object_lookup(dev->opaque,
> >ioc,
> >- &hdr,
> >&payload);
> >+ /* The backend always expects a response */
> >+ reply_ack = true;
> >+ ret = vhost_user_backend_handle_shared_object_lookup(dev->opaque,
> >+
> >&payload.object);
> > break;
> > default:
> > error_report("Received unexpected msg type: %d.", hdr.request);
> >@@ -1845,7 +1831,7 @@ static gboolean backend_read(QIOChannel *ioc,
> >GIOCondition condition,
> > * REPLY_ACK feature handling. Other reply types has to be managed
> > * directly in their request handlers.
> > */
> >- if (hdr.flags & VHOST_USER_NEED_REPLY_MASK) {
> >+ if (reply_ack) {
> > payload.u64 = !!ret;
> > hdr.size = sizeof(payload.u64);
> >
> >--
> >2.49.0
> >
>
