From: Albert Esteve <aesteve@redhat.com>
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: 1609476662 ("vhost-user: add shared_object msg")
Cc: qemu-stable@nongnu.org
Signed-off-by: Albert Esteve <aesteve@redhat.com>
Reviewed-by: Stefano Garzarella <sgarzare@redhat.com>
Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
Message-Id: <20251017072011.1874874-2-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 aac98f898a..4b0fae12ae 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;
CharFrontend *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);
--
MSTOn Sun, Nov 9, 2025 at 3:35 PM Michael S. Tsirkin <mst@redhat.com> wrote:
>
> From: Albert Esteve <aesteve@redhat.com>
>
> 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.
Hey Michal,
This patch was
Based-on: <20251016143827.1850397-1-aesteve@redhat.com>
… for main.
As this was the first time I did this based-on thingy, I am just
making sure that the other patch was not missed.
If this PULL is only targeting stable, then it's ok as is.
BR,
Albert
>
> Fixes: 1609476662 ("vhost-user: add shared_object msg")
> Cc: qemu-stable@nongnu.org
> Signed-off-by: Albert Esteve <aesteve@redhat.com>
> Reviewed-by: Stefano Garzarella <sgarzare@redhat.com>
> Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> Message-Id: <20251017072011.1874874-2-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 aac98f898a..4b0fae12ae 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;
> CharFrontend *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);
>
> --
> MST
>
On Mon, Nov 10, 2025 at 10:23:25AM +0100, Albert Esteve wrote:
> On Sun, Nov 9, 2025 at 3:35 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> >
> > From: Albert Esteve <aesteve@redhat.com>
> >
> > 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.
>
> Hey Michal,
>
> This patch was
> Based-on: <20251016143827.1850397-1-aesteve@redhat.com>
> … for main.
That's the SHMEM thing right? Yes but I rebased it dropping
the SHMEM dependency.
At least, I think I did it correctly.
> As this was the first time I did this based-on thingy, I am just
> making sure that the other patch was not missed.
> If this PULL is only targeting stable, then it's ok as is.
It is targeting 10.2 which is in freeze. So equivalently same.
> BR,
> Albert
>
> >
> > Fixes: 1609476662 ("vhost-user: add shared_object msg")
> > Cc: qemu-stable@nongnu.org
> > Signed-off-by: Albert Esteve <aesteve@redhat.com>
> > Reviewed-by: Stefano Garzarella <sgarzare@redhat.com>
> > Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > Message-Id: <20251017072011.1874874-2-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 aac98f898a..4b0fae12ae 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;
> > CharFrontend *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);
> >
> > --
> > MST
> >
On Mon, Nov 10, 2025 at 4:42 PM Michael S. Tsirkin <mst@redhat.com> wrote:
>
> On Mon, Nov 10, 2025 at 10:23:25AM +0100, Albert Esteve wrote:
> > On Sun, Nov 9, 2025 at 3:35 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> > >
> > > From: Albert Esteve <aesteve@redhat.com>
> > >
> > > 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.
> >
> > Hey Michal,
> >
> > This patch was
> > Based-on: <20251016143827.1850397-1-aesteve@redhat.com>
> > … for main.
>
> That's the SHMEM thing right? Yes but I rebased it dropping
> the SHMEM dependency.
>
> At least, I think I did it correctly.
Yes, removing the dependency is correctly applied. But that was only
required for the backport to stable.
If we merge this patch to main without the one it is based on, then
I'd need to send a new version of the SHMEM patch with the block that
you have dropped. I can do it, but I was trying to prioritize the
other one, as it was a lot harder to get approved. That is why I based
this patch on top of the SHMEM one and not the other way around.
Sorry if that was not clear from the message.
>
> > As this was the first time I did this based-on thingy, I am just
> > making sure that the other patch was not missed.
> > If this PULL is only targeting stable, then it's ok as is.
>
> It is targeting 10.2 which is in freeze. So equivalently same.
>
>
> > BR,
> > Albert
> >
> > >
> > > Fixes: 1609476662 ("vhost-user: add shared_object msg")
> > > Cc: qemu-stable@nongnu.org
> > > Signed-off-by: Albert Esteve <aesteve@redhat.com>
> > > Reviewed-by: Stefano Garzarella <sgarzare@redhat.com>
> > > Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
> > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > > Message-Id: <20251017072011.1874874-2-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 aac98f898a..4b0fae12ae 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;
> > > CharFrontend *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);
> > >
> > > --
> > > MST
> > >
>
On Mon, Nov 10, 2025 at 04:57:51PM +0100, Albert Esteve wrote:
> On Mon, Nov 10, 2025 at 4:42 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> >
> > On Mon, Nov 10, 2025 at 10:23:25AM +0100, Albert Esteve wrote:
> > > On Sun, Nov 9, 2025 at 3:35 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> > > >
> > > > From: Albert Esteve <aesteve@redhat.com>
> > > >
> > > > 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.
> > >
> > > Hey Michal,
> > >
> > > This patch was
> > > Based-on: <20251016143827.1850397-1-aesteve@redhat.com>
> > > … for main.
> >
> > That's the SHMEM thing right? Yes but I rebased it dropping
> > the SHMEM dependency.
> >
> > At least, I think I did it correctly.
>
> Yes, removing the dependency is correctly applied. But that was only
> required for the backport to stable.
>
> If we merge this patch to main without the one it is based on, then
> I'd need to send a new version of the SHMEM patch with the block that
> you have dropped. I can do it, but I was trying to prioritize the
> other one, as it was a lot harder to get approved. That is why I based
> this patch on top of the SHMEM one and not the other way around.
>
> Sorry if that was not clear from the message.
Right but I can't apply SHMEM patch in freeze so yes, it has to go
on top. Sorry it's like this.
> >
> > > As this was the first time I did this based-on thingy, I am just
> > > making sure that the other patch was not missed.
> > > If this PULL is only targeting stable, then it's ok as is.
> >
> > It is targeting 10.2 which is in freeze. So equivalently same.
> >
> >
> > > BR,
> > > Albert
> > >
> > > >
> > > > Fixes: 1609476662 ("vhost-user: add shared_object msg")
> > > > Cc: qemu-stable@nongnu.org
> > > > Signed-off-by: Albert Esteve <aesteve@redhat.com>
> > > > Reviewed-by: Stefano Garzarella <sgarzare@redhat.com>
> > > > Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
> > > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > > > Message-Id: <20251017072011.1874874-2-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 aac98f898a..4b0fae12ae 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;
> > > > CharFrontend *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);
> > > >
> > > > --
> > > > MST
> > > >
> >
On Mon, Nov 10, 2025 at 5:06 PM Michael S. Tsirkin <mst@redhat.com> wrote:
>
> On Mon, Nov 10, 2025 at 04:57:51PM +0100, Albert Esteve wrote:
> > On Mon, Nov 10, 2025 at 4:42 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> > >
> > > On Mon, Nov 10, 2025 at 10:23:25AM +0100, Albert Esteve wrote:
> > > > On Sun, Nov 9, 2025 at 3:35 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> > > > >
> > > > > From: Albert Esteve <aesteve@redhat.com>
> > > > >
> > > > > 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.
> > > >
> > > > Hey Michal,
> > > >
> > > > This patch was
> > > > Based-on: <20251016143827.1850397-1-aesteve@redhat.com>
> > > > … for main.
> > >
> > > That's the SHMEM thing right? Yes but I rebased it dropping
> > > the SHMEM dependency.
> > >
> > > At least, I think I did it correctly.
> >
> > Yes, removing the dependency is correctly applied. But that was only
> > required for the backport to stable.
> >
> > If we merge this patch to main without the one it is based on, then
> > I'd need to send a new version of the SHMEM patch with the block that
> > you have dropped. I can do it, but I was trying to prioritize the
> > other one, as it was a lot harder to get approved. That is why I based
> > this patch on top of the SHMEM one and not the other way around.
> >
> > Sorry if that was not clear from the message.
>
>
> Right but I can't apply SHMEM patch in freeze so yes, it has to go
> on top. Sorry it's like this.
Got it. Then it'll have to be the other way around. Thanks for
handling the rebase, then. I'll send the new version of the SHMEM
patch after this one lands.
>
> > >
> > > > As this was the first time I did this based-on thingy, I am just
> > > > making sure that the other patch was not missed.
> > > > If this PULL is only targeting stable, then it's ok as is.
> > >
> > > It is targeting 10.2 which is in freeze. So equivalently same.
> > >
> > >
> > > > BR,
> > > > Albert
> > > >
> > > > >
> > > > > Fixes: 1609476662 ("vhost-user: add shared_object msg")
> > > > > Cc: qemu-stable@nongnu.org
> > > > > Signed-off-by: Albert Esteve <aesteve@redhat.com>
> > > > > Reviewed-by: Stefano Garzarella <sgarzare@redhat.com>
> > > > > Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
> > > > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > > > > Message-Id: <20251017072011.1874874-2-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 aac98f898a..4b0fae12ae 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;
> > > > > CharFrontend *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);
> > > > >
> > > > > --
> > > > > MST
> > > > >
> > >
>
On 11/10/25 10:23, Albert Esteve wrote: > On Sun, Nov 9, 2025 at 3:35 PM Michael S. Tsirkin <mst@redhat.com> wrote: >> >> From: Albert Esteve <aesteve@redhat.com> >> >> 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. > > Hey Michal, > > This patch was > Based-on: <20251016143827.1850397-1-aesteve@redhat.com> > … for main. > > As this was the first time I did this based-on thingy, I am just > making sure that the other patch was not missed. > If this PULL is only targeting stable, then it's ok as is. This PR is targeting master, and "vhost-user: Add SHMEM_MAP/UNMAP requests" is not present. Albert, thanks for noticing. Michael, I'll hold off on applying this PR for the moment. r~
© 2016 - 2025 Red Hat, Inc.