Re: [PULL 04/14] vhost-user: fix shared object lookup handler logic

["Michael S. Tsirkin" <mst@redhat.com>]

On Mon, Nov 10, 2025 at 10:23:25AM +0100, Albert Esteve wrote:
> On Sun, Nov 9, 2025 at 3:35 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> >
> > From: Albert Esteve <aesteve@redhat.com>
> >
> > 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.
> 
> Hey Michal,
> 
> This patch was
> Based-on: <20251016143827.1850397-1-aesteve@redhat.com>
> … for main.

That's the SHMEM thing right?  Yes but I rebased it dropping
the SHMEM dependency.

At least, I think I did it correctly.

> As this was the first time I did this based-on thingy, I am just
> making sure that the other patch was not missed.
> If this PULL is only targeting stable, then it's ok as is.

It is targeting 10.2 which is in freeze. So equivalently same.


> BR,
> Albert
> 
> >
> > Fixes: 1609476662 ("vhost-user: add shared_object msg")
> > Cc: qemu-stable@nongnu.org
> > Signed-off-by: Albert Esteve <aesteve@redhat.com>
> > Reviewed-by: Stefano Garzarella <sgarzare@redhat.com>
> > Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > Message-Id: <20251017072011.1874874-2-aesteve@redhat.com>
> > ---
> >  hw/virtio/vhost-user.c | 40 +++++++++++++---------------------------
> >  1 file changed, 13 insertions(+), 27 deletions(-)
> >
> > diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
> > index aac98f898a..4b0fae12ae 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;
> >      CharFrontend *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;
> >      }
> >
> >      if (dmabuf_fd != -1) {
> > @@ -1757,11 +1743,6 @@ vhost_user_backend_handle_shared_object_lookup(struct vhost_user *u,
> >
> >      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);
> >
> > --
> > MST
> >