> /** > + * Read a control message over TCP socket and save it to @c > hip_packet_context > + * for future processing. Ideally, this would also describe at least the most important checks that this code performs, how it handles socket errors, etc. > +int hip_read_control_msg_tcp(int sockfd, struct hip_packet_context *const > ctx) const sockfd > + is_ipv4 = (src_addr.sa_family == AF_INET); > + if (is_ipv4) { > + saddr4 = (struct sockaddr_in *) &src_addr; > + IPV4_TO_IPV6_MAP(&saddr4->sin_addr, &ctx->src_addr); > + ctx->msg_ports.src_port = ntohs(saddr4->sin_port); > + } else { > + saddr6 = (struct sockaddr_in6 *) &src_addr; > + memcpy(&ctx->dst_addr, &saddr6->sin6_addr, sizeof(struct in6_addr)); a plain assignment instead of memcpy is easier to read in such cases, but that is a matter of taste. > + ctx->msg_ports.src_port = ntohs(saddr6->sin6_port); > + } Some of the socket_wrapper functions fit in very well here. > + HIP_DEBUG_IN6ADDR("src", &ctx->src_addr); > + HIP_DEBUG_IN6ADDR("dst", &ctx->dst_addr); Maybe more information than that? Such as 'received packet from TCP socket, src..., dst...'? > +static int hipd_library_mode = 0; Why not a boolean type? > === added file 'lib/hipdaemon/socket_wrapper.c' > --- lib/hipdaemon/socket_wrapper.c 1970-01-01 00:00:00 +0000 > +++ lib/hipdaemon/socket_wrapper.c 2012-02-26 19:08:21 +0000 It would be extremely good to have more documentation in this file. Why is the struct hipl_fd_info necessary, for example? I don't mean what information in contains - that is obvious - but what is its purpose? Why was it necessary? All of the functions in this file need to be updated with stricter const correctness. > +static struct hip_ll socket_list; What kind of socket is stored in this list? It's struct hipl_fd_info objects, right? Are there many such objects? If not it may be simpler to just use a fixed size array than a dynamic list that tends to be awkward to use (==read). > +static struct in6_addr default_hit; > +/** > + * Get information on a HIP socket by its file descritor. descri[p]tor Please use an editor with a spell checker and respect its suggestions :) > +/** > + * Add peer's hit-to-addr mapping to hadb. > + * @param peer_hit peer's hit > + * @param peer_addr peer's addr, v4 addr should be mapped. > + * @return 0 on success, -1 otherwise. > + */ > +int hipl_add_peer_info(const hip_hit_t *peer_hit, > + const struct in6_addr *peer_addr) > +{ > + return hip_hadb_add_peer_info(peer_hit, peer_addr, NULL, NULL); > +} What's the point? Why not call hip_hadb_add_peer_info() in the first place? More to come Stefan -- https://code.launchpad.net/~hipl-core/hipl/libhip/+merge/93782 Your team HIPL core team is subscribed to branch lp:hipl.