* [PATCH v3] vhost-user: fix shared object lookup handler logic
@ 2025-10-15 12:43 Albert Esteve
2025-10-15 12:57 ` Stefano Garzarella
0 siblings, 1 reply; 4+ messages in thread
From: Albert Esteve @ 2025-10-15 12:43 UTC (permalink / raw)
To: qemu-devel
Cc: Michael S. Tsirkin, Stefano Garzarella, Albert Esteve,
qemu-stable
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: qemu-stable@nongnu.org
Signed-off-by: Albert Esteve <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 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;
}
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);
--
2.49.0
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH v3] vhost-user: fix shared object lookup handler logic
2025-10-15 12:43 [PATCH v3] vhost-user: fix shared object lookup handler logic Albert Esteve
@ 2025-10-15 12:57 ` Stefano Garzarella
2025-10-16 8:20 ` Albert Esteve
2025-10-17 7:03 ` Albert Esteve
0 siblings, 2 replies; 4+ messages in thread
From: Stefano Garzarella @ 2025-10-15 12:57 UTC (permalink / raw)
To: Albert Esteve; +Cc: qemu-devel, Michael S. Tsirkin, qemu-stable
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: qemu-stable@nongnu.org
>Signed-off-by: Albert Esteve <aesteve@redhat.com>
>---
> hw/virtio/vhost-user.c | 40 +++++++++++++---------------------------
> 1 file changed, 13 insertions(+), 27 deletions(-)
Thanks! This patch LGTM, so
Reviewed-by: Stefano Garzarella <sgarzare@redhat.com>
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
>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v3] vhost-user: fix shared object lookup handler logic
2025-10-15 12:57 ` Stefano Garzarella
@ 2025-10-16 8:20 ` Albert Esteve
2025-10-17 7:03 ` Albert Esteve
1 sibling, 0 replies; 4+ messages in thread
From: Albert Esteve @ 2025-10-16 8:20 UTC (permalink / raw)
To: Stefano Garzarella; +Cc: qemu-devel, Michael S. Tsirkin, qemu-stable
On Wed, Oct 15, 2025 at 2:57 PM Stefano Garzarella <sgarzare@redhat.com> 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: qemu-stable@nongnu.org
> >Signed-off-by: Albert Esteve <aesteve@redhat.com>
> >---
> > hw/virtio/vhost-user.c | 40 +++++++++++++---------------------------
> > 1 file changed, 13 insertions(+), 27 deletions(-)
>
> Thanks! This patch LGTM, so
>
> Reviewed-by: Stefano Garzarella <sgarzare@redhat.com>
>
>
> 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?
Yeah, otherwise it was sending fd_num = 0 which should be safe, but it
is indeed cleaner and clearer to read.
>
> > }
> >
> > 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?
Unless I misunderstood the code, I think if fd_num = 0 the function
will not send any FD through the communication channel. But indeed, it
would be fair to return an error if `dmabuf_fd` is not valid, to let
the backend know something unexpected happened. I will prepare a
follow-up patch soonish with this :)
>
> 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
> >
>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v3] vhost-user: fix shared object lookup handler logic
2025-10-15 12:57 ` Stefano Garzarella
2025-10-16 8:20 ` Albert Esteve
@ 2025-10-17 7:03 ` Albert Esteve
1 sibling, 0 replies; 4+ messages in thread
From: Albert Esteve @ 2025-10-17 7:03 UTC (permalink / raw)
To: Stefano Garzarella; +Cc: qemu-devel, Michael S. Tsirkin, qemu-stable
On Wed, Oct 15, 2025 at 2:57 PM Stefano Garzarella <sgarzare@redhat.com> 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: qemu-stable@nongnu.org
> >Signed-off-by: Albert Esteve <aesteve@redhat.com>
> >---
> > hw/virtio/vhost-user.c | 40 +++++++++++++---------------------------
> > 1 file changed, 13 insertions(+), 27 deletions(-)
>
> Thanks! This patch LGTM, so
>
> Reviewed-by: Stefano Garzarella <sgarzare@redhat.com>
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
> >
>
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2025-10-17 7:04 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-10-15 12:43 [PATCH v3] vhost-user: fix shared object lookup handler logic Albert Esteve
2025-10-15 12:57 ` Stefano Garzarella
2025-10-16 8:20 ` Albert Esteve
2025-10-17 7:03 ` 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).