review needs-fixing On Tue, Sep 13, 2011 at 12:20:34PM +0000, Christian Röller wrote: > > === modified file 'Makefile.am' > --- Makefile.am 2011-08-10 15:03:07 +0000 > +++ Makefile.am 2011-09-13 12:20:25 +0000 > @@ -71,11 +71,15 @@ > TESTS = test/check_firewall \ > test/check_lib_core \ > test/check_lib_tool \ > - test/check_modules_midauth > + test/check_modules_midauth \ > + test/check_lib_tool \ > + test/check_hipd > check_PROGRAMS = test/check_firewall \ > test/check_lib_core \ > test/check_lib_tool \ > - test/check_modules_midauth > + test/check_modules_midauth \ > + test/check_lib_tool \ > + test/check_hipd duplicate entries alphabetical order > @@ -207,6 +211,10 @@ > > +test_check_hipd_SOURCES = test/check_hipd.c \ > + test/hipd/netdev.c \ > + modules/midauth/hipd/midauth.c \ > + $(hipd_hipd_sources) Please align the '\' vertically, same as in the rest of the file. > test_check_firewall_SOURCES = test/check_firewall.c \ > test/mocks.c \ Like here for example :) > --- hipd/netdev.c 2011-08-15 14:11:56 +0000 > +++ hipd/netdev.c 2011-09-13 12:20:25 +0000 > @@ -90,54 +90,68 @@ > > -static int hip_netdev_white_list[HIP_NETDEV_MAX_WHITE_LIST]; > -static int hip_netdev_white_list_count = 0; > +struct hip_netdev_whitelist_entry { > + unsigned int if_index; > + char if_label[IF_NAMESIZE]; > +}; > +static struct hip_netdev_whitelist_entry hip_netdev_white_list[HIP_NETDEV_MAX_WHITE_LIST]; > +static unsigned int hip_netdev_white_list_count = 0; > > -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) These unnecessary double prefixes (hip_ and netdev_) for symbols not visible outside of this file are annoying. In a perfect world, you would do a quick search and replace on trunk and merge that back into your branch, but I don't want to pile too much extra work on you. However, if you have a minute and the motivation.. Maybe you could drop them for the new code, but feel free to ignore me. > -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) Oh look, this file only had a hip_ prefix - yay for consistency in HIPL, even in the same file :) > @@ -1120,6 +1133,44 @@ > > /** > + * Gives you the interface label for a given IPv4 or IPv6 address. > + * > + * @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. extra whitespace > + * @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 NULL-terminated > + * is chosen big enough(IF_NAMESIZE == 16). The reference to IF_NAMESIZE does not really enlighten me.. > + */ > +int hip_find_label_for_address(const struct sockaddr *const addr, > + char *const label, unsigned int const sizeoflabel) const unsigned int sizeoflabel reads weird to me, like it was sizeof(label), but you forgot the (), label_size looks more natural to me. > @@ -1211,6 +1257,17 @@ > > + /* 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, sizeof(label)) && See, there we have 'sizeof(label)' :) > --- hipd/netdev.h 2011-05-18 15:12:07 +0000 > +++ hipd/netdev.h 2011-09-13 12:20:25 +0000 > @@ -56,4 +56,7 @@ > int hip_map_id_to_addr(const hip_hit_t *hit, const hip_lsi_t *lsi, > struct in6_addr *addr); > > +int hip_find_label_for_address(const struct sockaddr *const addr, > + char *const label, unsigned int const sizeoflabel); see above > --- 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. Happy new year! Oh wait, it's almost autumn ;) > --- test/hipd/netdev.c 1970-01-01 00:00:00 +0000 > +++ test/hipd/netdev.c 2011-09-13 12:20:25 +0000 > @@ -0,0 +1,89 @@ > + > +#define _BSD_SOURCE Is that required? > +#include > +#include > + > +#include "hipd/netdev.h" > +#include > +#include > +#include "lib/core/debug.h" > +#include "lib/core/prefix.h" > +#include "test_suites.h" Put system headers first, separate local headers with an empty line. > --- test/hipd/test_suites.h 1970-01-01 00:00:00 +0000 > +++ test/hipd/test_suites.h 2011-09-13 12:20:25 +0000 > @@ -0,0 +1,34 @@ > +/* > + * Copyright (c) 2010 Aalto University and RWTH Aachen University. see above > +#ifndef HIP_TEST_HIPD_TEST_SUITES_H > +#define HIP_TEST_HIPD_TEST_SUITES_H > + > +#include > + > +Suite *hipd_netdev(void); > + > + > +#endif /* HIP_TEST_HIPD_TEST_SUITES_H */ nit: extra empty line > --- test/lib/core/hostid.c 2011-09-05 08:46:53 +0000 > +++ test/lib/core/hostid.c 2011-09-13 12:20:25 +0000 > @@ -65,7 +65,7 @@ > START_TEST(test_serialize_deserialize_rsa_keys) > { > unsigned int i, keyrr_len = 0; > - int bits[3] = {1024, 2048, 3072}; > + int bits[3] = { 1024, 2048, 3072 }; > RSA *key = NULL, *key_deserialized = NULL; > EVP_PKEY *key_a = NULL, *key_b = NULL; > unsigned char *keyrr; unrelated Feel free to commit such things to trunk right away, but please leave them out of merge requests. Diego