Code review comment for lp:~christian-roeller/hipl/whitelisting

Hi Christian!

Thanks for this helpful contribution! My comments below:

> > === modified file 'hipd/netdev.c'
> > --- hipd/netdev.c 2011-05-31 18:21:28 +0000
> > +++ hipd/netdev.c 2011-07-14 15:34:41 +0000
> > -static void hip_netdev_white_list_add_index(int if_index)
> > +static void hip_netdev_white_list_add_index_and_name(int if_index, const char *const device_name)

[L] cleanup: please add a 'const' modifier to the 'if_index' argument
[L] cleanup: I guess that 'if_index' cannot be less than 0. If so, it would be
nice to change its type to unsigned to communicate this fact to the caller.

> > {
> > if (hip_netdev_white_list_count < HIP_NETDEV_MAX_WHITE_LIST) {
> > - hip_netdev_white_list[hip_netdev_white_list_count++] = if_index;
> > + hip_netdev_white_list[hip_netdev_white_list_count].if_index = if_index;
> > + strncpy(hip_netdev_white_list[hip_netdev_white_list_count].if_label, device_name, IF_NAMESIZE);
[L] maintainability: it is more maintainable to use
'sizeof(hip_netdev_white_list[0].if_label)' than 'IF_NAMESIZE' because then the
size of the 'if_label' field can be changed in the struct declaration without
having to touch other code.

> > @@ -120,16 +127,18 @@
> > }
> >
> > /**
> > - * Test if the given network interface index is white listed.
> > + * Test if the given network interface index plus label is white listed.
> > *
> > * @param if_index the index of the network interface to be tested
> > + * @param device_name the label of the network interface to be tested
> > * @return 1 if the index is whitelisted or zero otherwise
> > */
> > -static int hip_netdev_is_in_white_list(int if_index)
> > +static int hip_netdev_is_in_white_list(int if_index, char *device_name)

[M] policy: please apply full const correctness to the function arguments.
[L] cleanup: it would be nice to change the type of 'if_index' to unsigned as it
can never be < 0 (I guess)

> > {
> > - int i = 0;
> > + int i;
> > for (i = 0; i < hip_netdev_white_list_count; i++) {

[L] style: you could even place the declaration of 'i' in the for() header.

> > - if (hip_netdev_white_list[i] == if_index) {
> > + if (hip_netdev_white_list[i].if_index == if_index &&
> > + !strncmp(hip_netdev_white_list[i].if_label, device_name, IF_NAMESIZE)) {

[L] maintainability: 'sizeof()' instead of 'IF_NAMESIZE', see above

> > @@ -637,7 +645,7 @@
> > HIP_IFEL(!(if_index = if_nametoindex(g_iface->ifa_name)),
> > -1, "if_nametoindex failed\n");
> > /* Check if our interface is in the whitelist */
> > - if ((hip_netdev_white_list_count > 0) && (!hip_netdev_is_in_white_list(if_index))) {
> > + if ((hip_netdev_white_list_count > 0) && (!hip_netdev_is_in_white_list(if_index, g_iface->ifa_name))) {

[L] style: just a general comment: it would make more sense to move the '> 0'
check into the 'hip_netdev_is_in_white_list()' function so the caller does not
need to have internal knowledge about the list implementation. Furthermore it's
a really silly performance optimization.

> > @@ -1125,6 +1133,55 @@
> > }
> >
> > /**
> > + * find interface label for a given ip4 or ip6 addr
> > + *
> > + * @param addr address for which you want to know the label
> > + * @param label pointer where the function stores the label
> > + * @return one on success and non-one on error and write the label in param label
> > + */

[L] style: please use full sentences in documentation. I know it's annoying but
short-hand style tends to obfuscate the meaning.

> > +int hip_find_label_for_address(struct sockaddr *addr, char *label)

[M] policy: please ensure full const correctness

> > +{
> > + int res = 0;
> > + struct ifaddrs *myaddrs, *ifa = NULL;
> > +
> > + struct sockaddr_in *s4_in = NULL;
> > + struct sockaddr_in6 *s6_in = NULL;
> > + if (addr->sa_family == AF_INET) {
> > + s4_in = (struct sockaddr_in *)(addr);
> > + }
> > + if (addr->sa_family == AF_INET6) {
> > + s6_in = (struct sockaddr_in6 *)(addr);
> > + }
> > +
> > + struct sockaddr_in *s4;
> > + struct sockaddr_in6 *s6;

[M] policy: please declare variables at a consistent location. From my
experience, the style in HIPL is to declare variables at the beginning of their
scope before other statements, as required by ANSI C. The exception to that rule
is if a variable can be declared 'const' thanks to a later declaration.

> > + res = getifaddrs(&myaddrs);
> > + for (ifa = myaddrs; ifa != NULL; ifa = ifa->ifa_next) {
> > + if (ifa->ifa_addr == NULL) continue;
> > + if (ifa->ifa_addr->sa_family != AF_INET && ifa->ifa_addr->sa_family != AF_INET6) continue;

[M] policy: HIPL always uses "long" if statements including braces and newlines.
Please adhere to that style. Do you use the pre-commit hook for checking your
code style? I'm asking because I'm surprised that the checker does not catch this.

> > +
> > + if (ifa->ifa_addr->sa_family == AF_INET) {
> > + s4 = (struct sockaddr_in *)(ifa->ifa_addr);
> > + if (!memcmp(s4_in, s4, sizeof(struct sockaddr_in))) {

[L] maintainability: similar to the above cases, 'sizeof(s4_in)' would be more
easily maintainable and more obvious to read than 'sizeof(struct sockaddr_in)'.
The same applies to the code below.

> > + strncpy(label, ifa->ifa_name, IF_NAMESIZE);

[L] 'sizeof()'

> > + freeifaddrs(myaddrs);
> > + return 1;
> > + }
> > + }
> > + if (ifa->ifa_addr->sa_family == AF_INET6) {
> > + s6 = (struct sockaddr_in6 *)(ifa->ifa_addr);
> > + if (!memcmp(s6_in, s6, sizeof(struct sockaddr_in6))) {
> > + strncpy(label, ifa->ifa_name, IF_NAMESIZE);
> > + freeifaddrs(myaddrs);
> > + return 1;

[L] style: wouldn't it make more sense to skip the call to 'freeifaddrs()', set
'res' to '1' and leave the loop with a 'break' (same above)? It would save an
additional function exit, which unnecessarily complicates the control flow of
the function.

> > + }
> > + }
> > + }
> > + freeifaddrs(myaddrs);
> > + return res;
> > +}
> > @@ -1216,6 +1268,19 @@
> > HIP_DEBUG("Unknown addr family in addr\n");
> > }
> >
> > + /* find interface label for address and check if it is whitelisted or not */
> > + if (is_add) {
> > + char label[IF_NAMESIZE];
> > + if (hip_find_label_for_address(addr, label)) {
> > + if ((hip_netdev_white_list_count > 0) &&
> > + (!hip_netdev_is_in_white_list(ifa->ifa_index, label))) {

[L] simplicity: the check for 'hip_netdev_white_list_count' seems useless to me.
[M] style: the '(!hip_netdev[...]' line should be indented one more.

> > + HIP_DEBUG("Interface:<%s> is not whitelisted\n", label);
> > + continue;
> > + }
> > + }

[M] Does this code behave correctly when 'hip_find_label_for_address(addr,
label)' returns a non-zero value?

> > === modified file 'hipd/netdev.h'
> > --- hipd/netdev.h 2011-05-18 15:12:07 +0000
> > +++ hipd/netdev.h 2011-07-14 15:34:41 +0000

Please don't forget to update the Copyright dates when you modify files with
Copyright headers. Scatterbrains like myself may want to try out the pre-commit
hook in https://code.launchpad.net/~stefan.goetz/hipl/commitguard that
automatically checks for that.

Cheers,
 Stefan