On Sat, Jul 9, 2022 at 6:28 PM Stephen Hemminger <step...@networkplumber.org> 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. > > If you get chance to deconstruct API, switching to a numeric index > is safest similar to what Tyler suggested. Think of ethdev port number > and Posix file descriptor model. The advantage of an index is that > it can be validated more easily by the code that is called.
This is still a heavy change in the APIs. The deprecation notice did not mention such change, and I don't think many people are aware of this suggestion. This would need more discussion but I don't want to block the series on concluding on this question. So I am for going ahead with the series in its current form. -- David Marchand
