Re: [Qemu-devel] libvhost-user: Fix the VHOST_USER_PROTOCOL_F_SLAVE_SEND_FD check

[Dr. David Alan Gilbert]

* Boeuf, Sebastien (address@hidden) wrote:
> From 734625fe0c031d26e612800cd9331235f58ae2e0 Mon Sep 17 00:00:00 2001
> From: Sebastien Boeuf <address@hidden>
> Date: Wed, 7 Aug 2019 07:15:32 -0700
> Subject: [PATCH] libvhost-user: Fix the VHOST_USER_PROTOCOL_F_SLAVE_SEND_FD
>  check
> 
> Vhost user protocol features are set as a bitmask. And the following
> constant VHOST_USER_PROTOCOL_F_SLAVE_SEND_FD value is 10 because the bit
> 10 indicates if the features is set or not.
> 
> The proper way to check for the presence or absence of this feature is
> to shift 1 by the value of this constant and then mask it with the
> actual bitmask representing the supported protocol features.
> 
> This patch aims to fix the current code as it was not doing the
> shifting, but instead it was masking directly with the value of the
> constant itself.
> 
> Signed-off-by: Sebastien Boeuf <address@hidden>


Reviewed-by: Dr. David Alan Gilbert <address@hidden>

Michael: Can you pick this up?

