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

Revision history for this message
Christian Röller (christian-roeller) wrote :

> 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

Everything done...
I also adapt the autobuild script to my local system conditions here and ran it. It makes no trouble. So make alltests and the autobuild script work without problems.

Christian

« Back to merge proposal