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.