Network stack cleanup

[tbursztyka]

Current code base would need serious cleanup/optimization. Current status shows duplicates, un-proper modularization, under optimized data storage/lookup, naming issues etc...

What should be done:

• Keep each specific part into their own C file (i.e.: ipv4, ipv6, etc...).
  . For instance, icmpv4 redo in its code what ipv4 does already.
• Factorize as much as possible
• Reduce #ifdefs usage inside functions
  . Use IS_ENABLED()
  . Move CONFIG_* conditioned code to own function
• Naming convention
  .When it is a public function net_<domain><...>()
  .When it is a private function <domain><...>()
• de-clutter: if/for/while... imbrication. (The Linux rule of 3 maximum is nice), if functions are way too big, isolate logical blocks in dedicated functions etc...

Areas:

• ipv4.c/icmpv4.c : Network stack cleanup: IPv4 #8720
• ipv6.c/icmpv6.c : Network stack cleanup: IPv6 #8721
• connection.c : Network stack cleanup: connection #8722
• rpl.c : Network stack cleanup: RPL #8724
• net_context.c : Network stack cleanup: Net Context #8725
• dhcpv4 : Network stack cleanup: DHCPv4 #8727
• net_if: Network stack cleanup: Net IF #8728

[tbursztyka]

@jukkar, @rveerama1, @ozhuraki, @pfalcon, whoever else is interested.

[pfalcon]

Here's my 2 cents of proposed cleanup: I hate these things:

tcp.c:NET_CONN_CB(tcp_established)
tcp.c:NET_CONN_CB(tcp_synack_received)
tcp.c:NET_CONN_CB(tcp_syn_rcvd)

It obfuscates the fact that it's function (so, e.g. if you search for "tcp_established(", you'll get nothing), and completely hides the function parameter list. And all this for what?:

        static enum net_verdict name(struct net_conn *conn,       \
                                     struct net_pkt *pkt,         \
                                     void *user_data)             \
        {                                                         \
                enum net_verdict result;                          \
                                                                  \
                net_context_ref(user_data);                       \
                result = _##name(conn, pkt, user_data);           \
                net_context_unref(user_data);                     \
                return result;                                    \
        }                                  

As we have 3 occurrences of this stuff, we could just expand that macro to literal code, and won't lose much. But it's actually more than that - I don't think all that wrapping is actually necessary. Just think what it does - it extends context lifetime until the end of wrapper function. But the wrapper function doesn't use context in any way after the wrappee returns, so this extending is completely useless! It only adds confusion on the source level and extra callframe on the stack.

[pfalcon]

@jukkar , @tbursztyka : Can you assess the proposal in previous comment please?

[pfalcon]

It obfuscates the fact that it's function (so, e.g. if you search for "tcp_established(", you'll get nothing), and completely hides the function parameter list.

#9564 tries to alleviate at least the concerns above, with the code comments.

[pfalcon]

Overall, TCP is the most atrocious part of the stack code. #8726 doesn't pay it good attention. I've created #9570 to discuss the real guts of it.

[tbursztyka]

edited: removed the TCP cleanup tasks.

[tbursztyka]

done