> ---
>  contrib/libvhost-user/libvhost-user.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/contrib/libvhost-user/libvhost-user.c 
> b/contrib/libvhost-user/libvhost-user.c
> index fb61142bcc..d75a9c86ed 100644
> --- a/contrib/libvhost-user/libvhost-user.c
> +++ b/contrib/libvhost-user/libvhost-user.c
> @@ -1112,7 +1112,8 @@ bool vu_set_queue_host_notifier(VuDev *dev, VuVirtq 
> *vq, int fd,
>  
>      vmsg.fd_num = fd_num;
>  
> -    if ((dev->protocol_features & VHOST_USER_PROTOCOL_F_SLAVE_SEND_FD) == 0) 
> {
> +    if ((dev->protocol_features &
> +        (1ULL << VHOST_USER_PROTOCOL_F_SLAVE_SEND_FD)) == 0) {
>          return false;
>      }
>  
> @@ -2537,7 +2538,8 @@ int64_t vu_fs_cache_request(VuDev *dev, 
> VhostUserSlaveRequest req, int fd,
>  
>      vmsg.fd_num = fd_num;
>  
> -    if ((dev->protocol_features & VHOST_USER_PROTOCOL_F_SLAVE_SEND_FD) == 0) 
> {
> +    if ((dev->protocol_features &
> +        (1ULL << VHOST_USER_PROTOCOL_F_SLAVE_SEND_FD)) == 0) {
>          return -EINVAL;
>      }
>  
> -- 
> 2.17.1
> 
> ________________________________________
> From: Boeuf, Sebastien
> Sent: Wednesday, August 07, 2019 9:35 AM
> To: address@hidden
> Cc: address@hidden; address@hidden
> Subject: RE: libvhost-user: Fix the VHOST_USER_PROTOCOL_F_SLAVE_SEND_FD check
> 
> From 950c62dd450c8f6c3fc04269bbefa3a368bb39b6 Mon Sep 17 00:00:00 2001
> From: Sebastien Boeuf <address@hidden>
> Date: Wed, 7 Aug 2019 07:15:32 -0700
> Subject: [PATCH] libvhost-user: Fix the VHOST_USER_PROTOCOL_F_SLAVE_SEND_FD
>  check
> 
> Vhost user protocol features are set as a bitmask. And the following
> constant VHOST_USER_PROTOCOL_F_SLAVE_SEND_FD value is 10 because the bit
> 10 indicates if the features is set or not.
> 
> The proper way to check for the presence or absence of this feature is
> to shift 1 by the value of this constant and then mask it with the
> actual bitmask representing the supported protocol features.
> 
> This patch aims to fix the current code as it was not doing the
> shifting, but instead it was masking directly with the value of the
> constant itself.
> 
> Signed-off-by: Sebastien Boeuf <address@hidden>
> ---
>  contrib/libvhost-user/libvhost-user.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/contrib/libvhost-user/libvhost-user.c 
> b/contrib/libvhost-user/libvhost-user.c
> index fb61142bcc..8ff387deff 100644
> --- a/contrib/libvhost-user/libvhost-user.c
> +++ b/contrib/libvhost-user/libvhost-user.c
> @@ -71,7 +71,7 @@
> 
>  /* The version of the protocol we support */
>  #define VHOST_USER_VERSION 1
> -#define LIBVHOST_USER_DEBUG 0
> +#define LIBVHOST_USER_DEBUG 1
> 
>  #define DPRINT(...)                             \
>      do {                                        \
> @@ -1112,7 +1112,8 @@ bool vu_set_queue_host_notifier(VuDev *dev, VuVirtq 
> *vq, int fd,
> 
>      vmsg.fd_num = fd_num;
> 
> -    if ((dev->protocol_features & VHOST_USER_PROTOCOL_F_SLAVE_SEND_FD) == 0) 
> {
> +    if ((dev->protocol_features &
> +        (1ULL << VHOST_USER_PROTOCOL_F_SLAVE_SEND_FD)) == 0) {
>          return false;
>      }
> 
> @@ -2537,7 +2538,8 @@ int64_t vu_fs_cache_request(VuDev *dev, 
> VhostUserSlaveRequest req, int fd,
> 
>      vmsg.fd_num = fd_num;
> 
> -    if ((dev->protocol_features & VHOST_USER_PROTOCOL_F_SLAVE_SEND_FD) == 0) 
> {
> +    if ((dev->protocol_features &
> +        (1ULL << VHOST_USER_PROTOCOL_F_SLAVE_SEND_FD)) == 0) {
>          return -EINVAL;
>      }
> 
> --
> 2.17.1
> 
> ________________________________________
> From: Boeuf, Sebastien
> Sent: Wednesday, August 07, 2019 9:24 AM
> To: address@hidden
> Cc: address@hidden; address@hidden
> Subject: Re: libvhost-user: Fix the VHOST_USER_PROTOCOL_F_SLAVE_SEND_FD check
> 
> On Wed, 2019-08-07 at 17:09 +0100, Dr. David Alan Gilbert wrote:
> > * Boeuf, Sebastien (address@hidden) wrote:
> > > From 0a53a81db6dd069f9b7bcdcd386845bceb3a2ac6 Mon Sep 17 00:00:00
> > > 2001
> > > From: Sebastien Boeuf <address@hidden>
> > > Date: Wed, 7 Aug 2019 07:15:32 -0700
> > > Subject: [PATCH] libvhost-user: Fix the
> > > VHOST_USER_PROTOCOL_F_SLAVE_SEND_FD
> > >  check
> > >
> > > Vhost user protocol features are set as a bitmask. And the
> > > following
> > > constant VHOST_USER_PROTOCOL_F_SLAVE_SEND_FD value is 10 because
> > > the
> > > bit
> > > 10 indicates if the features is set or not.
> > >
> > > The proper way to check for the presence or absence of this feature
> > > is
> > > to shift 1 by the value of this constant and then mask it with the
> > > actual bitmask representing the supported protocol features.
> > >
> > > This patch aims to fix the current code as it was not doing the
> > > shifting, but instead it was masking directly with the value of the
> > > constant itself.
> > >
> > > Signed-off-by: Sebastien Boeuf <address@hidden>
> >
> > Nicely spotted.
> >
> > Two things;
> >   a) I think your mail client has wrapped the lines at some point.
> >   b) I think this is why the has_feature() functione exists, so does
> >      that become
> >
> >       if (!has_feature(dev->protocol_features,
> > VHOST_USER_PROTOCOL_F_SLAVE_SEND_FD))
> 
> Ah yes but that's because I forgot to check the patch format first :(
> 
> I'm going to update the patch.
> 
> Thanks,
> Sebastien
> >
> > Dave
> >
> > > ---
> > >  contrib/libvhost-user/libvhost-user.c | 4 ++--
> > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/contrib/libvhost-user/libvhost-user.c
> > > b/contrib/libvhost-
> > > user/libvhost-user.c
> > > index fb61142bcc..11909fb7c1 100644
> > > --- a/contrib/libvhost-user/libvhost-user.c
> > > +++ b/contrib/libvhost-user/libvhost-user.c
> > > @@ -1112,7 +1112,7 @@ bool vu_set_queue_host_notifier(VuDev *dev,
> > > VuVirtq *vq, int fd,
> > >
> > >      vmsg.fd_num = fd_num;
> > >
> > > -    if ((dev->protocol_features &
> > > VHOST_USER_PROTOCOL_F_SLAVE_SEND_FD)
> > > == 0) {
> > > +    if ((dev->protocol_features & (1ULL <<
> > > VHOST_USER_PROTOCOL_F_SLAVE_SEND_FD)) == 0) {
> > >          return false;
> > >      }
> > >
> > > @@ -2537,7 +2537,7 @@ int64_t vu_fs_cache_request(VuDev *dev,
> > > VhostUserSlaveRequest req, int fd,
> > >
> > >      vmsg.fd_num = fd_num;
> > >
> > > -    if ((dev->protocol_features &
> > > VHOST_USER_PROTOCOL_F_SLAVE_SEND_FD)
> > > == 0) {
> > > +    if ((dev->protocol_features & (1ULL <<
> > > VHOST_USER_PROTOCOL_F_SLAVE_SEND_FD)) == 0) {
> > >          return -EINVAL;
> > >      }
> > >
> > > --
> > > 2.17.1
> >
> > --
> > Dr. David Alan Gilbert / address@hidden / Manchester, UK
--
Dr. David Alan Gilbert / address@hidden / Manchester, UK