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:

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

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

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

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

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

>>>> === 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).

Cheers,
 Stefan

« Back to merge proposal