On Thu, Oct 17, 2024 at 10:27:30AM +0200, Albert Esteve wrote:
Albert Esteve
Senior Software Engineer
Red Hat
aest...@redhat.com
On Thu, Oct 17, 2024 at 9:38 AM Stefano Garzarella <sgarz...@redhat.com> wrote:
On Wed, Oct 16, 2024 at 11:06:06AM +0200, Albert Esteve wrote:
>VHOST_USER_BACKEND_SHARED_OBJECT_ADD and
>VHOST_USER_BACKEND_SHARED_OBJECT_REMOVE state
>in the spec that they return 0 for successful
>operations, non-zero otherwise. However,
>implementation relies on the return types
>of the virtio-dmabuf library, with opposite
>semantics (true if everything is correct,
>false otherwise). Therefore, current implementaion
s/implementaion/implementation
I hadn't seen it ;-P found with:
./scripts/checkpatch.pl --strict --branch master..HEAD --codespell
Never used the checkpatch script for spelling. Thanks!
>violates the specification.
>
>Revert the logic so that the implementation
>of the vhost-user handling methods matches
>the specification.
>
>Fixes: 043e127a126bb3ceb5fc753deee27d261fd0c5ce
This is in from 9.0 ...
>Fixes: 160947666276c5b7f6bca4d746bcac2966635d79
... and this from 8.2, so should we consider stable branches?
You mean in addition to the commits already reflected here?
Nope, I just mean if we need to cc qemu-sta...@nongnu.org in order to
backport this patch on stable branches.
See docs/devel/stable-process.rst
I think it depends if the backends are checking that return value.
The return value is optional (requires VHOST_USER_NEED_REPLY),
and I am not aware of any backend using this feature so far,
in general. So iiuc, that'd mean no need to include stable, right?
Yep, if no one uses it, we can avoid it for now. On the other hand if
the patch is simple, perhaps it might make sense to avoid future issues.
Michael WDYT?
Thanks
Stefano
Best,
Albert.
>Signed-off-by: Albert Esteve <aest...@redhat.com>
>---
> hw/virtio/vhost-user.c | 8 ++++----
> 1 file changed, 4 insertions(+), 4 deletions(-)
Thanks for the fix!
Reviewed-by: Stefano Garzarella <sgarz...@redhat.com>
>
>diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
>index 00561daa06..90917352a4 100644
>--- a/hw/virtio/vhost-user.c
>+++ b/hw/virtio/vhost-user.c
>@@ -1607,7 +1607,7 @@ vhost_user_backend_handle_shared_object_add(struct
vhost_dev *dev,
> QemuUUID uuid;
>
> memcpy(uuid.data, object->uuid, sizeof(object->uuid));
>- return virtio_add_vhost_device(&uuid, dev);
>+ return !virtio_add_vhost_device(&uuid, dev);
> }
>
> static int
>@@ -1623,16 +1623,16 @@ vhost_user_backend_handle_shared_object_remove(struct
vhost_dev *dev,
> struct vhost_dev *owner = virtio_lookup_vhost_device(&uuid);
> if (dev != owner) {
> /* Not allowed to remove non-owned entries */
>- return 0;
>+ return -EPERM;
> }
> break;
> }
> default:
> /* Not allowed to remove non-owned entries */
>- return 0;
>+ return -EPERM;
> }
>
>- return virtio_remove_resource(&uuid);
>+ return !virtio_remove_resource(&uuid);
> }
>
> static bool vhost_user_send_resp(QIOChannel *ioc, VhostUserHeader *hdr,
>--
>2.46.1
>
