On Fri, May 30, 2014 at 12:01:23AM +1000, Darren Reed wrote: > On 29/05/2014 12:29 PM, Mindaugas Rasiukevicius wrote: > >Darren Reed <darr...@netbsd.org> wrote: > >>On 29/05/2014 5:06 AM, Mindaugas Rasiukevicius wrote: > >>>Darren Reed <darr...@netbsd.org> wrote: > >>>>>No, there is no need to expose the structure. Even if there would be > >>>>>another internal component using the structure(s) one should consider > >>>>>accessors/mutators. Even if that component would have a good reason > >>>>>not to use accessors/mutators, the structure should be placed under > >>>>>#ifdef __SUBSYSTEM_PRIVATE (and certainly #ifdef _KERNEL). However, > >>>>>I strongly discourage to start from the last step without having a > >>>>>necessity first. > >>>>> > >>>>>One of the main reasons why we ended up with our network stack being > >>>>>such a mess is exactly this: the internal structures are exposed and > >>>>>accessed all over the place, there is a lack of strict interfacing, > >>>>>and the information hiding principle is not followed. > >>>>So if someone were to write a program to grovel through a crash dump > >>>>or /dev/mem and print out these structures, how would they get the > >>>>definition of it? > >>>This is getting off-topic, but for the sake of wondering readers: > >>> > >>>Serialize and export the structure, or create a wrapper structure used > >>>for data transportation, or implement interface with accessors/mutators. > >>>If you think that you should be able to fiddle with any structure in the > >>>kernel (as it would be 1980s) then you are plain wrong (do I really need > >>>to explain this?). > >>Your justification for including the structure in the .c file is that > >>developers can't betrusted to not use header files that clearly aren't > >>public interfaces. That's what code review is for and puts the focus of > >>what you're arguing as being a "human problem" and not a "technology > >>problem." You can't solve "human problems" with technology. Putting the > >>structure there won't stop determined people, it will just make it harder > >>and they'll begrudge NetBSD for making their life harder. > >Yes, and that is absolutely valid justification. Code review ought to > >prevent from such problems, but that does not always happen (for a variety > >of reasons - from developers lacking time, or lack of developers who are > >familiar with that particular code base, to developers trying to reach a > >compromise; you know the realities). Technology is also a one of the means > >to address the "human problem", at least partially. > > > >Do not get me wrong - I am not advocating black and white world of view. > >You cannot solve these problems with tough restrictions. Otherwise, one > >could advocate for adopting something like MISRA C standard; which I think > >is horrible - it takes away flexibility from an engineer, it forces you to > >produce ugly and tasteless code. However, it sort of does its job for the > >purpose it was created for, that is, to be idiot proof. I just think that > >the BSD community can do better than that. As an old open source we can > >produce code which is above the average quality. At least I hope so. > > > >Despite that, the point i.e. the justification still stands. We just try > >to apply some common sense when judging which restrictions are worth in our > >case and which are not. If code review would be so efficient solution, we > >would not have so much spaghetti code, as we do today. Right? :) Even if > >on average it is much better than a statistical commercial code. > > > >>Putting structures inside a .c file represents a very short term view > >>of how the implementation willevolve and be used. > >> > >>The method that I've seen used in Solaris (for example) is to use > >>foo_impl.h to providethe details of data structure that are essentially > >>private and those .h filesmay or may notbe shipped as part of the end > >>user system.Using pktqueue_private.h might be suitable. > >> > >><...> > >It is basically the third phase I described in my original response (and > >I do use _impl.h headers), except if you read it carefully - you should > >better have a necessity and provide a good reason for it. Because if you > >are fiddling with an internal structure - it is a good indicator that you > >might potentially be doing something wrong; even more so if you fiddle with > >a lockless data structure as in pktqueue's case. "It might be useful some > >day" is a very poor and dangerous reasoning in this case. > > > > All of your arguments boil down to "can't trust someone else." > > Why do you need to be so insulting of other developers in your arguments? > > Do you think you're the only person capable of making good design decisions? > > Sorry, but I won't be a party to that kind of attitude and want nothing more > to do with this.
I think that Mindaugas is being pragmatic here. Developers are not equally brilliant[*], observant of the rules, or perceptive of the patterns, layers, or abstractions in the code. He is writing the code in a way that discourages us from casually misusing or breaking it by getting under the abstraction. I don't find that offensive. Dave [*] However, we are all above average at NetBSD. -- David Young dyo...@pobox.com Urbana, IL (217) 721-9981