> Most of the following are just suggestions, though there's one stopper.
> 
> > -static void hip_netdev_white_list_add_index(int if_index)
> > +static void hip_netdev_white_list_add_index_and_name(const unsigned int
> if_index,
> > +                                                     const char *const
> device_name)
> >  {
> >      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));
> > +        hip_netdev_white_list_count++;
> 
> Sorry to bring up this issue again :)
> 
> I suspect you already know that the strncpy() may leave you with an
> unterminated if_label here, and you're betting on a sensible length of
> device_name. See e.g. [1] for an explanation.
> It's always a good idea to add an assert() when gambling, for the sole
> purpose of documentation if anything. But better still, I'd terminate
> the string explicitly.
> 
> If I'm mistaken here, please comment the code (or the device_name
> parameter) to clarify. Audit all the other instances of strncpy() you
> touched, too.

Actually you are right, but in this case the null-termination is guaranteed.
Because the max interface name length in linux is 15 and I am allocating 16 byte(IF_NAMESIZE)
for my hip_netdev_whitelist_entry struct. And in strncpy I always write IF_NAMESIZE many bytes,
so even in the case our system has a maximum interface name of 15, I would write at least one byte
more and terminate the string in this manner, because strncpy would fill up the buffer with nulls.
So if_label has at least one null-termination, possibly more. I know this is not the definition of 
a c-string with only one null-termination at the end, but actually this should not result in any problems. If I am wrong please correct me...  

> 
> Also, dropping the hip_netdev_ prefix, at least for static identifiers,
> looks safe to me and it would vastly shorten line lengths everywhere in
> this file.

Yes of course I can do it, thats because I didn't write this function I just adopt them.
So I tried to change as few as possible.

> 
> [1] http://stackoverflow.com/questions/1453876/why-does-strncpy-not-null-
> terminate
> 
> > +    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))) {
> > +                return 1;
> > +            }
> >          }
> >      }
> 
> Drop the outer if(...): This check is performed on the first for(...)
> iteration
> anyway.

Same here I guess this was the actual auther of the function, but of course you are right this is duplicative. I just don't see it...

> 
>      return 0;
>  }
> 
> >  /**
> > - * Add a network interface index number to the list of white listed
> > + * Add a network interface index number to the list of whitelisted
> >   * network interfaces by name.
> >   *
> 
> You replaced `white list' with `whitelist' in comments, so you should
> change it in identifiers as well: `white_list' -> `whitelist'.

Again a case of adoption, the actual author use this syntax, so I adopt it.
This change was more the grammar owed, which should be adhered to in comments more 
than for variable names. ;-) But I can change it of course lets see what the others
think about it...

> 
> >  /**
> > + * Gives you the interface label for a given IPv4 or IPv6 address.
> 
> `Gives you' -> `Returns'

Oops, this stands colloquially for Returns... ;-) Changed it...

> 
> > + *
> > + * @param addr          Address for which you want to know the label
> > + * @param label         Pointer where the function stores the label.
> > + * @param sizeoflabel        Size of the passed label parameter, should be
> chosen big enough
> > + *                      to be able to store every possible interface name
> plus null-termination.
> > + * @return              Zero on success, one on error and write the label
> > + *                      in param label if the given address exists in the
> system.
> > + *                      Label will be null terminated for sure, iff param
> label
> > + *                      is chosen big enough(IF_NAMESIZE == 16).
> > + */
> 
> I'd say it's reasonable to require `sizeof(label) >= IF_NAMESIZE' in the
> documentation and drop the sizeoflabel parameter. Note that you can
> specify
> 
> int hip_find_label_for_address(const struct sockaddr *const addr,
>                                char label[IF_NAMESIZE])
> 
> to enable a minimal compile-time check (at least for statically
> allocated buffers. Hey, better than nothing).

Yes you are right, but to make use of it we need a statically allocated buffer, which we use for calling this function. This would only make sense if the function itself is static, right?

The problem here is, that I couldn't determine the size of the passed char pointer array.
So I couldn't check if I would override the memory or not. This was the reason for the sizeoflabel parameter, which doesn't solve the problem, but makes it maybe more unlikely...

> 
> Also, refer to the doxygen section in HACKING on how to markup parameter
> names (I don't remember right now).

OK, I will have a look.

> 
> > +int hip_find_label_for_address(const struct sockaddr *const addr,
> > +                               char *const label, unsigned int const
> sizeoflabel)
> 
> Passing addresses by-value is preferred.
> 
> > +    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)) {
> > +            continue;
> > +        }
> 
> The NULL check is already inferred by the other checks.

Yes...

> 
> > +
> > +        if (!memcmp(addr, ifa->ifa_addr, sizeof(addr))) {
> > +            if (strlen(ifa->ifa_name) < sizeoflabel) {
> > +                strncpy(label, ifa->ifa_name, sizeoflabel);
> > +                res = 0;
> > +            }
> > +            break;
> > +        }
> > +    }
> 
> Stopper: addr is a pointer, so sizeof(addr) is always sizeof(void*); you
> meant sizeof(*addr).

Oh of course, thanks for the hint I have changed it...

> 
> This one may have slipped through your tests because accidentally,
> sizeof(void*) = sizeof(struct sockaddr). Be sure to check your code with
> IPv6, too.
> 
> > +                if (!hip_find_label_for_address(addr, label, sizeof(label))
> &&
> > +                    !hip_netdev_is_in_white_list(ifa->ifa_index, label)) {
> > +                    HIP_DEBUG("Interface:<%s> is not whitelisted\n",
> label);
> > +                    continue;
> > +                }
> 
> Maybe it's just me, but you return zero both for success and failure
> depending on the function, which confused me at first. Probably no big
> deal since these functions are not public, though.

Again due to adoption, hip_netdev_is_in_white_list was already there and for the function 
hip_find_label_for_address I have changed it because at some point of this review it was told that this is standard.

> 
> > === added file 'test/check_hipd.c'
> > --- test/check_hipd.c 1970-01-01 00:00:00 +0000
> > +++ test/check_hipd.c 2011-09-13 12:20:25 +0000
> > @@ -0,0 +1,41 @@
> > +/*
> > + * Copyright (c) 2010 Aalto University and RWTH Aachen University.
> > + *
> 
> 2011