> From: Jason Gunthorpe <jgg@xxxxxxxx> > Sent: Tuesday, August 30, 2022 11:10 PM > > On Tue, Aug 30, 2022 at 09:42:42AM -0400, Anthony Krowiak wrote: > > > > +/* > > > + * Alloc and initialize vfio_device so it can be registered to vfio > > > + * core. > > > + * > > > + * Drivers should use the wrapper vfio_alloc_device() for allocation. > > > + * @size is the size of the structure to be allocated, including any > > > + * private data used by the driver. > > > > > > It seems the purpose of the wrapper is to ensure that the object being > > allocated has as its first field a struct vfio_device object and to return > > its container. Why not just make that a requirement for this function - > > which I would rename vfio_alloc_device - and document it in the prologue? > > The caller can then cast the return pointer or use container_of. > > There are three fairly common patterns for this kind of thing > > 1) The caller open codes everything: > > driver_struct = kzalloc() > core_init(&driver_struct->core) > > 2) Some 'get priv' / 'get data' is used instead of container_of(): > > core_struct = core_alloc(sizeof(*driver_struct)) > driver_struct = core_get_priv(core_struct) > > 3) The allocations and initialization are consolidated in the core, > but we continue to use container_of() > > driver_struct = core_alloc(typeof(*driver_struct)) > > #1 has a general drawback that people routinely mess up the lifecycle > model and get really confused about when to do kfree() vs put(), > creating bugs. > > #2 has a general drawback of not using container_of() at all, and being > a bit confusing in some cases > > #3 has the general drawback of being a bit magical, but solves 1 and > 2's problems. > > I would not fix the struct layout without the BUILD_BUG_ON because > someone will accidently change the order and that becomes a subtle > runtime error - so at a minimum the wrapper macro has to exist to > check that. Agree. And gvt happened to hit this BUILD_BUG_ON when this series was being worked on. > > If you want to allow a dynamic struct layout and avoid the pitfall of > exposing the user to kalloc/kfree, then you still need the macro, and > it does some more complicated offset stuff. > > Having the wrapper macro be entirely type safe is appealing and > reduces code in the drivers, IMHO. Tell it what type you are initing > and get back init'd memory for that type that you always, always free > with a put operation. > > Jason
