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

 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 <check.h>
> +#include <stdlib.h>
> +
> +#include "hipd/netdev.h"
> +#include <ifaddrs.h>
> +#include <net/if.h>
> +#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 <check.h>
> +
> +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