[asterisk-dev] [asterisk-commits] rizzo: trunk r89379 - /trunk/include/asterisk/network.h

[Luigi Rizzo]

On Sun, Nov 18, 2007 at 01:57:29PM -0500, Simon Perreault wrote:
> On Saturday 17 November 2007 09:34:38 Luigi Rizzo wrote:
> > in my view network.h is just to hide system differences
> > (and provide trivial functions such as the one above),
> > while netwock provides additional services
> > for sockets (container, tls, etc.)
> 
> Yes, so why did you add inaddrcmp() to network.h? It doesn't exist on any 
> platform, it's specific to Asterisk. So it goes into netsock.h, not 
> network.h.
> 
> > you keep saying that, and i don't understand why.
> 
> Sorry, I kept repeating because I thought my voice did not carry to you.
> 
> > The reason for aggregating is
> > avoid repreating the #ifdef THIS #ifdef THAT all over the place.
> 
> I feel that's not a good enough reason. The better solution is to put the 
> #ifdef magic in
> 
> asterisk/compat/socket.h
> asterisk/compat/stdint.h
> asterisk/compat/...
> 
> , prepend asterisk/compat to the include path, and in the .c's use
> 
> #include <socket.h>
> #include <stdint.h>

that's obfuscation, not good engineering practice, because you make
the programmer believe that you use the standard stuff and instead
you don't.

The second reason for aggregating headers is lowering the bar
for programmers to write code. If we rely on people to know exactly
which subset of 25 system headers (and in which order) you need on
the reference linux version for which we created the asterisk/compat/*
files, we surely end up with lots of inconsistencies and unnecessary
includes - just browse at the code as it is now and you'll see.

If you clearly explain some simple instructions such as:

 - first and foremost, when you include a header don't cut&paste and
   write a short comment on why you need it (unless it's obvious
   e.g. compat.h, or you are building a module ...)
 - all source files need "asterisk/compat.h" which covers the basic
   C stuff and platform and compiler issues;
 - if you do something related to system I/O, include "asterisk/fileio.h"
   (this should cover open/fopen/fcntl/ioctl ...)
 - if you do something network related include "asterisk/network.h"
 - if you build a module include "asterisk/module.h"
 - if you build a channel driver include 'asterisk/channel.h"
 - if you need the functions supplied by module foobar,
   include "asterisk/foobar.h"
 ... (there isn't a lot more, really)

and the list is reasonably short, then it is easier to write the
code and for other people to check.

> other people will add headers that are not supposed to be there. You know 
> that the purpose of network.h is for system compat stuff, but other people in 

I said

    in my view network.h is just to hide system differences
    (and provide trivial functions such as the one above),

and that's what it is and what i want to document (we don't have a
document yet on asterisk APIs and code structure, but I am
certainly planning to write one).
There is a bit of compromise here, this inaddrcmp could certainly
go in asterisk/more_network.h but at the price of requiring two includes instead
of one in most places, or it coudl go in asterisk/compat.h or asterisk/utils.h
but then the latter would become the everything.h that nobody wants.

> the future will now know this and quickly will begin adding macros, short 
> inlines (you even did it with inaddrcmp()), and system headers vaguely 
> related to network stuff. network.h will grow, and eventually two problem 
> will appear:
> 
> 1. It would be hard to tell which headers a .c that would include that big 
> network.h really needs. As Russell wisely said, "there is some value in 
> knowing which special headers are needed in a file".
> 
> 2. Compile times. 'nuff said.

i am not sure what you mean with this statement.
i gave you numbers that they are not affected by my approach, and
i also showed numbers suggesting that using ccache cuts the build
time by factor of 10.  So far the evidence shows that compile times
are a non-problem.  Once again if you have numbers disproving that
i am all ears.

> You don't *need* to aggregate. So don't.

i actually have a different opinion - i do think we need to aggregate a bit
to make life easier to developers and improve quality control.

> Anyway, don't listen to me, I'm just coaching from the stands. ;)

i am afraid, that's the feeling i am having...

cheers
luigi