openggsn[master]: IPv6 support

[Harald Welte]

Patch Set 1:

(12 comments)

Hi Gerrie, first of all thanks a lot for your patch submission to OpenGGSN.  IPv6 support has been on the wishlist for a long time, but we have virtually received no outside contributions to OpenGGSN for a long time.  Here are some initial review comments.  For more extensive discussions, we can also always use the mailing list.

One general comment on commenting: Please use /* */ style commenting, rather than // style. If you think that's too much to ask, I will fix those up after we're done with the other review.

https://gerrit.osmocom.org/#/c/2870/1/ggsn/tun.c
File ggsn/tun.c:

Line 74: // From linux/ipv6.h
why not #include <linux/ipv6.h> instead of copying?  We're in a #ifdef(__linux__) section anyway.


Line 241: // None of this is currently required for IPv6
if it's not required for ipv6, we can simply remove/disable the code, even in the v4 case?


Line 894:   // TODO: ipv6
do you still intend to address those TODO items (in several places) in future versions of this patch?


https://gerrit.osmocom.org/#/c/2870/1/ggsn/tun.h
File ggsn/tun.h:

Line 20: /* This should not be re-declared!  Why not just use netinet/ip(6).h?
feel free to submit a separate patch(set) if you think there is some clean-up with re-use of system header files.


https://gerrit.osmocom.org/#/c/2870/1/gtp/gtp.c
File gtp/gtp.c:

Line 1473:   struct in6_addr* a;
I think we don't declare variables in the middle of functions but still the classic way on top of the function (or block).


https://gerrit.osmocom.org/#/c/2870/1/gtp/gtp.h
File gtp/gtp.h:

Line 401: extern int gsna2in_addr(struct in6_addr *dst, size_t* dstlen, struct ul16_t *gsna);
the function now appears to convert to in6_addr, so the name might change to reflect that, too?


https://gerrit.osmocom.org/#/c/2870/1/gtp/pdp.c
File gtp/pdp.c:

Line 332:   eua->l=18;
even if the existing code is not a good example in this case, it would be great if there were not more magic numbers bu t less.  the '16' below could possibly be a sizeof(struct in6addr) or something similar, and the 18 coudl be 2+sizeof()? Applies to other places, too.


Line 374:   case 0x21: /* IPv4 */
I'd also appreciate if we could introduce some #defines for those magic numbers. a comment above the #define should point to the specific section and 3GPP spec number.


https://gerrit.osmocom.org/#/c/2870/1/gtp/pdp.h
File gtp/pdp.h:

Line 161:   size_t gsnlc_len;
maybe it makes sense to introduce a new (sub)struct or typedef that encapsulates both the ul16_t for the address as well as the length?  It seems a bit "copy+paste style" to add a length member for each of the GSN addresses?


https://gerrit.osmocom.org/#/c/2870/1/sgsnemu/ippool.c
File sgsnemu/ippool.c:

Line 45:     if (addrlen == 4) {
might make sense to factor this addrlen2sockaddrlen into a separate function?


https://gerrit.osmocom.org/#/c/2870/1/sgsnemu/tun.c
File sgsnemu/tun.c:

Line 74: // From linux/ipv6.h
please simply #include the related header, or explain in the comment why this is not / cannot be done.


https://gerrit.osmocom.org/#/c/2870/1/sgsnemu/tun.h
File sgsnemu/tun.h:

Line 20: /* This should not be re-declared!  Why not just use netinet/ip(6).h?
same comment as before: If you think we can clean-up by using system level header files, a separate patch(set) about that is much appreciated.


-- 
To view, visit https://gerrit.osmocom.org/2870
To unsubscribe, visit https://gerrit.osmocom.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: If8ce8b4b8cd2ba97f7ba122de4703983111046e4
Gerrit-PatchSet: 1
Gerrit-Project: openggsn
Gerrit-Branch: master
Gerrit-Owner: groos at xiplink.com <groos at xiplink.com>
Gerrit-Reviewer: Harald Welte <laforge at gnumonks.org>
Gerrit-Reviewer: Jenkins Builder
Gerrit-HasComments: Yes