Re: Lockless IP input queue, the pktqueue interface

[Mindaugas Rasiukevicius <rmind%netbsd.org@localhost>]

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.

You have to provide a convincing argument to expose a structure, not the
other way round.  Exposing is trivial and we will always be able to do
that, but making it opaque again might be a tremendous job (somebody sent
an email today to freebsd-arch on the roadmap for struct ifnet - what an
incidental illustration!).


P.S. We have many interfaces which keep structures in .c and it is
off-topic to this thread; if you want continue with this philosophical
discussion, please create a new thread.

-- 
Mindaugas