Once my exam is over I have finally found the time to work on your improvement suggestions...
> 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.
Both done, you are right no reason here for dont make it unsigned and const...
>
> > > {
> > > 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.
Good point I didnt take this into account, its changed now
>
> > > @@ -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)
Same as above, so done...
>
> > > {
> > > - 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.
Yes I can, so I did... ;-) The actual writer did it like this, so I just adopt it...
>
> > > - 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
Done...but I use sizeof()-1 to do not override the null-termination
>
> > > @@ -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.
Of course you are right with both, so I did it...
>
> > > @@ -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.
I write to label and cast addr, so I cannot declare const, can I?
>
> > > +{
> > > + 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.
Done...
>
> > > + 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.
For me the statement doesnt include braces or a new line. Maybe I have changed it and forgot to commit it...maybe you can check it again...
>
> > > +
> > > + 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()'
Both done, like described above...
>
> > > + 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.
I did it this way to improve the loop iterations. So that the loop ends when it found a match.
>
> > > + }
> > > + }
> > > + }
> > > + 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.
The check for 'hip_netdev_white_list_count' is now anyway in 'hip_netdev_is_in_white_list'.
Indention came automatically by deleting the first line ;-)
>
> > > + 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?
This is why I only check this when a new address is assigned to the system.
For old addresses this check would of course fail. But when it is impossible
to find out the label, which should not be the case for a new address, the
behaviour would be just like it was before my changes.
>
> > > === 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.
I was just not fully sure. Did you mean just to change this line
'Copyright (c) 2010 Aalto University and RWTH Aachen University.'
from 2010 to 2011? When so, I will do it.
Once my exam is over I have finally found the time to work on your improvement suggestions...
> Hi Christian! white_list_ add_index( int if_index) white_list_ add_index_ and_name( int if_index, const
>
> 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_
> > > +static void hip_netdev_
> 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.
Both done, you are right no reason here for dont make it unsigned and const...
> 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 = hip_netdev_ white_list[ hip_netdev_ white_list_ count]. if_label, hip_netdev_ white_list[ 0].if_label) ' than 'IF_NAMESIZE' because then
> > > {
> > > if (hip_netdev_
> > > - hip_netdev_
> > > + hip_netdev_
> if_index;
> > > +
> strncpy(
> device_name, IF_NAMESIZE);
> [L] maintainability: it is more maintainable to use
> 'sizeof(
> the
> size of the 'if_label' field can be changed in the struct declaration without
> having to touch other code.
Good point I didnt take this into account, its changed now
> is_in_white_ list(int if_index) is_in_white_ list(int if_index, char *device_name)
> > > @@ -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_
> > > +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)
Same as above, so done...
> 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.
Yes I can, so I did... ;-) The actual writer did it like this, so I just adopt it...
> white_list[ i] == if_index) { white_list[ i].if_index == if_index && hip_netdev_ white_list[ i].if_label, device_name,
> > > - if (hip_netdev_
> > > + if (hip_netdev_
> > > + !strncmp(
> IF_NAMESIZE)) {
>
> [L] maintainability: 'sizeof()' instead of 'IF_NAMESIZE', see above
Done...but I use sizeof()-1 to do not override the null-termination
> g_iface- >ifa_name) ), white_list_ count > 0) && is_in_white_ list(if_ index)) ) { white_list_ count > 0) && is_in_white_ list(if_ index, g_iface- >ifa_name) )) { is_in_white_ list()' function so the caller does not
> > > @@ -637,7 +645,7 @@
> > > HIP_IFEL(!(if_index = if_nametoindex(
> > > -1, "if_nametoindex failed\n");
> > > /* Check if our interface is in the whitelist */
> > > - if ((hip_netdev_
> (!hip_netdev_
> > > + if ((hip_netdev_
> (!hip_netdev_
>
> [L] style: just a general comment: it would make more sense to move the '> 0'
> check into the 'hip_netdev_
> need to have internal knowledge about the list implementation. Furthermore
> it's
> a really silly performance optimization.
Of course you are right with both, so I did it...
>
> > > @@ -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.
Okidoki ;-) done
> label_for_ address( struct sockaddr *addr, char *label)
> > > +int hip_find_
>
> [M] policy: please ensure full const correctness
I write to label and cast addr, so I cannot declare const, can I?
>
> > > +{
> > > + 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.
Done...
> &myaddrs) ; addr->sa_ family != AF_INET && addr->sa_ family != AF_INET6) continue;
> > > + res = getifaddrs(
> > > + for (ifa = myaddrs; ifa != NULL; ifa = ifa->ifa_next) {
> > > + if (ifa->ifa_addr == NULL) continue;
> > > + if (ifa->ifa_
> 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.
For me the statement doesnt include braces or a new line. Maybe I have changed it and forgot to commit it...maybe you can check it again...
> 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()'
Both done, like described above...
> myaddrs) ; addr->sa_ family == AF_INET6) { myaddrs) ;
> > > + freeifaddrs(
> > > + 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.
I did it this way to improve the loop iterations. So that the loop ends when it found a match.
> myaddrs) ; label_for_ address( addr, label)) { white_list_ count > 0) && is_in_white_ list(ifa- >ifa_index, white_list_ count' seems useless to
> > > + }
> > > + }
> > > + }
> > > + 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_
> label))) {
>
> [L] simplicity: the check for 'hip_netdev_
> me.
> [M] style: the '(!hip_netdev[...]' line should be indented one more.
The check for 'hip_netdev_ white_list_ count' is now anyway in 'hip_netdev_ is_in_white_ list'.
Indention came automatically by deleting the first line ;-)
> "Interface: <%s> is not whitelisted\n", label_for_ address( addr,
> > > + HIP_DEBUG(
> label);
> > > + continue;
> > > + }
> > > + }
>
> [M] Does this code behave correctly when 'hip_find_
> label)' returns a non-zero value?
This is why I only check this when a new address is assigned to the system.
For old addresses this check would of course fail. But when it is impossible
to find out the label, which should not be the case for a new address, the
behaviour would be just like it was before my changes.
> /code.launchpad .net/~stefan. goetz/hipl/ commitguard that
> > > === 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:/
> automatically checks for that.
I was just not fully sure. Did you mean just to change this line
'Copyright (c) 2010 Aalto University and RWTH Aachen University.'
from 2010 to 2011? When so, I will do it.
>
> Cheers,
> Stefan