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

 review needs-fixing

On Thu, Aug 11, 2011 at 11:48:47AM +0000, Christian Röller wrote:
> Christian Röller has proposed merging lp:~christian-roeller/hipl/whitelisting into lp:hipl.
>
> --- hipd/netdev.c 2011-05-25 09:22:19 +0000
> +++ hipd/netdev.c 2011-08-11 11:48:38 +0000
> @@ -99,19 +99,27 @@
> 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, sizeof(hip_netdev_white_list[0].if_label) - 1);

This is an opportunity for breaking a line, as is

> @@ -119,17 +127,20 @@
>
> -static int hip_netdev_is_in_white_list(int if_index)
> +static int hip_netdev_is_in_white_list(const unsigned int if_index, const char *const device_name)

this one

> + if (hip_netdev_white_list_count > 0) {
> + for (unsigned int i = 0; i < hip_netdev_white_list_count; i++) {
> + if (hip_netdev_white_list[i].if_index == if_index &&
> + !strncmp(hip_netdev_white_list[i].if_label, device_name, sizeof(hip_netdev_white_list[0].if_label))) {

and this one.

> @@ -1122,6 +1132,40 @@
>
> /**
> + * This functions gives you the interface label for a given IPv4 or IPv6 address.

This sentence explains why starting a sentence with "this sentence" is
redundant and weird.

> + * @param addr address for which you want to know the label
> + * @param label pointer where the function stores the label
> + * @return one on success, zero on error and write the label in param label
> + * if the given addr exists in the system

address

The usual convention is to return zero on success, not one.

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

long line

> +{
> + int res = 0;
> + struct ifaddrs *myaddrs, *ifa = NULL;
> +
> + getifaddrs(&myaddrs);
> + for (ifa = myaddrs; ifa != NULL; ifa = ifa->ifa_next) {
> + if (ifa->ifa_addr == NULL ||
> + (ifa->ifa_addr->sa_family != AF_INET && ifa->ifa_addr->sa_family != AF_INET6)) {

long line

> @@ -1213,6 +1252,18 @@
>
> + /* 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_is_in_white_list(ifa->ifa_index, label))) {
> + HIP_DEBUG("Interface:<%s> is not whitelisted\n", label);
> + continue;

The conditions can be merged.

> --- hipd/netdev.h 2011-05-18 15:12:07 +0000
> +++ hipd/netdev.h 2011-08-11 11:48:38 +0000
> @@ -56,4 +56,6 @@
>
> +int hip_find_label_for_address(const struct sockaddr *const addr, char *const label);

long line

Diego