On Tue, Jun 28, 2022 at 09:29:05AM -0700, Stephen Hemminger wrote:

On Tue, 28 Jun 2022 09:22:13 -0700
Tyler Retzlaff [off-list ref] wrote:

quoted

On Tue, Jun 28, 2022 at 04:46:43PM +0200, David Marchand wrote:

quoted

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 <redacted>
---  

... snip ...

quoted

-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.

quoted

+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.