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

["Daniel P. Berrangé" <berrange@redhat.com>]

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.

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