hw/virtio/vhost-user.c | 36 +++++++++++------------------------- 1 file changed, 11 insertions(+), 25 deletions(-)
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
Signed-off-by: Albert Esteve <aesteve@redhat.com>
---
hw/virtio/vhost-user.c | 36 +++++++++++-------------------------
1 file changed, 11 insertions(+), 25 deletions(-)
diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
index 36c9c2e04d..b0bcee2d30 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)
{
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));
- 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);
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.0Thanks for the refactoring, IMO now it's much better and easy to read.
BTW, the previous title was better IMHO because it made it clear that
this is a fix, whereas now it looks more like a refactoring and could
therefore be difficult to interpret as something to be backported to
stable branches. So I suggest just to revert the title, the following
description LGTM.
On Wed, Oct 15, 2025 at 01:36:34PM +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
Should we cc stable?
If yes, please add:
Cc: qemu-stable@nongnu.org
>Signed-off-by: Albert Esteve <aesteve@redhat.com>
>---
> hw/virtio/vhost-user.c | 36 +++++++++++-------------------------
> 1 file changed, 11 insertions(+), 25 deletions(-)
>
>diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
>index 36c9c2e04d..b0bcee2d30 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)
Can we now uniform this function with the other `*_shared_object_*`
functions, passing `VhostUserShared *object` instead of the entire
payload?
The rest LGTM!
Thanks,
Stefano
> {
> 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));
>
>- 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);
> 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
>
On Wed, Oct 15, 2025 at 1:52 PM Stefano Garzarella <sgarzare@redhat.com> wrote:
>
> Thanks for the refactoring, IMO now it's much better and easy to read.
>
> BTW, the previous title was better IMHO because it made it clear that
> this is a fix, whereas now it looks more like a refactoring and could
> therefore be difficult to interpret as something to be backported to
> stable branches. So I suggest just to revert the title, the following
> description LGTM.
>
> On Wed, Oct 15, 2025 at 01:36:34PM +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
>
> Should we cc stable?
> If yes, please add:
>
> Cc: qemu-stable@nongnu.org
>
> >Signed-off-by: Albert Esteve <aesteve@redhat.com>
> >---
> > hw/virtio/vhost-user.c | 36 +++++++++++-------------------------
> > 1 file changed, 11 insertions(+), 25 deletions(-)
> >
> >diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
> >index 36c9c2e04d..b0bcee2d30 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)
>
> Can we now uniform this function with the other `*_shared_object_*`
> functions, passing `VhostUserShared *object` instead of the entire
> payload?
>
> The rest LGTM!
Agreed and addressed all your comments in the next revision :)
Thanks!
>
> Thanks,
> Stefano
>
> > {
> > 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));
> >
> >- 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);
> > 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
> >
>
© 2016 - 2025 Red Hat, Inc.