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

Good work concerning functionality! Some remarks below.

On 08.01.2012 18:55, Christoph Viethen wrote:
> === modified file 'hipd/hadb.c'
> --- hipd/hadb.c 2011-12-30 23:20:44 +0000
> +++ hipd/hadb.c 2012-01-08 17:54:25 +0000
> @@ -1400,6 +1400,45 @@
> }
>
> /**
> + * 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.

> +struct hip_hadb_state *hip_hadb_find_by_srchit_destip_port(hip_hit_t *src_hit,
> + const struct in6_addr *dst_ip,
> + in_port_t dst_port)

src_hit can be const.

> +{
> + LHASH_NODE *item, *aux;
> + struct hip_hadb_state *tmp;
> + int i;
> +
> + list_for_each_safe(item, aux, hadb_hit, i)
> + {
> + tmp = list_entry(item);
> + if (ipv6_addr_cmp(&tmp->hit_our, src_hit)) {
> + continue;
> + } else if (!ipv6_addr_cmp(&tmp->peer_addr, dst_ip)) {
> + if (tmp->peer_udp_port == dst_port) {
> + return tmp;
> + } else {
> + continue;
> + }
> + } else {
> + continue;
> + }

Would the following do the trick?

list_for_each_safe(item, aux, hadb_hit, i)
{
     tmp = list_entry(item);

     const bool ok = !ipv6_addr_cmp(&tmp->hit_our, src_hit) &&
                     !ipv6_addr_cmp(&tmp->peer_addr, dst_ip) &&
                      tmp->peer_udp_port == dst_port;
     if(ok) {
         return tmp;
     }
}

Extra bool indirection because our policy of keeping the opening brace
on the same line as the closing paren makes multi-line conditions very
hard to comprehend IMHO, but feel free to inline if you're fine with it.

> === modified file 'hipd/hadb.h'
> --- hipd/hadb.h 2011-11-25 17:56:24 +0000
> +++ hipd/hadb.h 2012-01-08 17:54:25 +0000
> @@ -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.

> @@ -2424,6 +2433,111 @@
> }
>
> /**
> + * Handles the<tt> hipconf daemon trigger bex</tt> command.

Same as above

> + if (ai_err == 0) {
> + int done = 0;
> + const struct addrinfo *ai_res = addrinfo_result;
> +
> + while (done == 0&& ai_res != NULL) {
> + if (ai_res->ai_addr->sa_family == AF_INET) {
> + /* turn address into an IPv4-mapped IPv6 address
> + * (RFC 4291, 2.5.5.2.) */
> + 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?

> + ai_res = ai_res->ai_next;
> + }
> +
> + if (addrinfo_result != NULL) {
> + freeaddrinfo(addrinfo_result);
> + }

I'd free it even if ai_err wasn't 0, just to be sure.

> + 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; }