Re: [PATCH 2/8] vfio: Rename __vfio_group_unset_container()

From: Alex Williamson <alex.williamson@redhat.com>
To: "Tian, Kevin" <kevin.tian@intel.com>
Cc: Jason Gunthorpe <jgg@nvidia.com>,
	Cornelia Huck <cohuck@redhat.com>,
	"kvm@vger.kernel.org" <kvm@vger.kernel.org>
Subject: Re: [PATCH 2/8] vfio: Rename __vfio_group_unset_container()
Date: Fri, 2 Sep 2022 08:39:34 -0600	[thread overview]
Message-ID: <20220902083934.7e6f6438.alex.williamson@redhat.com> (raw)
In-Reply-To: <BN9PR11MB5276122C4CBAACB295DD15E78C7A9@BN9PR11MB5276.namprd11.prod.outlook.com>

On Fri, 2 Sep 2022 03:51:01 +0000
"Tian, Kevin" <kevin.tian@intel.com> wrote:

> > From: Jason Gunthorpe <jgg@nvidia.com>
> > Sent: Friday, September 2, 2022 8:29 AM
> > 
> > On Wed, Aug 31, 2022 at 08:46:30AM +0000, Tian, Kevin wrote:  
> > > > From: Jason Gunthorpe <jgg@nvidia.com>
> > > > Sent: Wednesday, August 31, 2022 9:02 AM
> > > >
> > > > To vfio_container_detatch_group(). This function is really a container
> > > > function.
> > > >
> > > > Fold the WARN_ON() into it as a precondition assertion.
> > > >
> > > > A following patch will move the vfio_container functions to their own .c
> > > > file.
> > > >
> > > > Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
> > > > ---
> > > >  drivers/vfio/vfio_main.c | 11 +++++------
> > > >  1 file changed, 5 insertions(+), 6 deletions(-)
> > > >
> > > > diff --git a/drivers/vfio/vfio_main.c b/drivers/vfio/vfio_main.c
> > > > index bfa6119ba47337..e145c87f208f3a 100644
> > > > --- a/drivers/vfio/vfio_main.c
> > > > +++ b/drivers/vfio/vfio_main.c
> > > > @@ -928,12 +928,13 @@ static const struct file_operations vfio_fops = {
> > > >  /*
> > > >   * VFIO Group fd, /dev/vfio/$GROUP
> > > >   */
> > > > -static void __vfio_group_unset_container(struct vfio_group *group)
> > > > +static void vfio_container_detatch_group(struct vfio_group *group)  
> > >
> > > s/detatch/detach/  
> > 
> > Oops
> >   
> > > Given it's a vfio_container function is it better to have a container pointer
> > > as the first parameter, i.e.:
> > >
> > > static void vfio_container_detatch_group(struct vfio_container *container,
> > > 		struct vfio_group *group)  
> > 
> > Ah, I'm not so sure, it seems weird to make the caller do
> > group->container then pass the group in as well.
> > 
> > This call assumes the container is the container of the group, it
> > doesn't work in situations where that isn't true.  
> 
> Yes, this is a valid interpretation on doing this way.
> 
> Other places e.g. iommu_detach_group(domain, group) go the other way
> even if internally domain is not used at all. I kind of leave that pattern
> in mind thus raised this comment. But not a strong opinion.
> 
> > 
> > It is kind of weird layering in a way, arguably we should have the
> > current group stored in the container if we want things to work that
> > way, but that is a big change and not that wortwhile I think.
> > 
> > Patch 7 is pretty much the same, it doesn't work right unless the
> > container and device are already matched
> >   
> 
> If Alex won't have a different preference and with the typo fixed,

I don't get it, if a group is detaching itself from a container, why
isn't it vfio_group_detach_container().  Given our naming scheme,
vfio_container_detach_group() absolutely implies the args Kevin
suggests, even if they're redundant.  vfio_foo_* functions should
operate on a a vfio_foo object.  Thanks,

Alex