On Thu, Oct 17, 2024 at 11:28:56AM +0200, Albert Esteve wrote:
> On Thu, Oct 17, 2024 at 11:18 AM Daniel P. Berrangé <berra...@redhat.com>
> wrote:
> >
> > On Thu, Oct 17, 2024 at 11:12:32AM +0200, Albert Esteve wrote:
> > > On Thu, Oct 17, 2024 at 10:44 AM Daniel P. Berrangé <berra...@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
> > > > > violates the specification.
> > > > >
> > > > > Revert the logic so that the implementation
> > > > > of the vhost-user handling methods matches
> > > > > the specification.
> > > > >
> > > > > Fixes: 043e127a126bb3ceb5fc753deee27d261fd0c5ce
> > > > > Fixes: 160947666276c5b7f6bca4d746bcac2966635d79
> > > > > Signed-off-by: Albert Esteve <aest...@redhat.com>
> > > > > ---
> > > > > hw/virtio/vhost-user.c | 8 ++++----
> > > > > 1 file changed, 4 insertions(+), 4 deletions(-)
> > > > >
> > > > > 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);
> > > > > }
> > > >
> > > > This virtio_add_vhost_device() method returns a bool, but this
> > > > vhost_user_backend_handle_shared_object_add() method returns
> > > > an int, but fills that int with an inverted bool value. The
> > > > caller then assigns the return value to an int, but then
> > > > interprets the int as a bool, and assigns that bool result
> > > > to an u64.
> > > >
> > > > This call chain is madness :-(
> > >
> > > TBF most of the madness is part of the already existing
> > > handling infrastructure.
> > > vhost_user_backend_handle_shared_object_add()
> > > returns an int to be consistent with other handling
> > > functions.
> > >
> > > >
> > > > Change vhost_user_backend_handle_shared_object_add to return
> > > > a bool to reduce the madness IMHO.
> > >
> > > Changing it to bool would make it inconsistent
> > > wrt other handlers, and the casting would happen nonetheless
> > > on assignment. Not sure if that is an improvement.
> >
> > Well when the caller does
> >
> > payload.u64 = !!ret;
> >
> > it is saying that it only cares about the values
> > being 0 or 1. So how about just making these
> > methods return 0 or 1 then.
>
> Ah, I see your point. I introduced negative error
> values just because I saw other handlers doing
> it (e.g., vhost_user_backend_handle_vring_host_notifier()).
>
> > > > > 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;
>
> So you are suggesting here it could be `return 1;` instead?
> It does not look clear enough that it is an error value.
Add API documentation comments to these methods
"Returns: 0 on success, 1 on error"
With regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
