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

> Hi Christian!
>
> Thanks for your detailed reply! I was on holidays so that's why my answer is
> quite late:

No problem my first reply has also taken a while due to my exam preparations ;-)

>
> >>>> - if (hip_netdev_white_list[i] == if_index) {
> >>>> + if (hip_netdev_white_list[i].if_index == if_index &&
> >>>> + !strncmp(hip_netdev_white_list[i].if_label, device_name,
> >> IF_NAMESIZE)) {
> >>
> >> [L] maintainability: 'sizeof()' instead of 'IF_NAMESIZE', see above
> >
> > Done...but I use sizeof()-1 to do not override the null-termination
>
> Hm - in the case of the read-only 'strncmp()' function, I'm pretty sure that
> you
> want to check both strings to their full extent, including the null-
> terminating
> character. Otherwise, you can get false positives with unterminated strings (I
> know, that's a pathological case, but still, passing 'sizeof() - 1' to
> 'strncmp()' seems logically incorrect to me).

Yes of course...I never did it in the code. This was just a comment at the wrong position.

>
> >>>> +int hip_find_label_for_address(struct sockaddr *addr, char *label)
> >>
> >> [M] policy: please ensure full const correctness
> >
> > I write to label and cast addr, so I cannot declare const, can I?
>
> You can use 'const struct sockaddr *const addr' and cast it to 'const struct
> sockaddr_in const* s4_in'. Similarly, you can use 'char *const label' as you
> do
> not intend to or do change the pointer value.

Yes you are right, did it.

>
> >>>> + res = getifaddrs(&myaddrs);
> >>>> + for (ifa = myaddrs; ifa != NULL; ifa = ifa->ifa_next) {
> >>>> + if (ifa->ifa_addr == NULL) continue;
> >>>> + if (ifa->ifa_addr->sa_family != AF_INET &&
> >> ifa->ifa_addr->sa_family != AF_INET6) continue;
> >>
> >> [M] policy: HIPL always uses "long" if statements including braces and
> >> newlines.
> >> Please adhere to that style. Do you use the pre-commit hook for checking
> your
> >> code style? I'm asking because I'm surprised that the checker does not
> catch
> >> this.
> >
> > For me the statement doesnt include braces or a new line. Maybe I have
> changed it and forgot to commit it...maybe you can check it again...
>
> Exactly my point: it *should* include braces and newlines but currently it
> does
> not :-) It should read
>
> if (ifa->ifa_addr == NULL) {
> continue;
> }
>
> etc.

Ah OK now I got your point...sorry ;-) I changed it

>
> >> [L] maintainability: similar to the above cases, 'sizeof(s4_in)' would be
> more
> >> easily maintainable and more obvious to read than 'sizeof(struct
> >> sockaddr_in)'.
> >> The same applies to the code below.
> >>
> >>>> + strncpy(label, ifa->ifa_name, IF_NAMESIZE);
> >>
> >> [L] 'sizeof()'
> >
> > Both done, like described above...
>
> Just like 'strncmp()', 'strncpy()' should use the full character array - the
> code should also check that the string to copy is not longer than the given
> character array - 1 or otherwise return an error from the function. Only this
> combination will ensure that the string is either copied correctly or an error
> is detected under all circumstances.

OK, I changed it in the following way:
I check first if strlen(ifa->ifa_name) <= sizeof(label)-1 to make sure that we copy the
whole string.

>
> >>>> + freeifaddrs(myaddrs);
> >>>> + return 1;
> >>>> + }
> >>>> + }
> >>>> + if (ifa->ifa_addr->sa_family == AF_INET6) {
> >>>> + s6 = (struct sockaddr_in6 *)(ifa->ifa_addr);
> >>>> + if (!memcmp(s6_in, s6, sizeof(struct sockaddr_in6))) {
> >>>> + strncpy(label, ifa->ifa_name, IF_NAMESIZE);
> >>>> + freeifaddrs(myaddrs);
> >>>> + return 1;
> >>
> >> [L] style: wouldn't it make more sense to skip the call to 'freeifaddrs()',
> >> set
> >> 'res' to '1' and leave the loop with a 'break' (same above)? It would save
> an
> >> additional function exit, which unnecessarily complicates the control flow
> of
> >> the function.
> >
> > I did it this way to improve the loop iterations. So that the loop ends when
> it found a match.
>
> True, but there is code duplication with all the 'freeifaddrs()' and
> 'return's.
> Using 'break' would have the same effect (end the loop, when a match is found)
> but it would avoid the code duplication.

Yes thats true, I changed it...

>
> >>>> === modified file 'hipd/netdev.h'
> >>>> --- hipd/netdev.h 2011-05-18 15:12:07 +0000
> >>>> +++ hipd/netdev.h 2011-07-14 15:34:41 +0000
> >>
> >> Please don't forget to update the Copyright dates when you modify files
> with
> >> Copyright headers. Scatterbrains like myself may want to try out the pre-
> >> commit
> >> hook in https://code.launchpad.net/~stefan.goetz/hipl/commitguard that
> >> automatically checks for that.
> >
> > I was just not fully sure. Did you mean just to change this line
> > 'Copyright (c) 2010 Aalto University and RWTH Aachen University.'
> > from 2010 to 2011? When so, I will do it.
>
> Pretty much yes - the usual style is to use '2005-2006, 2008, 2010-2011' for
> consecutive years and non-consecutive years of modification (i.e. 2010-2011 in
> this case).

Done...

>
> Cheers,
> Stefan

Regards,
Christian