[hipl-dev] Re: [Merge] lp:~hipl-core/hipl/libhip into lp:hipl

  • From: Christof Mroz <christof.mroz@xxxxxxxxxxxxxx>
  • To: mp+97589@xxxxxxxxxxxxxxxxxx
  • Date: Thu, 15 Mar 2012 11:41:17 -0000

Review: Needs Information

> === modified file 'doc/HOWTO.xml.in'
> --- doc/HOWTO.xml.in  2012-01-25 10:44:48 +0000
> +++ doc/HOWTO.xml.in  2012-03-15 08:58:36 +0000
> @@ -2584,6 +2658,68 @@
> +    <para>
> +      Libhipl API set is mainly located in
> +      <emphasis>"socket_wrapper.h"</emphasis>. The initialization function:
> +      <emphasis>libhipd_init(void)</emphasis> is declared in
> +      <emphasis>"init.h"</emphasis>
> +    </para>

A <code> tag looks more appropriate than <emphasis> (also in other places 
throught the file)
http://www.docbook.org/tdg/en/html/code.html

> === added file 'lib/hipl/socket_wrapper.c'
> --- lib/hipl/socket_wrapper.c 1970-01-01 00:00:00 +0000
> +++ lib/hipl/socket_wrapper.c 2012-03-15 08:58:36 +0000
> @@ -0,0 +1,827 @@
> +    do {
> +        if (fd_info->proto == IPPROTO_TCP) {
> +            if (!hip_read_control_msg_tcp(fd, &ctx)) {
> +                hip_receive_control_packet(&ctx);
> +            }
> +        } else if (fd_info->family == AF_INET) {
> +            if (!hip_read_control_msg_v4(fd, &ctx, HIP_UDP_ZERO_BYTES_LEN)) {
> +                hip_receive_control_packet(&ctx);
> +            }
> +        } else {
> +            if (!hip_read_control_msg_v6(fd, &ctx, HIP_UDP_ZERO_BYTES_LEN)) {
> +                hip_receive_control_packet(&ctx);
> +            }
> +        }
> +    } while (fd_info->ha->state != HIP_STATE_ESTABLISHED);

Looks like a lot of this dispatching throughout the patch could be avoided by 
introducing a generic hip_read_control_msg() function that takes an extra 
fd_info parameter and runs a switch() through it.
Sorry if this has been mentioned before, but I couldn't find it in the archives 
right away.

> +    if (domain == AF_INET) {
> +        setsockopt(sock, SOL_SOCKET, SO_BROADCAST, &on, sizeof(on));
> +        setsockopt(sock, IPPROTO_IP, IP_PKTINFO, &on, sizeof(on));
> +        setsockopt(sock, IPPROTO_IP, IP_RECVERR, &off, sizeof(off));
> +        setsockopt(sock, SOL_SOCKET, SO_REUSEADDR, &on, sizeof(on));
> +    } else {
> +        setsockopt(sock, IPPROTO_IPV6, IPV6_RECVERR, &off, sizeof(off));
> +        setsockopt(sock, IPPROTO_IPV6, IPV6_2292PKTINFO, &on, sizeof(on));
> +        setsockopt(sock, SOL_SOCKET, SO_REUSEADDR, &on, sizeof(on));
> +    }

Since this is a library function (i.e., "user-friendly"), this should error out 
if neither AF_INET nor AF_INET6 are given.

> +        /* Drop the packet if it doesn't come from the address associated
> +         * with the correct peer. */
> +        if (fd_info->proto == IPPROTO_UDP) {
> +            if (addr->sa_family == AF_INET) {

Since this is a library function, you should check that addr is indeed non-NULL 
for UDP.
But better yet: don't use the caller's buffer in the first place but provide 
your own.

> +                peer_addr4 = &((struct sockaddr_in *) addr)->sin_addr;
> +                IPV4_TO_IPV6_MAP(peer_addr4, &peer_addr);
> +                peer_addr6 = &peer_addr;
> +            } else {

peer_addr is probably a useless indirection.

> === added file 'lib/hipl/socket_wrapper.h'
> --- lib/hipl/socket_wrapper.h 1970-01-01 00:00:00 +0000
> +++ lib/hipl/socket_wrapper.h 2012-03-15 08:58:36 +0000
> @@ -0,0 +1,52 @@
> +int hipl_add_peer_info(const hip_hit_t *const peer_hit,
> +                       const struct in6_addr *const peer_addr);

Would a corresponding remove function make sense?
-- 
https://code.launchpad.net/~hipl-core/hipl/libhip/+merge/97589
Your team HIPL core team is requested to review the proposed merge of 
lp:~hipl-core/hipl/libhip into lp:hipl.

Other related posts: