On Tue, Jun 28, 2022 at 8:23 PM Tyler Retzlaff <roret...@linux.microsoft.com> wrote: > > On Tue, Jun 28, 2022 at 10:38:27AM -0700, Stephen Hemminger wrote: > > On Tue, 28 Jun 2022 10:07:12 -0700 > > Tyler Retzlaff <roret...@linux.microsoft.com> wrote: > > > > > > > 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. > > > > It seems to be a difference in style/taste. > > it's not i've sited at least one example of a mistake that becomes a > compile time failure where application code is incorrectly authored > where use of a pointer offers no such protection. > > > The Linux/Unix side prefers opaque structure pointers. > > Windows (and LLVM) uses numeric handles. > > > > At this point DPDK should follow the Linux bus. > > dpdk is multi-platform and unix does not necessarily standardize on > pointer to struct for opaque objects. freebsd has many apis notably > bus_space that does uses handles and as previously mentioned posix > threads uses handles. > > i understand that linux is an important platform but it isn't the only > platform dpdk targets and just because it is important doesn't mean it > should always enjoy being the defacto standard. > > anyway, i'll leave it for the patch author to decide. i still like the > patch series either way. i just think this would make applications more > robust.
Thanks for this feedback Tyler. I would lean towards Stephen opinion atm, but I am not decided yet. For now, I'll post a v2, extending the series to other internal objects. We can conclude on this topic during 22.11. -- David Marchand
