Re: [PATCH] vhost-user: fix shared object lookup handler logic

[Albert Esteve <aesteve@redhat.com>]

On Wed, Oct 15, 2025 at 12:02 PM Stefano Garzarella <sgarzare@redhat.com> wrote:
>
> On Wed, Oct 15, 2025 at 11:19:55AM +0200, Albert Esteve wrote:
> >Fix vhost_user_backend_handle_shared_object_lookup() logic to handle
> >the error path the same way as other handlers do. The main
> >difference between them is that shared_object_lookup handler
> >sends the reply from within the handler itself.
> >
> >What vhost_user_backend_handle_shared_object_lookup() returns, depends
> >on whether vhost_user_backend_send_dmabuf_fd() succeded or not to send
> >a reply. Any check that results in an error before that only
> >determines the return value in the response. However, when an error
> >in sending the response within the handler occurs, we want to jump
> >to err and close the backend channel to be consistent with other message
> >types. On the other hand, when the response succeds then the
> >VHOST_USER_NEED_REPLY_MASK flag is unset and the reply in backend_read
> >is skipped, going directly to the fdcleanup.
> >
> >Fixes: 160947666276c5b7f6bca4d746bcac2966635d79
>
> Looking at that commit, I honestly don't understand why the reply is
> handled differently for
> vhost_user_backend_handle_shared_object_lookup().
> Why can't we handle it here like we do for all the other calls?
>
> If the backend always expects a response for that, can't we do something
> like this? (And of course
> vhost_user_backend_handle_shared_object_lookup() returns the value
> instead of touching the payload.)

Exactly, if I remember correctly (and judging by the specs and code
it's like that), the reason is that this handler needed a response
unconditionally. Your proposal looks good to me, and we can clean the
dmabuf_fd static function, which does not do much either.

Let me send a quick v2, thanks for checking.

>
> diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
> index 36c9c2e04d..da874c4add 100644
> --- a/hw/virtio/vhost-user.c
> +++ b/hw/virtio/vhost-user.c
> @@ -1790,6 +1790,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 +1809,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,6 +1836,8 @@ static gboolean backend_read(QIOChannel *ioc, GIOCondition condition,
>                                                                &payload.object);
>           break;
>       case VHOST_USER_BACKEND_SHARED_OBJECT_LOOKUP:
> +        /* The backend always expects a response (XXX: is that right?) */
> +        reply_ack = true;
>           ret = vhost_user_backend_handle_shared_object_lookup(dev->opaque, ioc,
>                                                                &hdr, &payload);
>           break;
> @@ -1845,7 +1850,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);
>
> Thanks,
> Stefano
>
> >Signed-off-by: Albert Esteve <aesteve@redhat.com>
> >---
> > hw/virtio/vhost-user.c | 7 +++++--
> > 1 file changed, 5 insertions(+), 2 deletions(-)
> >
> >diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
> >index 36c9c2e04d..163c3d8ca5 100644
> >--- a/hw/virtio/vhost-user.c
> >+++ b/hw/virtio/vhost-user.c
> >@@ -1833,8 +1833,11 @@ 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);
> >+        /* Handler manages its own response, check error and close connection */
> >+        if (vhost_user_backend_handle_shared_object_lookup(dev->opaque, ioc,
> >+                                                           &hdr, &payload) < 0) {
> >+            goto err;
> >+        }
> >         break;
> >     default:
> >         error_report("Received unexpected msg type: %d.", hdr.request);
> >--
> >2.49.0
> >
>