Code review comment for lp:~cviethen/hipl/pisa-pairing-tng

On 09.01.2012 03:43, Christoph Viethen wrote:
> Hello,
>
> on Jan 8, 2012, at 7:43 PM, Christof Mroz wrote:
>
>>> + * This function searches for a<tt> struct hip_hadb_state</tt> entry from the
>>> + * @c hadb_hit by the given tuple (source hit, dest. ip, dest. port).
>>
>> Use ::hip_hadb_state and ::hadb_hit rather than syntactic markup, so
>> that doxygen can generate cross-references in html output.
>
> Many thanks for the Doxygen advice! Not my greatest area of expertise, really.
>
> Would it make sense to add the "::" and, additionally, keep the syntactic markup? Especially in the case of "struct hip_hadb_state" the keyword "struct" looks a little "alone" if it's kept complete unformatted.

I'd just drop the "struct" altogether, because for high-level
comprehension it doesn't matter whether it's a struct or maybe a typedef
or whatever. When actually calling the function you need to look at the
prototype anyway, and will spot the "struct" qualifier.

> I'll put both in right now - maybe you could take a look again whether that's okay or a bit too much?

Sure, unless the function is now obsoleted by Miika's advice.

>>> @@ -1094,6 +1093,56 @@
>>> + /* Find out: 1. our own hit, */
>>> + if (hip_get_default_hit(&our_hit)< 0) {
>>> + return -1;
>>> + }
>>
>> You should run uncrustify again.
>
> Did that, didn't change. :-) Though I must say that "my" code looks a bit different from the one you're quoting (e.g., there's a space between ")" and "<"). No idea what happened. E-mail software acting up?

You're right there, interesting... good to know that my user-agent sucks :)

>>> + * Handles the<tt> hipconf daemon trigger bex</tt> command.
>>
>> Same as above
>
> You mean the "::" part, or the "run uncrustify again" part? I'll just leave it for the moment - it's meant as a command line, so "::" at least doesn't make sense from my (limited) point of view.

Agreed, :: doesn't make sense here.

>>> + IPV4_TO_IPV6_MAP(&((struct sockaddr_in *) ai_res->ai_addr)->sin_addr,&ipv6_addr);
>>> + done = 1;
>>> + }
>>> + if (ai_res->ai_addr->sa_family == AF_INET6) {
>>> + memcpy(&ipv6_addr,&((struct sockaddr_in6 *) ai_res->ai_addr)->sin6_addr, sizeof(struct in6_addr));
>>> + done = 1;
>>> + }
>>
>> These lines can be shortened without much hassle. Also, are you sure
>> there is no utilty function for copying IPV6 addresses yet?
>
> Good idea - there's ipv6_addr_copy() of course. Which makes shortening the whole thing easier, as well. Please let me know whether my shortening / wrapping strategy looks good. Seems a little awkward to me, but let's see.

OK, take a look at HACKING when in doubt.

>>> + if (done == 0) {
>>> + HIP_ERROR("Failed to find any IP address for hostname.");
>>> + return -1;
>>> + }
>>
>> This while loop looks like it can be simplified:
>>
>> for(ai_res = addrinfo_result; ai_res != NULL; ai_res = ai_res->next) {
>> if(...) { copy; break; }
>> if(...) { copy; break; }
>> }
>>
>> if(!ai_res) { error; }
>
> Not sure - right now I feel I should leave it the way it is. I'd like to preserve the information whether I found something while walking the addrinfo_result or not - just "break;"ing doesn't achieve that, as far as I can tell right now. (I could have just walked the whole list without finding anything and still arrived at the same place in the code.)

The only way that ai_res can be NULL after the loop is when it wasn't
aborted preliminary (using break). But maybe it's not as clear as I
thought... then I'd rather keep the temporary variable.