Re: [dpdk-dev] [PATCH] net/virtio-user: check negotiated features before set

["Legacy, Allain" <Allain.Legacy@windriver.com>]

> -----Original Message-----
> From: Tiwei Bie [mailto:tiwei.bie@intel.com]
> Sent: Tuesday, August 14, 2018 11:40 PM
> To: Legacy, Allain
> Cc: maxime.coquelin@redhat.com; zhihong.wang@intel.com;
> dev@dpdk.org; Peters, Matt; Zhang, Qing Long (Eric)
> Subject: Re: [PATCH] net/virtio-user: check negotiated features before set
> 
> On Thu, Aug 09, 2018 at 01:34:55PM -0500, Allain Legacy wrote:
> > From: eric zhang <eric.zhang@windriver.com>
> >
> > This patch checks negotiated features to see if necessary to offload
> > before set the tap device offload capabilities. It also checks if
> > kernel support the TUNSETOFFLOAD operation.
> >
> > Signed-off-by: eric zhang <eric.zhang@windriver.com>
> > Signed-off-by: Allain Legacy <allain.legacy@windriver.com>
> > ---
> >  drivers/net/virtio/virtio_user/vhost_kernel.c     |  2 +-
> >  drivers/net/virtio/virtio_user/vhost_kernel_tap.c | 56
> > +++++++++++++++++------
> > drivers/net/virtio/virtio_user/vhost_kernel_tap.h |  2 +-
> >  3 files changed, 44 insertions(+), 16 deletions(-)
> >
<...>
> 
> I'm not quite sure whether it's a good idea to return failure when failed to set
> offloads to tap. And QEMU also doesn't do this:
> 
> https://github.com/qemu/qemu/blob/6ad90805383e/net/tap-linux.c#L261

Regardless of what has already been done in qemu my preference is that if a requested feature cannot be set then a failure should be propagated up to the application.  Otherwise, the application is left to debug a silent failure which, depending on what that is, may be difficult.   But, as the maintainer, that is your call so if you want it changed to a silent failure I will do that.   Just so that I am clear you want both failures to be ignored?  That is, if a requested offload is not supported we should silently ignore the -ENOTSUP, and also if the ioctl fails for some other reason we should ignore that too?


> 
> We should also check whether offloads available when handling
> VHOST_GET_FEATURES.

That will have to be a different patch as it is outside of the scope of what was being fixed by this one.   For this issue we were fixing the fact that offload features were being enabled even when they were not requested by the application.


Allain