review needs-fixing

On Sun, Jan 08, 2012 at 05:55:27PM +0000, Christoph Viethen wrote:
> Christoph Viethen has proposed merging lp:~cviethen/hipl/pisa-pairing-tng into lp:hipl.

Just a couple of nits, I don't know that part of the code well.

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

This sentence explains why starting a sentence with "this sentence" is
silly; same for "this function" in Doxygen function documentation.

> --- hipd/hadb.h	2011-11-25 17:56:24 +0000
> +++ hipd/hadb.h	2012-01-08 17:54:25 +0000
> @@ -115,6 +115,8 @@
>  
> +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);
> +

Break this long line.

> --- hipd/netdev.c	2011-12-30 23:20:44 +0000
> +++ hipd/netdev.c	2012-01-08 17:54:25 +0000
> @@ -1094,6 +1093,56 @@
>  
>  /**
> + * Gets called in case of an intended opportunistic base exchange - checks
> + * first whether a host association is already established, and won't perform
> + * the base exchange if so.
> + *
> + * If no host association exists, proceeds to pass everything through to
> + * hip_netdev_trigger_bex_msg().

Please start by explaining what the function does, not why and avoid the
third person singular.

> + * @param  msg  the message from hipconf
> + * @return zero on success and non-zero on error

It returns -1.

> +int hip_netdev_trigger_opp_bex_msg(const struct hip_common *const msg)
> +{
> +    hip_hit_t our_hit;
> +
> +    /* Find out: 1. our own hit, */
> +    if (hip_get_default_hit(&our_hit) < 0) {
> +        return -1;
> +    }
> +
> +    /* 2. the IP address the base exchange should be sent to, */
> +    const struct hip_tlv_common *const param = hip_get_param(msg, HIP_PARAM_IPV6_ADDR);
> +
> +    if (!param) {
> +        return -1;
> +    }

These two conditions could be merged.

> +    /* and 3. the UDP port number base exchanges will be sent to. */
> +    const in_port_t ha_peer_port = (hip_nat_status ? hip_get_peer_nat_udp_port() : 0);

pointless parentheses

> +    if (entry != NULL) {

if (entry) {

> --- lib/core/conf.c	2012-01-05 15:56:43 +0000
> +++ lib/core/conf.c	2012-01-08 17:54:25 +0000
> @@ -2424,6 +2433,111 @@
>  
>  /**
> + * Handles the <tt> hipconf daemon trigger bex </tt> command.

Handle ..

> + * @param msg       a pointer to the buffer where the message for hipd will
> + *                   be written.
> + * @param action    numeric action identifier (ACTION_TRIGGER is expected).
> + * @param opt       an array of pointers to the command line arguments after
> + *                   the action and type.

We don't generally use that extra space indentation.

> + * @param optc      the number of elements in the opt array (@b 1).
> + * @param send_only currently unused
> + * @return          zero on success, or -1 on error.
> + */
> +static int conf_handle_trigger_bex(struct hip_common *const msg,
> +                                                  const int action,
> +                                                const char *opt[],
> +                                                  const int optc,
> +                                                 UNUSED int send_only)

Ugh..

> +    if (action != ACTION_TRIGGER) {
> +        return -1;
> +    }
> +
> +    if (optc > 1) {
> +        HIP_ERROR("Too many arguments for trigger bex command.\n");
> +        return -1;
> +    }

Could be merged.

> +    if (ai_err == 0) {

maybe

  if (!ai_err) {

> +        while (done == 0 && ai_res != NULL) {

same here, maybe

  while (!done && ai_res) {

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

Drop the NULL check.

> +    const int uh_err = hip_build_user_hdr(msg, HIP_MSG_TRIGGER_OPP_BEX, 0);
> +
> +    if (uh_err) {
> +        HIP_ERROR("Failed to build user message header: %s\n", strerror(uh_err));
> +        return -1;
> +    }
> +
> +    if (hip_opportunistic_ipv6_to_hit(&ipv6_addr, &pseudo_hit, HIP_HIT_TYPE_HASH100)) {
> +        HIP_ERROR("hip_opportunistic_ipv6_to_hit() failed.\n");
> +        return -1;
> +    }
> +
> +    if (hip_build_param_contents(msg, &pseudo_hit, HIP_PARAM_HIT, sizeof(hip_hit_t))) {
> +        HIP_ERROR("Failed to build HIT parameter to hipconf message.\n");
> +        return -1;
> +    }
> +
> +    if (hip_build_param_contents(msg, &ipv6_addr, HIP_PARAM_IPV6_ADDR, sizeof(struct in6_addr))) {
> +        HIP_ERROR("Failed to build IP address parameter to hipconf message.\n");
> +        return -1;
> +    }

The variable indirection with uh_err looks inconsistent to me, you don't
do it in the other cases.

Diego