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'll put both in right now - maybe you could take a look again whether that's okay or a bit too much?

> src_hit can be const.

And I found a few more, still. :-)

> Would the following do the trick?
> 
> list_for_each_safe(item, aux, hadb_hit, i)
> {
>     tmp = list_entry(item);
> 
>    [...]

Thanks, good idea - that happens when one just lazily copies and slightly modifies somebody else's code fragment. :)

>> @@ -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?

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

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

>> +        if (addrinfo_result != NULL) {
>> +            freeaddrinfo(addrinfo_result);
>> +        }
> 
> I'd free it even if ai_err wasn't 0, just to be sure.

I'll be naive and trust the POSIX example code and the Linux man page (which at least suggests one only needs to look after that when getaddrinfo() was successful). :-)

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

Many thanks for your useful input!

Cheers,

  Christoph

-- 
 <email address hidden>