Darren Reed <darrenr%netbsd.org@localhost> wrote:
On 29/05/2014 5:06 AM, Mindaugas Rasiukevicius wrote:
Darren Reed <darrenr%netbsd.org@localhost> 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.