| Message ID | 20241016090606.2358056-1-aesteve@redhat.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | vhost-user: fix shared object return values | expand |
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 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
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
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
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 > >> > > >> > > >
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,
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(-)