HIPL

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

whitelisting
Merge into trunk

Revision history for this message

Christian Röller (christian-roeller) wrote on 2011-07-24:

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.

Okidoki ;-) done

>
> > > +int hip_find_label_for_address(struct sockaddr *addr, char *label)
>
> [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...

>
> > > + 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.

>
> Cheers,
> Stefan

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.

Okidoki ;-) done

> 
> > > +int hip_find_label_for_address(struct sockaddr *addr, char *label)
> 
> [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...

> 
> > > +    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.

> 
> Cheers,
>         Stefan

« Back to merge proposal