* [PATCH] vhost-user: fix shared object lookup handler logic @ 2025-10-15 9:19 Albert Esteve 2025-10-15 10:02 ` Stefano Garzarella 0 siblings, 1 reply; 3+ messages in thread From: Albert Esteve @ 2025-10-15 9:19 UTC (permalink / raw) To: qemu-devel; +Cc: Stefano Garzarella, Michael S. Tsirkin, Albert Esteve 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 ^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] vhost-user: fix shared object lookup handler logic 2025-10-15 9:19 [PATCH] vhost-user: fix shared object lookup handler logic Albert Esteve @ 2025-10-15 10:02 ` Stefano Garzarella 2025-10-15 11:00 ` Albert Esteve 0 siblings, 1 reply; 3+ messages in thread From: Stefano Garzarella @ 2025-10-15 10:02 UTC (permalink / raw) To: Albert Esteve; +Cc: qemu-devel, Michael S. Tsirkin 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 > ^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] vhost-user: fix shared object lookup handler logic 2025-10-15 10:02 ` Stefano Garzarella @ 2025-10-15 11:00 ` Albert Esteve 0 siblings, 0 replies; 3+ messages in thread From: Albert Esteve @ 2025-10-15 11:00 UTC (permalink / raw) To: Stefano Garzarella; +Cc: qemu-devel, Michael S. Tsirkin 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 > > > ^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2025-10-15 11:02 UTC | newest] Thread overview: 3+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2025-10-15 9:19 [PATCH] vhost-user: fix shared object lookup handler logic Albert Esteve 2025-10-15 10:02 ` Stefano Garzarella 2025-10-15 11:00 ` Albert Esteve
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).