Re: [RFC PATCH 1/7] vfio: Include optional device match in vfio_device_ops callbacks

[Cornelia Huck <cohuck@redhat.com>]

On Thu, 6 Feb 2020 11:18:42 -0700
Alex Williamson <alex.williamson@redhat.com> wrote:

> On Thu, 6 Feb 2020 12:14:19 +0100
> Cornelia Huck <cohuck@redhat.com> wrote:
> 
> > On Tue, 04 Feb 2020 16:05:43 -0700
> > Alex Williamson <alex.williamson@redhat.com> wrote:
> >   
> > > Allow bus drivers to provide their own callback to match a device to
> > > the user provided string.
> > > 
> > > Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
> > > ---
> > >  drivers/vfio/vfio.c  |   19 +++++++++++++++----
> > >  include/linux/vfio.h |    3 +++

> I think with your first option we arrive at something like this:
> 
> diff --git a/drivers/vfio/vfio.c b/drivers/vfio/vfio.c
> index dda1726adda8..b5609a411139 100644
> --- a/drivers/vfio/vfio.c
> +++ b/drivers/vfio/vfio.c
> @@ -883,14 +883,15 @@ static struct vfio_device *vfio_device_get_from_name(struct vfio_group *group,
>  
>  		if (it->ops->match) {
>  			ret = it->ops->match(it->device_data, buf);
> -			if (ret < 0 && ret != -ENODEV) {
> +			if (ret < 0) {
>  				device = ERR_PTR(ret);
>  				break;
>  			}
> -		} else
> -			ret = strcmp(dev_name(it->dev), buf);
> +		} else {
> +			ret = !strcmp(dev_name(it->dev), buf);
> +		}
>  
> -		if (!ret) {
> +		if (ret) {
>  			device = it;
>  			vfio_device_get(device);
>  			break;
> diff --git a/include/linux/vfio.h b/include/linux/vfio.h
> index 755e0f0e2900..029694b977f2 100644
> --- a/include/linux/vfio.h
> +++ b/include/linux/vfio.h
> @@ -26,8 +26,9 @@
>   *         operations documented below
>   * @mmap: Perform mmap(2) on a region of the device file descriptor
>   * @request: Request for the bus driver to release the device
> - * @match: Optional device name match callback (return: 0 for match, -ENODEV
> - *         (or >0) for no match and continue, other -errno: no match and stop)
> + * @match: Optional device name match callback (return: 0 for no-match, >0 for
> + *         match, -errno for abort (ex. match with insufficient or incorrect
> + *         additional args)
>   */
>  struct vfio_device_ops {
>  	char	*name;
> 
> 
> I like that.  Thanks,
> 
> Alex

Looks good to me.