All of lore.kernel.org
 help / color / mirror / Atom feed
From: Stefan Hajnoczi <stefanha@redhat.com>
To: "Dr. David Alan Gilbert (git)" <dgilbert@redhat.com>
Cc: virtio-fs@redhat.com, qemu-devel@nongnu.org, vgoyal@redhat.com,
	groug@kaod.org
Subject: Re: [PATCH v3 03/26] DAX: vhost-user: Rework slave return values
Date: Tue, 4 May 2021 16:23:38 +0100	[thread overview]
Message-ID: <YJFm+hocgArYnWAB@stefanha-x1.localdomain> (raw)
In-Reply-To: <20210428110100.27757-4-dgilbert@redhat.com>

[-- Attachment #1: Type: text/plain, Size: 2172 bytes --]

On Wed, Apr 28, 2021 at 12:00:37PM +0100, Dr. David Alan Gilbert (git) wrote:
> From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> 
> All the current slave handlers on the qemu side generate an 'int'
> return value that's squashed down to a bool (!!ret) and stuffed into
> a uint64_t (field of a union) to be returned.
> 
> Move the uint64_t type back up through the individual handlers so
> that we can make one actually return a full uint64_t.
> 
> Note that the definition in the interop spec says most of these
> cases are defined as returning 0 on success and non-0 for failure,
> so it's OK to change from a bool to another non-0.
> 
> Vivek:
> This is needed because upcoming patches in series will add new functions
> which want to return full error code. Existing functions continue to
> return true/false so, it should not lead to change of behavior for
> existing users.
> 
> Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> Reviewed-by: Greg Kurz <groug@kaod.org>
> ---
>  hw/virtio/vhost-backend.c         |  6 +++---
>  hw/virtio/vhost-user.c            | 31 ++++++++++++++++---------------
>  include/hw/virtio/vhost-backend.h |  2 +-
>  3 files changed, 20 insertions(+), 19 deletions(-)
> 
> diff --git a/hw/virtio/vhost-backend.c b/hw/virtio/vhost-backend.c
> index 31b33bde37..1686c94767 100644
> --- a/hw/virtio/vhost-backend.c
> +++ b/hw/virtio/vhost-backend.c
> @@ -401,8 +401,8 @@ int vhost_backend_invalidate_device_iotlb(struct vhost_dev *dev,
>      return -ENODEV;
>  }
>  
> -int vhost_backend_handle_iotlb_msg(struct vhost_dev *dev,
> -                                          struct vhost_iotlb_msg *imsg)
> +uint64_t vhost_backend_handle_iotlb_msg(struct vhost_dev *dev,
> +                                        struct vhost_iotlb_msg *imsg)
>  {
>      int ret = 0;

This patch is messy. We want uint64_t but these functions use int ret
internally. The actual return values are true/false instead of int 0 and
1.

Please use uint64_t everywhere: return 0 for success and 1 for failure
instead of bool literals and change the type of the local ret variables
to uint64_t.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

WARNING: multiple messages have this Message-ID (diff)
From: Stefan Hajnoczi <stefanha@redhat.com>
To: "Dr. David Alan Gilbert (git)" <dgilbert@redhat.com>
Cc: virtio-fs@redhat.com, qemu-devel@nongnu.org, vgoyal@redhat.com
Subject: Re: [Virtio-fs] [PATCH v3 03/26] DAX: vhost-user: Rework slave return values
Date: Tue, 4 May 2021 16:23:38 +0100	[thread overview]
Message-ID: <YJFm+hocgArYnWAB@stefanha-x1.localdomain> (raw)
In-Reply-To: <20210428110100.27757-4-dgilbert@redhat.com>

[-- Attachment #1: Type: text/plain, Size: 2172 bytes --]

On Wed, Apr 28, 2021 at 12:00:37PM +0100, Dr. David Alan Gilbert (git) wrote:
> From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> 
> All the current slave handlers on the qemu side generate an 'int'
> return value that's squashed down to a bool (!!ret) and stuffed into
> a uint64_t (field of a union) to be returned.
> 
> Move the uint64_t type back up through the individual handlers so
> that we can make one actually return a full uint64_t.
> 
> Note that the definition in the interop spec says most of these
> cases are defined as returning 0 on success and non-0 for failure,
> so it's OK to change from a bool to another non-0.
> 
> Vivek:
> This is needed because upcoming patches in series will add new functions
> which want to return full error code. Existing functions continue to
> return true/false so, it should not lead to change of behavior for
> existing users.
> 
> Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> Reviewed-by: Greg Kurz <groug@kaod.org>
> ---
>  hw/virtio/vhost-backend.c         |  6 +++---
>  hw/virtio/vhost-user.c            | 31 ++++++++++++++++---------------
>  include/hw/virtio/vhost-backend.h |  2 +-
>  3 files changed, 20 insertions(+), 19 deletions(-)
> 
> diff --git a/hw/virtio/vhost-backend.c b/hw/virtio/vhost-backend.c
> index 31b33bde37..1686c94767 100644
> --- a/hw/virtio/vhost-backend.c
> +++ b/hw/virtio/vhost-backend.c
> @@ -401,8 +401,8 @@ int vhost_backend_invalidate_device_iotlb(struct vhost_dev *dev,
>      return -ENODEV;
>  }
>  
> -int vhost_backend_handle_iotlb_msg(struct vhost_dev *dev,
> -                                          struct vhost_iotlb_msg *imsg)
> +uint64_t vhost_backend_handle_iotlb_msg(struct vhost_dev *dev,
> +                                        struct vhost_iotlb_msg *imsg)
>  {
>      int ret = 0;

This patch is messy. We want uint64_t but these functions use int ret
internally. The actual return values are true/false instead of int 0 and
1.

Please use uint64_t everywhere: return 0 for success and 1 for failure
instead of bool literals and change the type of the local ret variables
to uint64_t.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

  reply	other threads:[~2021-05-04 15:37 UTC|newest]

Thread overview: 132+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-04-28 11:00 [PATCH v3 00/26] virtiofs dax patches Dr. David Alan Gilbert (git)
2021-04-28 11:00 ` [Virtio-fs] " Dr. David Alan Gilbert (git)
2021-04-28 11:00 ` [PATCH v3 01/26] virtiofs: Fixup printf args Dr. David Alan Gilbert (git)
2021-04-28 11:00   ` [Virtio-fs] " Dr. David Alan Gilbert (git)
2021-05-04 14:54   ` Stefan Hajnoczi
2021-05-04 14:54     ` [Virtio-fs] " Stefan Hajnoczi
2021-05-05 11:06     ` Dr. David Alan Gilbert
2021-05-05 11:06       ` [Virtio-fs] " Dr. David Alan Gilbert
2021-05-06 15:56   ` Dr. David Alan Gilbert
2021-05-06 15:56     ` [Virtio-fs] " Dr. David Alan Gilbert
2021-04-28 11:00 ` [PATCH v3 02/26] virtiofsd: Don't assume header layout Dr. David Alan Gilbert (git)
2021-04-28 11:00   ` [Virtio-fs] " Dr. David Alan Gilbert (git)
2021-05-04 15:12   ` Stefan Hajnoczi
2021-05-04 15:12     ` [Virtio-fs] " Stefan Hajnoczi
2021-05-06 15:56   ` Dr. David Alan Gilbert
2021-05-06 15:56     ` [Virtio-fs] " Dr. David Alan Gilbert
2021-04-28 11:00 ` [PATCH v3 03/26] DAX: vhost-user: Rework slave return values Dr. David Alan Gilbert (git)
2021-04-28 11:00   ` [Virtio-fs] " Dr. David Alan Gilbert (git)
2021-05-04 15:23   ` Stefan Hajnoczi [this message]
2021-05-04 15:23     ` Stefan Hajnoczi
2021-05-27 15:59     ` Dr. David Alan Gilbert
2021-05-27 15:59       ` [Virtio-fs] " Dr. David Alan Gilbert
2021-04-28 11:00 ` [PATCH v3 04/26] DAX: libvhost-user: Route slave message payload Dr. David Alan Gilbert (git)
2021-04-28 11:00   ` [Virtio-fs] " Dr. David Alan Gilbert (git)
2021-05-04 15:26   ` Stefan Hajnoczi
2021-05-04 15:26     ` [Virtio-fs] " Stefan Hajnoczi
2021-04-28 11:00 ` [PATCH v3 05/26] DAX: libvhost-user: Allow popping a queue element with bad pointers Dr. David Alan Gilbert (git)
2021-04-28 11:00   ` [Virtio-fs] " Dr. David Alan Gilbert (git)
2021-04-28 11:00 ` [PATCH v3 06/26] DAX subprojects/libvhost-user: Add virtio-fs slave types Dr. David Alan Gilbert (git)
2021-04-28 11:00   ` [Virtio-fs] " Dr. David Alan Gilbert (git)
2021-04-29 15:48   ` Dr. David Alan Gilbert
2021-04-29 15:48     ` [Virtio-fs] " Dr. David Alan Gilbert
2021-04-28 11:00 ` [PATCH v3 07/26] DAX: virtio: Add shared memory capability Dr. David Alan Gilbert (git)
2021-04-28 11:00   ` [Virtio-fs] " Dr. David Alan Gilbert (git)
2021-04-28 11:00 ` [PATCH v3 08/26] DAX: virtio-fs: Add cache BAR Dr. David Alan Gilbert (git)
2021-04-28 11:00   ` [Virtio-fs] " Dr. David Alan Gilbert (git)
2021-05-05 12:12   ` Stefan Hajnoczi
2021-05-05 12:12     ` [Virtio-fs] " Stefan Hajnoczi
2021-05-05 18:59     ` Dr. David Alan Gilbert
2021-05-05 18:59       ` [Virtio-fs] " Dr. David Alan Gilbert
2021-04-28 11:00 ` [PATCH v3 09/26] DAX: virtio-fs: Add vhost-user slave commands for mapping Dr. David Alan Gilbert (git)
2021-04-28 11:00   ` [Virtio-fs] " Dr. David Alan Gilbert (git)
2021-05-05 14:15   ` Stefan Hajnoczi
2021-05-05 14:15     ` [Virtio-fs] " Stefan Hajnoczi
2021-05-27 16:57     ` Dr. David Alan Gilbert
2021-05-27 16:57       ` [Virtio-fs] " Dr. David Alan Gilbert
2021-04-28 11:00 ` [PATCH v3 10/26] DAX: virtio-fs: Fill in " Dr. David Alan Gilbert (git)
2021-04-28 11:00   ` [Virtio-fs] " Dr. David Alan Gilbert (git)
2021-05-05 16:43   ` Stefan Hajnoczi
2021-05-05 16:43     ` [Virtio-fs] " Stefan Hajnoczi
2021-04-28 11:00 ` [PATCH v3 11/26] DAX: virtiofsd Add cache accessor functions Dr. David Alan Gilbert (git)
2021-04-28 11:00   ` [Virtio-fs] " Dr. David Alan Gilbert (git)
2021-04-28 11:00 ` [PATCH v3 12/26] DAX: virtiofsd: Add setup/remove mappings fuse commands Dr. David Alan Gilbert (git)
2021-04-28 11:00   ` [Virtio-fs] " Dr. David Alan Gilbert (git)
2021-05-06 15:02   ` Stefan Hajnoczi
2021-05-06 15:02     ` [Virtio-fs] " Stefan Hajnoczi
2021-04-28 11:00 ` [PATCH v3 13/26] DAX: virtiofsd: Add setup/remove mapping handlers to passthrough_ll Dr. David Alan Gilbert (git)
2021-04-28 11:00   ` [Virtio-fs] " Dr. David Alan Gilbert (git)
2021-04-28 11:00 ` [PATCH v3 14/26] DAX: virtiofsd: Wire up passthrough_ll's lo_setupmapping Dr. David Alan Gilbert (git)
2021-04-28 11:00   ` [Virtio-fs] " Dr. David Alan Gilbert (git)
2021-04-28 11:00 ` [PATCH v3 15/26] DAX: virtiofsd: Make lo_removemapping() work Dr. David Alan Gilbert (git)
2021-04-28 11:00   ` [Virtio-fs] " Dr. David Alan Gilbert (git)
2021-04-28 11:00 ` [PATCH v3 16/26] DAX: virtiofsd: route se down to destroy method Dr. David Alan Gilbert (git)
2021-04-28 11:00   ` [Virtio-fs] " Dr. David Alan Gilbert (git)
2021-04-28 11:00 ` [PATCH v3 17/26] DAX: virtiofsd: Perform an unmap on destroy Dr. David Alan Gilbert (git)
2021-04-28 11:00   ` [Virtio-fs] " Dr. David Alan Gilbert (git)
2021-04-28 11:00 ` [PATCH v3 18/26] DAX/unmap: virtiofsd: Add VHOST_USER_SLAVE_FS_IO Dr. David Alan Gilbert (git)
2021-04-28 11:00   ` [Virtio-fs] " Dr. David Alan Gilbert (git)
2021-05-06 15:12   ` Stefan Hajnoczi
2021-05-06 15:12     ` [Virtio-fs] " Stefan Hajnoczi
2021-05-27 17:44     ` Dr. David Alan Gilbert
2021-05-27 17:44       ` [Virtio-fs] " Dr. David Alan Gilbert
2021-05-06 15:16   ` Stefan Hajnoczi
2021-05-06 15:16     ` [Virtio-fs] " Stefan Hajnoczi
2021-05-27 17:31     ` Dr. David Alan Gilbert
2021-05-27 17:31       ` [Virtio-fs] " Dr. David Alan Gilbert
2021-04-28 11:00 ` [PATCH v3 19/26] DAX/unmap virtiofsd: Add wrappers for VHOST_USER_SLAVE_FS_IO Dr. David Alan Gilbert (git)
2021-04-28 11:00   ` [Virtio-fs] " Dr. David Alan Gilbert (git)
2021-04-28 12:53   ` Dr. David Alan Gilbert
2021-04-28 12:53     ` [Virtio-fs] " Dr. David Alan Gilbert
2021-04-28 11:00 ` [PATCH v3 20/26] DAX/unmap virtiofsd: Parse unmappable elements Dr. David Alan Gilbert (git)
2021-04-28 11:00   ` [Virtio-fs] " Dr. David Alan Gilbert (git)
2021-05-06 15:23   ` Stefan Hajnoczi
2021-05-06 15:23     ` [Virtio-fs] " Stefan Hajnoczi
2021-05-27 17:56     ` Dr. David Alan Gilbert
2021-05-27 17:56       ` [Virtio-fs] " Dr. David Alan Gilbert
2021-04-28 11:00 ` [PATCH v3 21/26] DAX/unmap virtiofsd: Route unmappable reads Dr. David Alan Gilbert (git)
2021-04-28 11:00   ` [Virtio-fs] " Dr. David Alan Gilbert (git)
2021-05-06 15:27   ` Stefan Hajnoczi
2021-05-06 15:27     ` [Virtio-fs] " Stefan Hajnoczi
2021-04-28 11:00 ` [PATCH v3 22/26] DAX/unmap virtiofsd: route unmappable write to slave command Dr. David Alan Gilbert (git)
2021-04-28 11:00   ` [Virtio-fs] " Dr. David Alan Gilbert (git)
2021-05-06 15:28   ` Stefan Hajnoczi
2021-05-06 15:28     ` [Virtio-fs] " Stefan Hajnoczi
2021-04-28 11:00 ` [PATCH v3 23/26] DAX:virtiofsd: implement FUSE_INIT map_alignment field Dr. David Alan Gilbert (git)
2021-04-28 11:00   ` [Virtio-fs] " Dr. David Alan Gilbert (git)
2021-04-28 11:00 ` [PATCH v3 24/26] vhost-user-fs: Extend VhostUserFSSlaveMsg to pass additional info Dr. David Alan Gilbert (git)
2021-04-28 11:00   ` [Virtio-fs] " Dr. David Alan Gilbert (git)
2021-05-06 15:31   ` Stefan Hajnoczi
2021-05-06 15:31     ` [Virtio-fs] " Stefan Hajnoczi
2021-05-06 15:32   ` Stefan Hajnoczi
2021-05-06 15:32     ` [Virtio-fs] " Stefan Hajnoczi
2021-04-28 11:00 ` [PATCH v3 25/26] vhost-user-fs: Implement drop CAP_FSETID functionality Dr. David Alan Gilbert (git)
2021-04-28 11:00   ` [Virtio-fs] " Dr. David Alan Gilbert (git)
2021-04-28 11:01 ` [PATCH v3 26/26] virtiofsd: Ask qemu to drop CAP_FSETID if client asked for it Dr. David Alan Gilbert (git)
2021-04-28 11:01   ` [Virtio-fs] " Dr. David Alan Gilbert (git)
2021-05-06 15:37   ` Stefan Hajnoczi
2021-05-06 15:37     ` [Virtio-fs] " Stefan Hajnoczi
2021-05-06 16:02     ` Vivek Goyal
2021-05-06 16:02       ` [Virtio-fs] " Vivek Goyal
2021-05-10  9:05       ` Stefan Hajnoczi
2021-05-10  9:05         ` [Virtio-fs] " Stefan Hajnoczi
2021-05-10 15:23         ` Vivek Goyal
2021-05-10 15:23           ` [Virtio-fs] " Vivek Goyal
2021-05-10 15:32           ` Stefan Hajnoczi
2021-05-10 15:32             ` [Virtio-fs] " Stefan Hajnoczi
2021-05-27 19:09             ` Dr. David Alan Gilbert
2021-05-27 19:09               ` [Virtio-fs] " Dr. David Alan Gilbert
2021-06-10 15:29               ` Dr. David Alan Gilbert
2021-06-10 15:29                 ` [Virtio-fs] " Dr. David Alan Gilbert
2021-06-10 16:23                 ` Stefan Hajnoczi
2021-06-10 16:23                   ` [Virtio-fs] " Stefan Hajnoczi
2021-06-16 12:36                   ` Dr. David Alan Gilbert
2021-06-16 12:36                     ` [Virtio-fs] " Dr. David Alan Gilbert
2021-06-16 15:29                     ` Stefan Hajnoczi
2021-06-16 15:29                       ` [Virtio-fs] " Stefan Hajnoczi
2021-06-16 18:35                       ` Dr. David Alan Gilbert
2021-06-16 18:35                         ` [Virtio-fs] " Dr. David Alan Gilbert
2021-04-28 11:27 ` [PATCH v3 00/26] virtiofs dax patches no-reply
2021-04-28 11:27   ` [Virtio-fs] " no-reply
2021-05-06 15:37 ` Stefan Hajnoczi
2021-05-06 15:37   ` [Virtio-fs] " Stefan Hajnoczi

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=YJFm+hocgArYnWAB@stefanha-x1.localdomain \
    --to=stefanha@redhat.com \
    --cc=dgilbert@redhat.com \
    --cc=groug@kaod.org \
    --cc=qemu-devel@nongnu.org \
    --cc=vgoyal@redhat.com \
    --cc=virtio-fs@redhat.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.