[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.
[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.
[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.
[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.
[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?
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.
Hi Christian!
Thanks for this helpful contribution! My comments below:
> > === modified file 'hipd/netdev.c' white_list_ add_index( int if_index) white_list_ add_index_ and_name( int if_index, const char *const device_name)
> > --- hipd/netdev.c 2011-05-31 18:21:28 +0000
> > +++ hipd/netdev.c 2011-07-14 15:34:41 +0000
> > -static void hip_netdev_
> > +static void hip_netdev_
[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.
> > { white_list_ count < HIP_NETDEV_ MAX_WHITE_ LIST) { white_list[ hip_netdev_ white_list_ count++ ] = if_index; white_list[ hip_netdev_ white_list_ count]. if_index = if_index; hip_netdev_ white_list[ hip_netdev_ white_list_ count]. if_label, device_name, IF_NAMESIZE); hip_netdev_ white_list[ 0].if_label) ' than 'IF_NAMESIZE' because then the
> > if (hip_netdev_
> > - hip_netdev_
> > + hip_netdev_
> > + strncpy(
[L] maintainability: it is more maintainable to use
'sizeof(
size of the 'if_label' field can be changed in the struct declaration without
having to touch other code.
> > @@ -120,16 +127,18 @@ is_in_white_ list(int if_index) is_in_white_ list(int if_index, char *device_name)
> > }
> >
> > /**
> > - * 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_
> > +static int hip_netdev_
[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)
> > { white_list_ count; i++) {
> > - int i = 0;
> > + int i;
> > for (i = 0; i < hip_netdev_
[L] style: you could even place the declaration of 'i' in the for() header.
> > - if (hip_netdev_ white_list[ i] == if_index) { white_list[ i].if_index == if_index && hip_netdev_ white_list[ i].if_label, device_name, IF_NAMESIZE)) {
> > + if (hip_netdev_
> > + !strncmp(
[L] maintainability: 'sizeof()' instead of 'IF_NAMESIZE', see above
> > @@ -637,7 +645,7 @@ g_iface- >ifa_name) ), white_list_ count > 0) && (!hip_netdev_ is_in_white_ list(if_ index)) ) { white_list_ count > 0) && (!hip_netdev_ is_in_white_ list(if_ index, g_iface- >ifa_name) )) {
> > HIP_IFEL(!(if_index = if_nametoindex(
> > -1, "if_nametoindex failed\n");
> > /* Check if our interface is in the whitelist */
> > - if ((hip_netdev_
> > + if ((hip_netdev_
[L] style: just a general comment: it would make more sense to move the '> 0' is_in_white_ list()' function so the caller does not
check into the 'hip_netdev_
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) ; addr->sa_ family != AF_INET && ifa->ifa_ addr->sa_ family != AF_INET6) continue;
> > + for (ifa = myaddrs; ifa != NULL; ifa = ifa->ifa_next) {
> > + if (ifa->ifa_addr == NULL) continue;
> > + if (ifa->ifa_
[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.
> > + addr->sa_ family == AF_INET) {
> > + if (ifa->ifa_
> > + 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) ; addr->sa_ family == AF_INET6) { myaddrs) ;
> > + return 1;
> > + }
> > + }
> > + if (ifa->ifa_
> > + s6 = (struct sockaddr_in6 *)(ifa->ifa_addr);
> > + if (!memcmp(s6_in, s6, sizeof(struct sockaddr_in6))) {
> > + strncpy(label, ifa->ifa_name, IF_NAMESIZE);
> > + freeifaddrs(
> > + 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.
> > + } myaddrs) ; label_for_ address( addr, label)) { white_list_ count > 0) && is_in_white_ list(ifa- >ifa_index, label))) {
> > + }
> > + }
> > + freeifaddrs(
> > + 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_
> > + if ((hip_netdev_
> > + (!hip_netdev_
[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 /code.launchpad .net/~stefan. goetz/hipl/ commitguard that
Copyright headers. Scatterbrains like myself may want to try out the pre-commit
hook in https:/
automatically checks for that.
Cheers,
Stefan