review needs-fixing On Tue, Apr 17, 2012 at 04:10:25PM +0000, Xin wrote: > Xin has proposed merging lp:~hipl-core/hipl/libhip into lp:hipl. > > 3. The merge size is reduced. Since we get the single-process test cases, we > decide to postpone the hipnetcat and its test cases to another merge > proposal. > I also improve the code to reduce the code size. \o/ > --- Makefile.am 2012-03-13 15:27:06 +0000 > +++ Makefile.am 2012-04-17 16:09:27 +0000 > @@ -58,17 +58,21 @@ > > ### libraries ### > -lib_LTLIBRARIES = lib/core/libhipcore.la > +lib_LTLIBRARIES = lib/core/libhipcore.la \ > + lib/hipl/libhipl.la I still don't like the general directory layout we are designing. What about just core/ libhipl/ We do not place hipd and hipfw below a binaries directory, so we might as well have the libraries at the top level. > @@ -202,6 +166,47 @@ > > +lib_hipl_libhipl_la_SOURCES = lib/hipl/accessor.c \ > + lib/hipl/lhipl.c \ > + lib/hipl/lhipl_sock.c \ > + lib/hipl/lhipl_operations.c \ Why this lhipl prefix? > --- doc/HOWTO.xml.in 2012-02-28 18:38:18 +0000 > +++ doc/HOWTO.xml.in 2012-04-17 16:09:27 +0000 > @@ -2659,6 +2659,112 @@ > + > + <para> > + Libhipl provides HIP functionality as a library for upper layer > + applications and it does not require the presence of the HIPL daemon. without requiring the HIPL daemon. > + Instead, the HIP control messages are transmitted on top of TCP/UDP. s/the// > + <para> > + Libhipl API set is located in <emphasis>"lhipl.h"</emphasis>. Refer to > + its API documentation for detail information of each function. The libhipl API is described in <emphasis>"lhipl.h"</emphasis>. Detailed information about each function is available there. > + <formalpara> > + <title>Name-based API</title> > + <para> > + Compared to standard socket API, one apparent difference in Libhipl is > + that a peer is specified by given a peer name and a port number instead s/given/giving/ > + of a sockaddr data structure. The peer name is a string representation > of > + the peer. Currently, Libhipl only support HIT string as peer name. supports a In other places you lowercase "libhipl", so be consistent here. > + The function hipl_socket() creates a new libhipl socket. An ID is > + assigned to each created libhipl socket and returned by this function, > + then caller can pass this ID to other libhipl functions to operates on then the caller, operate > + this libhipl socket. NOTE: there is NO relation between libhipl socket > + ID and normal socket file descriptor, and the result of using IDs, descriptors > + <formalpara><title>Setup TCP connection</title><para> > + When using libhipl in TCP mode, TCP connection setup is the first task. > + similar to the normal socket syntax, libhipl provides: Similar > + <formalpara> > + <title>libhipl Base Exchange (BEX) and user data transmission</title> > + <para> > + Data transmission is handled by <emphasis>hipl_sendto()</emphasis> and > + <emphasis>hipl_recvfrom()</emphasis>. > + In libhipl, The first sending data call triggers BEX and the first > + receiving call waits for incoming BEX message. Once BEX succeeds, for an > + the pending user data is transmitted to the peer side. This BEX process > + is handled by libhipl automatically by default. Meanwhile, we also > + provide an advanced way to handle BEX step by step, which we will > + explain bellow. > + </para> You suddenly speak of "we" when above you spoke in impersonal form. Keep impersonal form. > + <emphasis>hipl_add_peer_info()</emphasis> adds peer's HIT and > IP the peer's > + address mappings to the database of libhipl. The library can > use > + this information to look up peer's locator when a peer name is a peer's > + <emphasis>hipl_lib_set_bex_feedback()</emphasis>: This function > + turns on/off the step-by-step process of BEX. By default, BEX turns the step-by-step BEX process on/off. > --- lib/core/hostid.c 2012-03-01 14:26:43 +0000 > +++ lib/core/hostid.c 2012-04-17 16:09:27 +0000 > @@ -743,11 +745,16 @@ > goto out_err; > } > } else if (!use_default) { > + hi_file_dup = strdup(hi_file); > + if ((err = check_and_create_dir(dirname(hi_file_dup), > HIP_DIR_MODE))) { > + HIP_ERROR("Could not create directory for path: %s\n", > hi_file); > + goto out_err; > + } > @@ -1093,6 +1100,7 @@ > > + free(hi_file_dup); > free(dsa_host_id); > free(dsa_pub_host_id); > free(rsa_host_id); This string duplication is not necessary AFAICT. > --- hipd/init.c 2012-04-16 14:47:42 +0000 > +++ lib/hipl/init.c 2012-04-17 16:09:27 +0000 > @@ -118,6 +120,34 @@ > > +#define HIPL_USER_DIR ".hipl" > +#define HIPL_USER_RSA_KEY_NAME "hipl_lib_rsa_key" Why "hipl_lib" and not "libhipl"? > @@ -1090,6 +1313,80 @@ > > /** > + * Initialization function for libhipl. > + * > + * @param debug_level debug level for log output. > + * @return 0 on success, negative number on errors. error > +int hipl_lib_init(enum logdebug debug_level) > +{ > + int err = 0; > + int keypath_len = 0; > + char *key_path = NULL; > + struct hip_common *msg = NULL; > + struct passwd *pwd; > + > + hipl_set_libhip_mode(); > + hip_nat_status = 1; > + > + /* disable hip_firewall */ > + lsi_status = HIP_MSG_LSI_OFF; Why are you disabling the firewall? > + /* set the initial verbosity level */ > + hip_set_logdebug(debug_level); The comment is rather redundant. > + HIP_IFEL(snprintf(key_path, keypath_len, "%s/%s/%s%s", pwd->pw_dir, > + HIPL_USER_DIR, > + HIPL_USER_RSA_KEY_NAME, > + DEFAULT_PUB_HI_FILE_NAME_SUFFIX) < 0, > + -1, "snprintf() failed"); > + > + HIP_DEBUG("Using key: %s\n", key_path); > + HIP_IFEL(!(msg = hip_msg_alloc()), -ENOMEM, "hip_msg_alloc()"); > + if (hip_serialize_host_id_action(msg, ACTION_ADD, 0, 0, "rsa", > + key_path, 0, 0, 0)) { > + free(msg); > + HIP_IFEL(!(msg = hip_msg_alloc()), -ENOMEM, "hip_msg_alloc()"); > + HIP_IFEL(hip_serialize_host_id_action(msg, ACTION_NEW, 0, 0, "rsa", > + key_path, RSA_KEY_DEFAULT_BITS, > + DSA_KEY_DEFAULT_BITS, > + ECDSA_DEFAULT_CURVE), -1, > + "Fail to create local key at %s.", key_path); > + free(msg); > + HIP_IFE(!(msg = hip_msg_alloc()), -ENOMEM); > + HIP_IFEL(hip_serialize_host_id_action(msg, ACTION_ADD, 0, 0, "rsa", > + key_path, 0, 0, 0), -1, > + "Fail to load local key at %s.", key_path); > + } > + HIP_IFE(hip_handle_add_local_hi(msg), -1); Oh, the HIP_IFEL ugliness. Please remind me why you allocate msg multiple times. > --- hipd/init.h 2011-11-25 17:56:24 +0000 > +++ lib/hipl/init.h 2012-04-17 16:09:27 +0000 > @@ -60,4 +62,6 @@ > int is_output); > void hip_exit(void); > > +int hipl_lib_init(enum logdebug); > + > #endif /* HIPL_HIPD_INIT_H */ I'm only commenting here, but this applies to all header files you moved: All the multiple inclusion guards are now inconsistent; we use the path as multiple inclusion guard. > --- lib/hipl/lhipl.c 1970-01-01 00:00:00 +0000 > +++ lib/hipl/lhipl.c 2012-04-17 16:09:27 +0000 > @@ -0,0 +1,536 @@ > + > +/** > + * @file > + * This file contains the implementation of the libhipl public APIs. > + */ This sentence explains why starting a sentence with "This sentence" is silly and awkward. Same for "this file"... > +/* A switch to turn off/on BEX feedback. Move "off/on" to the end of the sentence, more below. > +/** > + * build a @c sockaddr_in6 address to store peer's HIT and port number from a > + * string-based peer name and a port number. > + * @param peername the string representation of a peer, only support HIT > string > + * currently. > + * @param port the port of the peer. > + * @param peer the @c sockaddr_in6 to hold peer's HIT and port number. > + * @return 0 on success, -1 on error. These are > +/** > + * Close a socket. > + * > + * Send HIP CLOSE message to the associated peer and deletes the libhipl delete > +int hipl_sendto(const hipl_sock_id hsock_id, const void *const msg, > + const size_t len, const int flags, > + const char *const peername, uint16_t port) > +{ > + > + if (hipl_lib_bex_feedback()) { > + err = hipl_sendmsg_internal(hsock, ¶ms, flags); > + } else { > + fd_set fdset; > + if (hipl_hsock_ha_state(hsock) == HIP_STATE_UNASSOCIATED) { > + HIP_DEBUG("Sending via hsock %d, Triggering BEX.\n", hsock->sid); > + err = hipl_sendmsg_internal(hsock, ¶ms, flags); > + HIP_IFEL(err != -EWAITBEX, -1, "hipl_sendmsg_internal() > failed\n"); > + } > + if (hipl_hsock_ha_state(hsock) == HIP_STATE_ESTABLISHED) { > + HIP_DEBUG("Sending via hsock %d, HA established.\n", hsock->sid); > + err = hipl_sendmsg_internal(hsock, ¶ms, flags); > + } else { > + while (hipl_hsock_ha_state(hsock) != HIP_STATE_ESTABLISHED) { > + FD_ZERO(&fdset); > + FD_SET(hsock->sock_fd, &fdset); > + HIP_DEBUG("Sending via hsock %d, Waiting BEX.\n", > hsock->sid); > + err = select(hsock->sock_fd + 1, &fdset, NULL, NULL, NULL); > + HIP_IFEL(err < 0, -1, "select(): %s\n", strerror(errno)); > + err = hipl_sendmsg_internal(hsock, ¶ms, flags); > + HIP_IFEL(err < 0 && err != -EWAITBEX && err != > -EBEXESTABLISHED, > + -1, "hipl_sendmsg_internal() failed\n"); > + } > + err = hipl_sendmsg_internal(hsock, ¶ms, flags); > + } > + } > + > +out_err: > + free(buf); > + return err; > +} HIP_IFEL ugliness > --- lib/hipl/lhipl.h 1970-01-01 00:00:00 +0000 > +++ lib/hipl/lhipl.h 2012-04-17 16:09:27 +0000 > @@ -0,0 +1,76 @@ > + > --- lib/hipl/lhipl_operations.c 1970-01-01 00:00:00 +0000 > +++ lib/hipl/lhipl_operations.c 2012-04-17 16:09:27 +0000 > @@ -0,0 +1,771 @@ > + > +/** > + * @file > + * This file contains the internal implementation of the libhipl socket > + * related operations. This sentence explains why starting a sentence with "this sentence" is awkward; same for "this file". > +static int hipl_is_control_msg(char *const buf, unsigned int len, > + struct hipl_sock *const hsock) > +{ > + char udp_pad[HIP_UDP_ZERO_BYTES_LEN] = { 0 }; > + struct hip_common *msg; > + struct sockaddr_storage src = { 0 }; > + socklen_t srclen = sizeof(src); > + > + if (len < sizeof(struct hip_common)) { > + return 0; > + } > + > + if (!memcmp(udp_pad, buf, HIP_UDP_ZERO_BYTES_LEN)) { > + HIP_DEBUG("Message is padded\n"); > + msg = (struct hip_common *) (buf + HIP_UDP_ZERO_BYTES_LEN); > + len -= HIP_UDP_ZERO_BYTES_LEN; > + } else { > + msg = (struct hip_common *) buf; > + } > + > + if (getsockname(hsock->sock_fd, (struct sockaddr *) &src, &srclen) < 0) { > + HIP_PERROR("getsockname()"); > + return true; > + } > + > + return !hip_verify_network_header(msg, (struct sockaddr *) &src, > + (struct sockaddr *) > &hsock->peer_locator, > + len); > +} IIUC both pointer parameters can be even more const. > +static int recv_msg_wrapper(struct hipl_sock *const hsock, > + struct msghdr *const msg, const int flags, > + struct hip_packet_context *const ctx, > + bool *is_user_msg) > +{ > + > + char *buf = (char *) msg->msg_iov->iov_base; pointless void* cast > +static int nonb_result_check(const int ret, const int err) nonb? > +/** > + * Trigger BEX in a non-blocking way. > + * > + * @param hsock the libhipl socket to trigger BEX. > + * @param src_hit the source HIT for base exchange. > + * @param dst_hit the destination HIT for base exchange. > + * @param dst_port the destination port. > + * @return -1 on error, -EWAITBEX when the BEX is pending, 0 if > sending > + * BEX trigger message successfully. > + */ > +static int nonb_trigger_bex(struct hipl_sock *hsock, const hip_hit_t > *src_hit, > + const hip_hit_t *dst_hit, const int dst_port) So "nonb" is "nonblocking"? That is not obvious. Please type a few more characters. > +{ > + struct in6_addr dst_addr; > + int err = 0, flag; > + > + flag = fcntl(hsock->sock_fd, F_GETFL, 0); > + fcntl(hsock->sock_fd, F_SETFL, flag | O_NONBLOCK); > + > + err = hip_map_id_to_addr(dst_hit, NULL, &dst_addr); > + HIP_IFEL(err < 0, -1, "failed to match hit to IP\n"); > + HIP_IFEL(ipv6_addr_any(&dst_addr), -1, "Couldn't map HIT to IP\n"); > + > + set_hip_connection_parameters(hsock->sock_fd, hsock->src_port, dst_port); > + err = netdev_trigger_bex(src_hit, dst_hit, NULL, NULL, NULL, &dst_addr); > + HIP_DEBUG("netdev_trigger_bex returns %d, errno = %d\n", err, errno); > + err = nonb_result_check(err, errno); > + if (err == 0) { > + hsock->ha = hip_hadb_find_byhits(src_hit, dst_hit); > + } > + > +out_err: > + fcntl(hsock->sock_fd, F_SETFL, flag); > + return err; > +} HIP_IFEL ugliness. many more below > --- lib/hipl/lhipl_sock.c 1970-01-01 00:00:00 +0000 > +++ lib/hipl/lhipl_sock.c 2012-04-17 16:09:27 +0000 > @@ -0,0 +1,150 @@ > + > +/** > + * @file > + * This file contains functions for maintaining libhipl sockets. see above > --- lib/hipl/lhipl_sock.h 1970-01-01 00:00:00 +0000 > +++ lib/hipl/lhipl_sock.h 2012-04-17 16:09:27 +0000 > @@ -0,0 +1,67 @@ > + > +/* This struct contains internal information about each libhipl socket. another awkward comment > --- test/check_libhipl.c 1970-01-01 00:00:00 +0000 > +++ test/check_libhipl.c 2012-04-17 16:09:27 +0000 > @@ -0,0 +1,216 @@ > + > +#ifndef max > +#define max(a, b) (((a) > (b)) ? (a) : (b)) > +#endif Don't we have this somewhere already? > +int main(void) > +{ > + int number_failed; > + Suite *s = hipnc_suite(); > + SRunner *sr = srunner_create(NULL); > + > + srunner_add_suite(sr, s); > + srunner_run_all(sr, CK_NORMAL); > + > + number_failed = srunner_ntests_failed(sr); > + srunner_free(sr); > + > + return (number_failed == 0) ? EXIT_SUCCESS : EXIT_FAILURE; > +} Please run this test through valgrind and check for memory leaks. Diego