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]>
Thanks, I will send a new version of the patch to base it on https://lists.gnu.org/archive/html/qemu-devel/2025-10/msg04130.html. The other patch relies on the REPLY_MASK flag, so I need to add a `reply_ack = false;` or else it'd send two replies. > > > 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? > > > } > > > > 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? > > 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 > > >
