hw/virtio/vhost-user.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)
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);
}
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);
}
static bool vhost_user_send_resp(QIOChannel *ioc, VhostUserHeader *hdr,
--
2.46.1On 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 :-(
Change vhost_user_backend_handle_shared_object_add to return
a bool to reduce the madness IMHO.
>
> 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.
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 :|
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 :|
>
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 :|
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 :|
>
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é <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.
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 :|
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
s/implementaion/implementation
I hadn't seen it ;-P found with:
./scripts/checkpatch.pl --strict --branch master..HEAD --codespell
>violates the specification.
>
>Revert the logic so that the implementation
>of the vhost-user handling methods matches
>the specification.
>
>Fixes: 043e127a126bb3ceb5fc753deee27d261fd0c5ce
This is in from 9.0 ...
>Fixes: 160947666276c5b7f6bca4d746bcac2966635d79
... and this from 8.2, so should we consider stable branches?
I think it depends if the backends are checking that return value.
>Signed-off-by: Albert Esteve <aesteve@redhat.com>
>---
> hw/virtio/vhost-user.c | 8 ++++----
> 1 file changed, 4 insertions(+), 4 deletions(-)
Thanks for the fix!
Reviewed-by: Stefano Garzarella <sgarzare@redhat.com>
>
>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);
> }
>
> 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);
> }
>
> static bool vhost_user_send_resp(QIOChannel *ioc, VhostUserHeader *hdr,
>--
>2.46.1
>
Albert Esteve
Senior Software Engineer
Red Hat
aesteve@redhat.com
On Thu, Oct 17, 2024 at 9:38 AM Stefano Garzarella <sgarzare@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
>
> s/implementaion/implementation
>
> I hadn't seen it ;-P found with:
> ./scripts/checkpatch.pl --strict --branch master..HEAD --codespell
Never used the checkpatch script for spelling. Thanks!
>
> >violates the specification.
> >
> >Revert the logic so that the implementation
> >of the vhost-user handling methods matches
> >the specification.
> >
> >Fixes: 043e127a126bb3ceb5fc753deee27d261fd0c5ce
>
> This is in from 9.0 ...
>
> >Fixes: 160947666276c5b7f6bca4d746bcac2966635d79
>
> ... and this from 8.2, so should we consider stable branches?
You mean in addition to the commits already reflected here?
>
> I think it depends if the backends are checking that return value.
The return value is optional (requires VHOST_USER_NEED_REPLY),
and I am not aware of any backend using this feature so far,
in general. So iiuc, that'd mean no need to include stable, right?
Best,
Albert.
>
> >Signed-off-by: Albert Esteve <aesteve@redhat.com>
> >---
> > hw/virtio/vhost-user.c | 8 ++++----
> > 1 file changed, 4 insertions(+), 4 deletions(-)
>
> Thanks for the fix!
>
> Reviewed-by: Stefano Garzarella <sgarzare@redhat.com>
>
> >
> >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);
> > }
> >
> > 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);
> > }
> >
> > static bool vhost_user_send_resp(QIOChannel *ioc, VhostUserHeader *hdr,
> >--
> >2.46.1
> >
>
On Thu, Oct 17, 2024 at 10:27:30AM +0200, Albert Esteve wrote:
>Albert Esteve
>
>Senior Software Engineer
>
>Red Hat
>
>aesteve@redhat.com
>
>
>
>On Thu, Oct 17, 2024 at 9:38 AM Stefano Garzarella <sgarzare@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
>>
>> s/implementaion/implementation
>>
>> I hadn't seen it ;-P found with:
>> ./scripts/checkpatch.pl --strict --branch master..HEAD --codespell
>
>Never used the checkpatch script for spelling. Thanks!
>
>>
>> >violates the specification.
>> >
>> >Revert the logic so that the implementation
>> >of the vhost-user handling methods matches
>> >the specification.
>> >
>> >Fixes: 043e127a126bb3ceb5fc753deee27d261fd0c5ce
>>
>> This is in from 9.0 ...
>>
>> >Fixes: 160947666276c5b7f6bca4d746bcac2966635d79
>>
>> ... and this from 8.2, so should we consider stable branches?
>
>You mean in addition to the commits already reflected here?
Nope, I just mean if we need to cc qemu-stable@nongnu.org in order to
backport this patch on stable branches.
See docs/devel/stable-process.rst
>
>>
>> I think it depends if the backends are checking that return value.
>
>The return value is optional (requires VHOST_USER_NEED_REPLY),
>and I am not aware of any backend using this feature so far,
>in general. So iiuc, that'd mean no need to include stable, right?
Yep, if no one uses it, we can avoid it for now. On the other hand if
the patch is simple, perhaps it might make sense to avoid future issues.
Michael WDYT?
Thanks
Stefano
>
>Best,
>Albert.
>
>>
>> >Signed-off-by: Albert Esteve <aesteve@redhat.com>
>> >---
>> > hw/virtio/vhost-user.c | 8 ++++----
>> > 1 file changed, 4 insertions(+), 4 deletions(-)
>>
>> Thanks for the fix!
>>
>> Reviewed-by: Stefano Garzarella <sgarzare@redhat.com>
>>
>> >
>> >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);
>> > }
>> >
>> > 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);
>> > }
>> >
>> > static bool vhost_user_send_resp(QIOChannel *ioc, VhostUserHeader *hdr,
>> >--
>> >2.46.1
>> >
>>
>
On Thu, Oct 17, 2024 at 11:38 AM Stefano Garzarella <sgarzare@redhat.com> wrote:
>
> On Thu, Oct 17, 2024 at 10:27:30AM +0200, Albert Esteve wrote:
> >Albert Esteve
> >
> >Senior Software Engineer
> >
> >Red Hat
> >
> >aesteve@redhat.com
> >
> >
> >
> >On Thu, Oct 17, 2024 at 9:38 AM Stefano Garzarella <sgarzare@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
> >>
> >> s/implementaion/implementation
> >>
> >> I hadn't seen it ;-P found with:
> >> ./scripts/checkpatch.pl --strict --branch master..HEAD --codespell
> >
> >Never used the checkpatch script for spelling. Thanks!
> >
> >>
> >> >violates the specification.
> >> >
> >> >Revert the logic so that the implementation
> >> >of the vhost-user handling methods matches
> >> >the specification.
> >> >
> >> >Fixes: 043e127a126bb3ceb5fc753deee27d261fd0c5ce
> >>
> >> This is in from 9.0 ...
> >>
> >> >Fixes: 160947666276c5b7f6bca4d746bcac2966635d79
> >>
> >> ... and this from 8.2, so should we consider stable branches?
> >
> >You mean in addition to the commits already reflected here?
>
> Nope, I just mean if we need to cc qemu-stable@nongnu.org in order to
> backport this patch on stable branches.
> See docs/devel/stable-process.rst
Got it, thanks!
>
> >
> >>
> >> I think it depends if the backends are checking that return value.
> >
> >The return value is optional (requires VHOST_USER_NEED_REPLY),
> >and I am not aware of any backend using this feature so far,
> >in general. So iiuc, that'd mean no need to include stable, right?
>
> Yep, if no one uses it, we can avoid it for now. On the other hand if
> the patch is simple, perhaps it might make sense to avoid future issues.
I will wait until tomorrow and post a new version of the patch fixing the typo
and changing the return as suggested by Daniel.
The patch is simple, so it could make sense to include stable. But in principle,
I understood that I can avoid stable for now. If we decide otherwise and I
have already sent v2, we can add them a posteriori I guess.
BR,
Albert.
>
> Michael WDYT?
>
> Thanks
> Stefano
>
> >
> >Best,
> >Albert.
> >
> >>
> >> >Signed-off-by: Albert Esteve <aesteve@redhat.com>
> >> >---
> >> > hw/virtio/vhost-user.c | 8 ++++----
> >> > 1 file changed, 4 insertions(+), 4 deletions(-)
> >>
> >> Thanks for the fix!
> >>
> >> Reviewed-by: Stefano Garzarella <sgarzare@redhat.com>
> >>
> >> >
> >> >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);
> >> > }
> >> >
> >> > 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);
> >> > }
> >> >
> >> > static bool vhost_user_send_resp(QIOChannel *ioc, VhostUserHeader *hdr,
> >> >--
> >> >2.46.1
> >> >
> >>
> >
>
© 2016 - 2024 Red Hat, Inc.