Patch Set 2: (4 comments)
https://gerrit.osmocom.org/#/c/4256/2/lib/in46_addr.c File lib/in46_addr.c: Line 198: unsigned int netmask_ipv4_prefixlen(const struct in_addr *netmask) the new functions should have test cases in the (newly introduced) in46a_tests.c file. Adding these three functions and their test cases can then be split out into a separate patch. Also, if we don't need the ipv4/ipv6 specific functions in other code, they could be made static with in46a_prefixlne being the only exported API. Also, doxygen documentation is missing. Line 234: unsigned int in46a_prefixlen(const struct in46_addr *netmask) let's call it in46a_netmasklen or the like? I don't want to have a name that implies it returns the length of a prefix (whihc would take an in46_prefix as input argument anyway) https://gerrit.osmocom.org/#/c/4256/2/lib/tun.c File lib/tun.c: Line 755: int tun_ipv4_local_get(const struct tun_t *tun, struct in46_prefix *prefix) I think this is copy+paste style programming with the ipv6 function below, and the two should probably be unified. Would you agree? https://gerrit.osmocom.org/#/c/4256/2/lib/tun.h File lib/tun.h: Line 97: int tun_ipv6_local_get(const struct tun_t *tun, struct in46_prefix *prefix, int flags); the change of tun_ipv6_local_get() api with flags could be split out as separate commit -- To view, visit https://gerrit.osmocom.org/4256 To unsubscribe, visit https://gerrit.osmocom.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I8e91f7280d60490c858a769dd578c1c8e54e9243 Gerrit-PatchSet: 2 Gerrit-Project: osmo-ggsn Gerrit-Branch: master Gerrit-Owner: Pau Espin Pedrol <[email protected]> Gerrit-Reviewer: Harald Welte <[email protected]> Gerrit-Reviewer: Jenkins Builder Gerrit-HasComments: Yes
