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?
>
> 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?
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
> >
>
