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.
>
> >
> > >
> > > 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);
> > > }
> >
> > These return values are inconsistent.
> >
> > In some places you're returning a negative errno, but in this
> > last place you're returning true or false, by calling
> > virtio_remove_resource which is a 'bool' method & inverting it.
>
> Well, specification only distinguish between zero and non-zero values.
> But for clarity, I guess I could do something like:
> ```
> if (!virtio_remove_resource(&uuid)) {
> return -EINVAL;
> }
>
> return 0;
> ```
>
> Same for the vhost_user_backend_handle_shared_object_add()
> handler (in that case there is no inconsistency with positive or negative
> return values, but still better to maintain similar strategy for all
> handlers).
Returning an errno value, when the caller only wants 0 or 1 is
pointless.
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 :|
