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

[Albert Esteve <aesteve@redhat.com>]

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.

>
> >
> >  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).

BR,
Albert.

>
> On top of this inconsistency, it has all the same madness mentioneed
> above.
>
> 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 :|
>