On 11/05/12 00:55, Diego Biurrun wrote: > review needs-fixing > > >> @@ -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? It is a short form for libhipl. This prefix can help to identify src code files related to libhipl. >> --- 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. strdup() is necessary before using dirname(), or you are suggesting other ways? can you clarify? >> +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? > > Because we don't use hipfw when using libhipl. libhipl also doesn't use kernel based ipsec. >> + 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. Ok, now I use a hip_msg_init() instead of calling free() and hip_msg_alloc(), so no need to allocate multiple times. Some HIP_IFEL is still necessary though, for memory deallocation. > >> +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 > I think I followed the guideline of HIP_IFEL, I use it here because I need to free memory at the end. > >> +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. I am not able to add more const qualifiers because this function calls hip_verify_network_header(), which will discard those const... > >> +{ >> + 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 Here the case is similar. I need to make sure I revert the nonblocking at the end of the function, just like free memory, so I use HIP_IFEL > > >> --- 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? I didn't find any... Thanks for the review. I will propose a new merge once I finish moving the hipl directory and valgrind check. Xin