Re: [PATCH] vhost-user: fix shared object return values

[Albert Esteve <aesteve@redhat.com>]

On Thu, Oct 17, 2024 at 11:18 AM Daniel P. Berrangé <berrange@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é <berrange@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 <aesteve@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.

Maybe I could create a define like:

#define EVHOST_USER 1

and use it here (and probably a good idea to change other
handling functions in a different commit, to be consistent).
WDYT?

BR,
Albert.

> > > >          }
> > > >          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 :|
>