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
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

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.)

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
>

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
> >
>