Hi Deigo, Thanks for the detail review. I will fix them as soon as possible :) Best regards, Xin Gu On 06/02/12 19:44, Diego Biurrun wrote:
review needs-fixing On Fri, Feb 03, 2012 at 10:58:21AM +0000, Xin wrote:Libhip merge proposal: The libhip branch mainly aims to provide a convenient way for system test of HIPL without full installation and configuration. In addition to this purpose, it also provides a socket API alike library approach for application to use HIP. Last by not least, since vanilla Linux TCP does not yet support long periods of disconnectivity [1] , libhip can be a solution in this condition. In the libhip branch, we build a library version of HIP for upper applications, which only exposes traditional socket like API. when using the libhip, hip control messages are transmitted over TCP or UDP, which is similar to TLS/DTLS but we have a unified protocol to handle both datagram and streaming traffic[2]. Compared to TLS/DTLS, this is a big advantage and it may be better use case for HIP[3]. In the libhip, most of code of hipd has been moved to lib/hipdaemon which then becomes a library for both hipd and libhip. By this way, the libhip can reuse the code of hipd to the max extend. This is also the reason why libhip can be a system test approach for hipd, especially in the process of base exchange, the libhip and hipd share the same code base. Meanwhile, the hip daemon, and other functionalities previous exist in the trunk, are kept unchanged and functioning after this merge. We also implement a sample application called hipnetcat, which is a client-server style application on top of libhip and these 2 sides establish HIP association via BEX before actual data communication. The hipnetcat is integrated into our automatically test framework (check_hipnetcat). In check_hipnetcat, 2 hipnetcat processes try to establish BEX on loopback address on top of TCP/UDP, therefore, the execution of this test suite will check the functionality of base exchange in a system level.Hmmmmmmmmmmmmmmmmmmmmmmm, I'm sceptical. Much of this sounds as if it were not necessarily tied to librarizing hipd. For example, the system level test could be done without it. Or maybe I don't yet understand well enough. Below are some quick comments on the implementation. Look out for similar stuff in the other code you add. There is some work ahead before this is fit for merging.--- Makefile.am 2012-01-30 12:28:31 +0000 +++ Makefile.am 2012-02-03 10:57:21 +0000 @@ -72,12 +73,15 @@ test/check_hipfw \ test/check_lib_core \ test/check_lib_tool \ - test/check_modules_midauth + test/check_modules_midauth \ + test/check_hipnetcat + check_PROGRAMS = test/check_hipd \ test/check_hipfw \ test/check_lib_core \ test/check_lib_tool \ - test/check_modules_midauth + test/check_modules_midauth \ + test/check_hipnetcat endifThat stuff used to be in alphabetical order. There's more stuff I could complain about in this file, but there's no point in reviewing at this level yet I think.--- hipd/hipd.c 2011-11-25 16:40:40 +0000 +++ hipd/hipd.c 2012-02-03 10:57:21 +0000 @@ -132,23 +84,8 @@ +/* Create /etc/hip stuff and exit (used for binary hipfw packaging) */ +int create_configs_and_exit = 0;This looks like a leftover from a botched merge.--- lib/core/message.c 2011-11-08 15:25:41 +0000 +++ lib/core/message.c 2012-02-03 10:57:21 +0000 @@ -685,6 +685,74 @@ +int hip_read_control_msg_tcp(int sockfd, struct hip_packet_context *ctx) +{ + int err = 0; + int len; + int is_ipv4; + struct sockaddr src_addr, dst_addr; + struct sockaddr_in *saddr4; + struct sockaddr_in6 *saddr6; + socklen_t saddr_len = sizeof(struct sockaddr); + + hip_msg_init(ctx->input_msg); + + len = recv(sockfd, ctx->input_msg, HIP_MAX_PACKET, 0); + if (len< 0) { + HIP_PERROR("recvfrom(): "); + err = -1; + goto out_err; + } + + /* Get peer address */ + memset(&src_addr, 0, sizeof(src_addr));Initialize the variables to zero at declaration.+ /* Get local (bound) address */ + memset(&dst_addr, 0, sizeof dst_addr);This compiles?+ memmove(ctx->input_msg, + ((char *) ctx->input_msg) + HIP_UDP_ZERO_BYTES_LEN, + HIP_MAX_PACKET - HIP_UDP_ZERO_BYTES_LEN); + len -= HIP_UDP_ZERO_BYTES_LEN; + + HIP_IFEL(hip_verify_network_header(ctx->input_msg,&src_addr,&dst_addr, + len), + -1, "verifying network header failed\n"); + +out_err: + return err;You are fiercely abusing goto in this function - just return.--- hipd/init.c 2012-01-18 21:21:26 +0000 +++ lib/hipdaemon/init.c 2012-02-03 10:57:21 +0000 @@ -118,6 +120,8 @@ +#define HIP_USER_DIR ".hip/"HIP is the protocol of which there are other implementations.@@ -523,6 +527,210 @@ +static int libhip_init_handle_functions(void) +{ + int err = 0; + + return err; +}That variable is never used.@@ -866,7 +1074,7 @@ * @param signum signal the signal hipd received from OS */ -static void hip_close(int signum) +static void hipd_close(int signum) { @@ -928,8 +1136,8 @@ /* Register signal handlers */ - signal(SIGINT, hip_close); - signal(SIGTERM, hip_close); + signal(SIGINT, hipd_close); + signal(SIGTERM, hipd_close); signal(SIGCHLD, sig_chld);Push that separately.@@ -1090,6 +1298,71 @@ return err; } +int libhipd_init(void) +{ + +out_err: + if (msg) { + free(msg); + } + if (key_path) { + free(key_path); + }pointless NULL checks--- lib/hipdaemon/socket_wrapper.c 1970-01-01 00:00:00 +0000 +++ lib/hipdaemon/socket_wrapper.c 2012-02-03 10:57:21 +0000 @@ -0,0 +1,795 @@ +/** + * @file + * + * Copyright (c) 2010 Aalto University) and RWTH Aachen University. + *This should not be a Doxygen comment block.+#define _BSD_SOURCE + +#include<string.h> +#include<unistd.h> + +#include "close.h" +#include "hadb.h" +#include "hidb.h" +#include "init.h" +#include "input.h" +#include "netdev.h" +#include "output.h" +#include "socket_wrapper.h" + +#include "lib/core/builder.h" +#include "lib/core/conf.h" +#include "lib/core/crypto.h" +#include "lib/core/hip_udp.h" +#include "lib/core/linkedlist.h" +#include "lib/core/message.h" +#include "lib/core/prefix.h"Put the lib includes before the ones from the same directory.+struct hip_fd_info { + int fd; + int bound_port; + int family; + int proto; + struct hip_hadb_state *ha; +}; + +static struct hip_ll socket_list; +static struct in6_addr default_hit; + +static int auto_bind(const struct hip_fd_info *info); +static struct hip_fd_info *hip_socket_get_info(int fd); +static void set_hip_connection_parameters(int sock_fd, int local_port, + int remote_port); +static uint16_t get_port_from_saddr(const struct sockaddr *addr); +static void build_sockaddr(struct in6_addr *const addr, const uint16_t port, + struct sockaddr_storage *const ss); +static struct hip_fd_info *create_new_fd_info(const int fd, + const uint16_t bound_port, + const int family, const int proto);Reorder the functions so that you don't need forward declarations.+static int hip_is_control_msg(char *buf, unsigned int len, + struct hip_fd_info *fd_info) +{ + char udp_pad[HIP_UDP_ZERO_BYTES_LEN]; + struct hip_common *msg; + struct sockaddr_storage src, dst; + + if (len< sizeof(struct hip_common)) { + return 0; + } + + memset(udp_pad, 0, sizeof(udp_pad)); + + memset(&src, 0, sizeof(src)); + memset(&dst, 0, sizeof(dst));Same as above; initialize instead of memsetting, more below...+out_err: + if (ctx.input_msg) { + free(ctx.input_msg); + } + if (ctx.output_msg) { + free(ctx.output_msg); + } + return err;more pointless NULL checks, more below ...+int hip_bind(int fd, const struct sockaddr *address, socklen_t address_len) +{ + int err = 0; + struct hip_fd_info *fd_info; + struct sockaddr_storage laddr; + socklen_t laddr_len = sizeof(laddr); + uint16_t request_port; + + fd_info = hip_socket_get_info(fd); + HIP_IFEL(!fd_info, -1, "Fd %d is not a hip socket, exiting.\n", fd); + + request_port = get_port_from_saddr(address); + if ((err = bind(fd, address, address_len)) == 0) { + if (request_port == 0) { + HIP_IFEL(getsockname(fd, (struct sockaddr *)&laddr,&laddr_len), + -1, "getsockname() failed\n"); + request_port = get_port_from_saddr((struct sockaddr *)&laddr); + } + fd_info->bound_port = ntohs(request_port); + } else { + HIP_PERROR("bind error:"); + } + + HIP_DEBUG("bind to port %d\n", fd_info->bound_port); + +out_err: + return err; +}HIP_IFEL abuse, more below--- lib/hipdaemon/socket_wrapper.h 1970-01-01 00:00:00 +0000 +++ lib/hipdaemon/socket_wrapper.h 2012-02-03 10:57:21 +0000 @@ -0,0 +1,43 @@ +/** + * @file + * + * Copyright (c) 2010 Aalto University) and RWTH Aachen University.see above+#include<sys/socket.h> + +void hip_init_socket_wrapper(void); +int add_peer_info(const hip_hit_t *peer_hit, + const struct in6_addr *peer_addr); + +int hip_socket(int domain, int type, int protocol); +int hip_close(int fd); +int hip_bind(int fd, const struct sockaddr *address, socklen_t address_len); +int hip_sendto(int fd, const void *msg, size_t len, + int flags, const struct sockaddr *dst_hit, + socklen_t dst_len); +int hip_recvfrom(int fd, void *buf, size_t len, int flags, + struct sockaddr *addr, socklen_t *addr_len); +int hip_connect(int fd, const struct sockaddr *addr, socklen_t addr_len); +int hip_accept(int fd, struct sockaddr *addr, socklen_t *addr_len);This is missing some #includes to compile standalone, try 'make checkheaders'. It's also missing multiple inclusion guards.=== added file 'modules/heartbeat/Makefile.am.THIS' --- modules/heartbeat/Makefile.am.THIS 1970-01-01 00:00:00 +0000 +++ modules/heartbeat/Makefile.am.THIS 2012-02-03 10:57:21 +0000 @@ -0,0 +1,5 @@ +lib_LTLIBRARIES += modules/heartbeat/hipd/libhipheartbeat.la + +modules_heartbeat_hipd_libhipheartbeat_la_SOURCES = modules/heartbeat/hipd/heartbeat.c + +lib_hipdaemon_libhipdaemon_la_LIBADD += modules/heartbeat/hipd/libhipheartbeat.laDude, you wanna work on your conflict resolution ;)--- test/check_hipnetcat.c 1970-01-01 00:00:00 +0000 +++ test/check_hipnetcat.c 2012-02-03 10:57:21 +0000 @@ -0,0 +1,203 @@ +/* + * Copyright (c) 2010-2011 Aalto University and RWTH Aachen University.2012?--- test/hipnetcat.c 1970-01-01 00:00:00 +0000 +++ test/hipnetcat.c 2012-02-03 10:57:21 +0000 @@ -0,0 +1,366 @@ +/** + * @file + * + * Copyright (c) 2010 Aalto University) and RWTH Aachen University.... There's HIP_IFEL abuse and pointless memsetting galore here. Diego