On Tue, Jun 28, 2022 at 09:29:05AM -0700, Stephen Hemminger wrote:
> On Tue, 28 Jun 2022 09:22:13 -0700
> Tyler Retzlaff <roret...@linux.microsoft.com> wrote:
>
> > On Tue, Jun 28, 2022 at 04:46:43PM +0200, David Marchand wrote:
> > > Make rte_bus opaque for non internal users.
> > > This will make extending this object possible without breaking the ABI.
> > >
> > > Introduce a new driver header and move rte_bus definition and helpers.
> > >
> > > Signed-off-by: David Marchand <david.march...@redhat.com>
> > > ---
> >
> > ... snip ...
> >
> > > -struct rte_bus {
> > > - RTE_TAILQ_ENTRY(rte_bus) next; /**< Next bus object in linked list */
> > > - const char *name; /**< Name of the bus */
> > > - rte_bus_scan_t scan; /**< Scan for devices attached to bus */
> > > - rte_bus_probe_t probe; /**< Probe devices on bus */
> > > - rte_bus_find_device_t find_device; /**< Find a device on the bus */
> > > - rte_bus_plug_t plug; /**< Probe single device for drivers */
> > > - rte_bus_unplug_t unplug; /**< Remove single device from driver */
> > > - rte_bus_parse_t parse; /**< Parse a device name */
> > > - rte_bus_devargs_parse_t devargs_parse; /**< Parse bus devargs */
> > > - rte_dev_dma_map_t dma_map; /**< DMA map for device in the bus */
> > > - rte_dev_dma_unmap_t dma_unmap; /**< DMA unmap for device in the bus */
> > > - struct rte_bus_conf conf; /**< Bus configuration */
> > > - rte_bus_get_iommu_class_t get_iommu_class; /**< Get iommu class */
> > > - rte_dev_iterate_t dev_iterate; /**< Device iterator. */
> > > - rte_bus_hot_unplug_handler_t hot_unplug_handler;
> > > - /**< handle hot-unplug failure on the bus */
> > > - rte_bus_sigbus_handler_t sigbus_handler;
> > > - /**< handle sigbus error on the bus */
> > > -
> > > -};
> >
> > since we're overhauling anyway we could follow suit with a number of the
> > lessons from posix apis e.g. pthread and eliminate typed pointers for a
> > little extra value.
> >
> > > +struct rte_bus;
> > > +struct rte_device;
> >
> > to avoid people tripping over mishandling pointers in/out of the api
> > surface taking the opaque object you could declare opaque handle for the
> > api to operate on instead. it would force the use of a cast in the
> > implementation but would avoid accidental void * of the wrong thing that
> > got cached being passed in. if the cast was really undesirable just
> > whack it under an inline / internal function.
>
> I don't like that because it least to dangerous casts in the internal code.
> Better to keep the the type of the object. As long as the API only passes
> around an pointer to a struct, without exposing the contents of the struct;
> it is safer and easier to debug.
as i mentioned you can use an inline/internal function or even a macro
to hide the cast, you could provide some additional integrity checks
here if desired as a value add.
the fact that you expose that it is a struct is an internal
implementation detail, if truly opaque tomorrow you could convert it
to a simple integer that indexes or enumerates something and prevents
any meaningful interpretation in the application.
when you say it is safer to debug i think you mean for dpdk devs not the
application developer because unless the app developer does something
really gross/dangerous casting they really can't "mishandle" the opaque
object except to use one that isn't initialized at all which we
can detect and handle internally in a general way.
i will however concede there would be slightly more finger work when
debugging dpdk itself since gdb / debugger doesn't automatically infer
type so you end up having to tell gdb what is in the uintptr_t.
anyway just drawing from experience in the driver frameworks we maintain
in windows, i think one of our regrets is that we didn't do this from
day 1 and subsequentl that we initially only used one opaque type
instead of defining separate (not implicitly convertable) types to each
opaque type.
