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

[Jason Gunthorpe <jgg@nvidia.com>]

On Tue, Sep 06, 2022 at 01:38:11PM -0600, Alex Williamson wrote:

> I might refine these to:
> 
> struct vfio_container *vfio_container_from_file(struct file *filep);
> 
> int vfio_group_use_container(struct vfio_group *group);
> void vfio_group_unuse_container(struct vfio_group *group);
> 
> int vfio_container_attach_group(struct vfio_container *container,
> 				struct vfio_group *group);
> void vfio_group_detach_container(struct vfio_group *group);

IMHO it is weird to sacrifice the clear pair'd naming, keep the group
first?

> void vfio_device_container_register(struct vfio_device *device);
> void vfio_device_container_unregister(struct vfio_device *device);
> 
> long vfio_container_ioctl_check_extension(struct vfio_container *container,
>  					  unsigned long arg);

I'm fine with all of these, if you like it I'll change it - it is a
lot of work to change the names and rebase everything so please lets
all agree first.

> int vfio_device_pin_container_pages(struct vfio_device *device, dma_addr_t iova,
> 				    int npage, int prot, struct page **pages);
> void vfio_device_unpin_container_pages(struct vfio_device *device, dma_addr_t
> 				       iova, int npage);
> 
> int vfio_device_dma_rw_container(struct vfio_device *device, dma_addr_t iova,
> 				 void *data, size_t len, bool write);

These are exported symbols and I don't want to mess with them. The
fact they are in container.c is temporary artifact, as they do touch
the struct vfio_container. The iommufd series puts a wrapper in main.c
and the above can have signatures that only take in a container * -
which is what you suggest below. So lets leave them alone here.

> Overall, I'm still not really on board with sacrificing a "the name
> tells you how to use it" API in order to break down devices, groups,
> and containers into their own subsystems.  We should not only consider
> the logical location of the function, but the usability and
> intuitiveness of the API.

Sure, I'm not set on any particular naming scheme.

> Are we necessarily finding the right splits here?  The {un}use,
> {un}pin, and dma_rw in particular seem like they could be decomposed
> further.  

I can't think how to do anything better for {un}use. They are in
container.c because the entire container_users mechanism is gone when
the container code is gone, even though the mechanism is part of the
struct vfio_group. unuse doesn't even touch the vfio_container..

Lets just leave them with their group names and they slightly oddly
stay in container.c

> vfio_container_{un}use() with a vfio_container object.  Or a
> vfio_device intermediary that can call vfio_container_ functions for
> {un}pin/rw, also with a container object.  Thanks,

Some of the split is an artifact of how the code is right now. I don't
think you'd end up with exactly this interface if you designed it from
scratch, but we have what we have, and I don't have a lot of
enthusiasm to restructure significantly for the sake of naming..

Thanks,
Jason