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

 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