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
