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

On Tue, Sep 13, 2011 at 03:41:04PM +0000, Christof Mroz wrote:
>
> > +
> > + 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.

good catch!

> > + 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.

You can find this all over HIPL, annoying as heck if you ask me :)

Somebody should open a bug about this and add it to the eternal todo
list... :)

Diego