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. 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. [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. 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'. > /** > + * Gives you the interface label for a given IPv4 or IPv6 address. `Gives you' -> `Returns' > + * > + * @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). Also, refer to the doxygen section in HACKING on how to markup parameter names (I don't remember right now). > +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. > + > + 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). 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. > === 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