On Thu, Oct 17, 2024 at 11:38 AM Stefano Garzarella <sgarz...@redhat.com> wrote:
>
> 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
Got it, thanks!
>
> >
> >>
> >> 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.
I will wait until tomorrow and post a new version of the patch fixing the typo
and changing the return as suggested by Daniel.
The patch is simple, so it could make sense to include stable. But in principle,
I understood that I can avoid stable for now. If we decide otherwise and I
have already sent v2, we can add them a posteriori I guess.
BR,
Albert.
>
> 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
> >> >
> >>
> >
>
