[hipl-dev] Re: [Merge] lp:~hipl-core/hipl/libhip into lp:hipl
- From: Diego Biurrun <diego@xxxxxxxxxx>
- To: mp+92480@xxxxxxxxxxxxxxxxxx
- Date: Sun, 12 Feb 2012 15:04:40 -0000
Review: Disapprove
review disapprove
On Fri, Feb 10, 2012 at 02:24:28PM +0000, Xin wrote:
> Xin has proposed merging lp:~hipl-core/hipl/libhip into lp:hipl.
>
> Requested reviews:
> Diego Biurrun (diego-biurrun)
> Xin (eric-nevup)
> Miika Komu (miika-iki)
You are requesting review from yourself and not all of hipl-core?
> For more details, see:
> https://code.launchpad.net/~hipl-core/hipl/libhip/+merge/92480
>
> Feb 10, 2012 code revised based on Diego's review
>
> --- Makefile.am 2012-02-09 09:30:59 +0000
> +++ Makefile.am 2012-02-10 14:23:36 +0000
> @@ -64,17 +65,21 @@
>
> ### libraries ###
> lib_LTLIBRARIES = lib/core/libhipcore.la
> +lib_LTLIBRARIES += lib/hipdaemon/libhipdaemon.la
OK, time to start with the design review I guess: I don't like the
naming. We previously had this empty lib/ directory that only contains
subdirs and I previously discussed renaming it. Your merge request
makes this all the more urgent.
> @@ -83,58 +88,20 @@
>
> ### source declarations ###
>
> -test_auth_performance_SOURCES = test/auth_performance.c
> -test_certteststub_SOURCES = test/certteststub.c
> -test_dh_performance_SOURCES = test/dh_performance.c
> -test_fw_port_bindings_performance_SOURCES = hipfw/file_buffer.c \
> - hipfw/line_parser.c \
> - hipfw/port_bindings.c \
> -
> test/fw_port_bindings_performance.c
> -test_hc_performance_SOURCES = test/hc_performance.c
> +test_auth_performance_SOURCES = test/auth_performance.c
> +test_certteststub_SOURCES = test/certteststub.c
> +test_dh_performance_SOURCES = test/dh_performance.c
> +test_fw_port_bindings_performance_SOURCES = hipfw/file_buffer.c \
> + hipfw/line_parser.c \
> + hipfw/port_bindings.c \
> +
> test/fw_port_bindings_performance.c
> +test_hc_performance_SOURCES = test/hc_performance.c
> +test_hipnetcat_SOURCES = test/hipnetcat.c
This was better before. Please avoid senseless and unrelated cosmetic
changes.
> @@ -164,8 +131,7 @@
> hipfw/user_ipsec_api.c \
> hipfw/user_ipsec_esp.c \
> hipfw/user_ipsec_fw_msg.c \
> - hipfw/user_ipsec_sadb.c \
> - modules/midauth/lib/midauth_builder.c
> + hipfw/user_ipsec_sadb.c
This reminds me of this whole modularity nonsense - not related to your
merge request of course...
> @@ -244,21 +255,24 @@
>
> ### static library dependencies ###
>
> -hipd_hipd_LDADD = lib/core/libhipcore.la
> -hipfw_hipfw_LDADD = lib/core/libhipcore.la
> +hipd_hipd_LDADD = lib/hipdaemon/libhipdaemon.la
> +hipfw_hipfw_LDADD = lib/hipdaemon/libhipdaemon.la
Why does the firewall depend on libhipdaemon?
> --- doc/HOWTO.xml.in 2012-01-25 10:44:48 +0000
> +++ doc/HOWTO.xml.in 2012-02-10 14:23:36 +0000
> @@ -764,6 +764,58 @@
> </para>
> </section> <!-- handover -->
>
> + <section id="ch_hipnetcat">
> + <title>Test HIPL by the hipnetcat program</title>
s/by/with/
> + <para>
> + The hipnetcat can be used to test the base exchange functionality of
> the HIPL without
s/The //
> + installing the HIPL binary. This section explains its usage in detail.
HIPL is the name of the project, not of the binary.
> + <para>
> + The hipnetcat program takes similar parameters as the normal netcat
> program. In
> + order to check the functionality of the base exchange, a hipnetcat
> server and a
> + hipnetcat client are required.
> + </para>
Please keep lines below 80 characters where easily possible.
> + <para>
> + The usage of hipnetcat is shown as follow:
> + <programlisting>
> + Usage: hipnetcat [-hlt] [-p source_port] [-s source_ip_address]
> + [-d dest_port] [peer_identifier[s]]
> +
> + -h: help
> + -l: Listening mode, hip netcat acts as the server side.
> + -t: Using TCP as transportation protocol, otherwise UDP will be
> used.
> + </programlisting>
> + The peer_identifiers can be a list of combination of peer's HIT, IP
> address and
> + host name.
list of combination? can? What else "can" they be? You really lost me
here.
> + <para>
> + Example 1: localhost hipnetcat connection via TCP. The server listens
> on 127.0.0.1:22300
> + and the client connects from 127.0.0.1:22345 with HIT
> 2001:1c:809e:244a:c33:78fb:45e3:d132.
All of this could use some semantic markup. Look around in the file for
the tags used to designate options, etc.
> @@ -2583,7 +2635,79 @@
>
> </section>
> -
> + <section id="ch_libhip_usage">
The empty line was there on purpose.
> + <title>The Libhip Extension</title>
not libhipl?
> + <para>
> + The Libhip provides HIP functionality as a library for upper layer
> + applications and it does not require the present of normal hip
presence, HIP
Please check your spelling of hip/HIP/HIPL everywhere. The first is a
prefix, the second the name of the protocol, the third the name of the
project.
> + daemon. Instead, the HIP control messages are transmitted on top of
> + TCP/UDP. From the application points of view, they get similar API set
The application has multiple pointS of view?
> + <para>
> + The Libhip API set is listed as follow:
> + <programlisting>
> + <emphasis>"socket_wrapper.h"</emphasis>
> + 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);
> +
> + <emphasis>"init.h"</emphasis>
> + int libhipd_init(void);
> + </programlisting>
> + </para>
What's the point of repeating the header here? Just seems like an
invitation to update one, but not the other to me.
> + <para>
> + In <emphasis>hip_sendto</emphasis> and
> <emphasis>hip_connect</emphasis>,
> + the destination address should be a HIT, not an IP address.The
> destination
space after period
> + <listitem>
> + <para>
> + In <emphasis>hip_accept</emphasis>, after it accepts a new peer,
> the
it?
> + <emphasis>addr</emphasis> parameter returns a structure which
> container peer's
s/which/with/ ?
> + HIT and port (NOT IP). the <emphasis>addr_len</emphasis> parameter
> returns
Capitalize the start of sentences.
> + For more detail usage of the libhip, please refer to the hipnetcat
> program
For a more detailed libhip usage example, please refer
> --- lib/core/linkedlist.c 2011-08-15 14:11:56 +0000
> +++ lib/core/linkedlist.c 2012-02-10 14:23:36 +0000
> @@ -297,6 +297,31 @@
>
> +/*
> + * Deletes the first node in a list with the given element as its data.
> + * If there is no match, does nothing.
> + *
> + * @param linkedlist the list where from to remove the element.
list to remove an element from
> --- lib/core/message.c 2011-11-08 15:25:41 +0000
> +++ lib/core/message.c 2012-02-10 14:23:36 +0000
> @@ -686,6 +686,80 @@
>
> /**
> + * Read a control message over TCP socket.
> + * Used by libhip API.
Why do you have to mention this is libhip{l} API? I would expect that
API to be implemented by libhip{l}, not some random other place.
> --- hipd/esp_prot_hipd_msg.c 2011-12-16 13:37:33 +0000
> +++ lib/hipdaemon/esp_prot_hipd_msg.c 2012-02-10 14:23:36 +0000
> @@ -57,6 +57,10 @@
>
> +int esp_prot_active = 0;
> +int esp_prot_num_transforms = 0;
> +long esp_prot_num_parallel_hchains = 0;
This looks suspicious. Why are you moving these global variables, but
not the extern declarations?
Why is this part of this merge request and split off and done separately?
The same applies to all the other similar changes. I'm not convinced you
need to do them (here).
> --- hipd/hadb.c 2012-01-25 20:45:27 +0000
> +++ lib/hipdaemon/hadb.c 2012-02-10 14:23:36 +0000
> @@ -105,6 +105,23 @@
>
> +/* Flag to show if hipl is running in libhip mode (=1) or normal mode (=0).
> + * This variable should NOT be accessed directly. Always use the accessor
> + * functions instead.
> + */
> +static int hipd_library_mode = 0;
> +
> +int is_libhip_mode()
> +{
> + return hipd_library_mode;
> +}
> +
> +int set_libhip_mode()
> +{
> + hipd_library_mode = 1;
> + return 0;
> +}
Why in this file?
> --- hipd/init.c 2012-01-18 21:21:26 +0000
> +++ lib/hipdaemon/init.c 2012-02-10 14:23:36 +0000
> @@ -74,12 +75,14 @@
> #include "accessor.h"
> #include "close.h"
> #include "dh.h"
> +#include "esp_prot_hipd_msg.h"
> #include "esp_prot_light_update.h"
> #include "hadb.h"
> #include "hidb.h"
> #include "hip_socket.h"
> #include "hipd.h"
> #include "hiprelay.h"
> +#include "init.h"
> #include "input.h"
> #include "maintenance.h"
> #include "nat.h"
> @@ -88,9 +91,8 @@
> #include "output.h"
> #include "pkt_handling.h"
> #include "registration.h"
> +#include "socket_wrapper.h"
> #include "user.h"
> -#include "init.h"
> -#include "hipd/esp_prot_hipd_msg.h"
We always have the .h file that corresponds to a given .c file last.
> @@ -523,6 +527,210 @@
>
> +static int libhip_init_handle_functions(void)
> +{
> + int err = 0;
> +
> + return err;
> +}
I wonder what you need the variable for and why the function is not of
type void.
> @@ -866,7 +1074,7 @@
> */
> -static void hip_close(int signum)
> +static void hipd_close(int signum)
> {
> static int terminate = 0;
>
> @@ -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);
I repeat: Please push such changes separately.
If you do not understand a review comment or disagree, ask or speak up.
Just ignoring review comments can be construed as rude behavior.
> @@ -1090,6 +1298,68 @@
>
> +int libhipd_init(void)
> +{
> + set_libhip_mode();
> + hip_nat_status = 1;
> +#ifdef CONFIG_HIP_FIREWALL
> + hipfw_status = 0;
> +#endif
You never tested in a setup with the firewall enabled.
I'm getting more and more sceptical of this whole merge proposal.
It has obviously seen little to no testing, at least outside of
the very basic standard configuration that it worked in.
It seems that the previous iteration was never compiled. What sort of
testing did you do? Does this pass 'make alltests'? Does it pass the
autobuilder?
> + HIP_IFE(!(key_path = malloc(keypath_len)), -1);
HIP_IFE abuse and nondescript return value.
> --- hipd/output.c 2012-01-16 22:06:09 +0000
> +++ lib/hipdaemon/output.c 2012-02-10 14:23:36 +0000
> @@ -1252,7 +1272,6 @@
> if (dst_is_ipv4) {
> IPV6_TO_IPV4_MAP(peer_addr, &dst4->sin_addr);
> dst4->sin_family = AF_INET;
> -
> HIP_DEBUG_INADDR("dst4", &dst4->sin_addr);
> } else {
unrelated cosmetics
> --- lib/hipdaemon/socket_wrapper.c 1970-01-01 00:00:00 +0000
> +++ lib/hipdaemon/socket_wrapper.c 2012-02-10 14:23:36 +0000
> @@ -0,0 +1,812 @@
> +
> +/**
> + * @file
> + * This file contains implementation of libhip API.
> + *
> + */
pointless empty line
libhip makes no sense, it should be libhip_l_.
You are also missing some articles in that sentence, i.e.
This file contains the implementation of the libhipl API.
> +/**
> + * Build a suitable sockaddr_storage.
> + * If the addr is V4MAPPED, the storage family will be INET4.
Are you referring to "addr" the function parameter? Then there is
Doxygen markup for such cases, otherwise please do not abbreviate
words in prose as opposed to variable names.
> + * Otherwise the sotrage family will be INET6.
storage
You may consider running a spellchecker.
> + * @param addr a V6 addr or V4MAPPED addr
address, see above
> + * @param port port number (in NETWORK order)
The network (byte?) order comment confuses me. Why should the caller
have to worry about this?
> + * @param ss the sockaddr_storage to be filled.
> + */
> +static void build_sockaddr(struct in6_addr *const addr, const uint16_t port,
> + struct sockaddr_storage *const ss)
> +{
> +
> + if (IN6_IS_ADDR_V4MAPPED(addr)) {
> + struct sockaddr_in *const in = (struct sockaddr_in *) ss;
> + in->sin_family = AF_INET;
> + IPV6_TO_IPV4_MAP(addr, &in->sin_addr);
> + in->sin_port = port;
> + } else {
> + struct sockaddr_in6 *const in6 = (struct sockaddr_in6 *) ss;
> + in6->sin6_family = AF_INET6;
> + ipv6_addr_copy(&in6->sin6_addr, addr);
> + in6->sin6_port = port;
> + }
Why do you use variable indirection?
> +/**
> + * Create a new hip socket info struct and insert it into global list.
HIP?
> +static struct hip_fd_info *create_new_fd_info(const int fd,
> + const uint16_t bound_port,
> + const int family, const int
> proto)
> +{
> + int err = 0;
> + struct hip_fd_info *fd_info_new = NULL;
> +
> + fd_info_new = malloc(sizeof(struct hip_fd_info));
> + HIP_IFEL(!fd_info_new, -1, "malloc()\n");
This is HIP_IFEL abuse IMO, you can just return from here and do so with
a proper ENOMEM return value instead.
> + memset(fd_info_new, 0, sizeof(struct hip_fd_info));
calloc()
> +/**
> + * Get information on a socket by FD
about a
> +/**
> + * Initialization function for socket wrapper functionality
> + */
> +void hip_init_socket_wrapper(void)
> +{
> + hip_ll_init(&socket_list);
> + memset(&default_hit, 0, sizeof(default_hit));
Global variables are initialized to 0, so this seems pointless.
> +/**
> + * Add peer hit-addr mapping to hadb.
> + * @param peer_hit peer's hit
> + * @param peer_addr peer's addr, v4 addr should be mapped.
Vertical alignment would make this more readable.
> + * @return 0 if success, -1 otherwise.
s/if/on/
> + */
> +int add_peer_info(const hip_hit_t *peer_hit,
> + const struct in6_addr *peer_addr)
nit: Unbreak this line.
> +{
> +int 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);
> +}
Does this even compile?
I'm stopping my review here. Sorry, but I feel like I'm wasting precious
time on things that haven't seen even minimal testing.
> --- test/check_hipnetcat.c 1970-01-01 00:00:00 +0000
> +++ test/check_hipnetcat.c 2012-02-10 14:23:36 +0000
> @@ -0,0 +1,208 @@
> +
> +#include <arpa/inet.h>
> +#include <stdlib.h>
> +#include <stdio.h>
> +#include <sys/wait.h>
> +#include <sys/select.h>
> +#include <unistd.h>
> +#include <check.h>
nit: see above about include ordering
> +#include "lib/hipdaemon/hidb.h"
> +#include "lib/hipdaemon/init.h"
> +
> +#define TEST_HIPNC_TIMEOUT 10
> +
> +static Suite *hipnc_suite(void);
> +static void hipnc_test_start(char *serv_argv[], char *client_argv[]);
You should not require forward declarations.
> +START_TEST(test_hipnc_lo_udp)
> +{
> + char *serv_argv[16];
> + char *client_argv[16];
> + struct in6_addr local_hit = { { { 0 } } };
> + char hit_buf[128];
> + int idx = 0;
> +
> + /* init */
> + fail_if(libhipd_init(), "fail to init libhip");
> + fail_if(hip_get_default_hit(&local_hit),
> + "fail to load local hit for hipnetcat startup");
> + fail_if(inet_ntop(AF_INET6, &local_hit, hit_buf, 128) == NULL,
> + "fail to parse hit to string.");
failED, HIT
> + serv_argv[idx++] = (char *) 0;
Do you happen to mean NULL here?
> + client_argv[idx++] = (char *) 0;
ditto
same above
> + hipnc_test_start(serv_argv, client_argv);
> +}
> +END_TEST
> +
> +static void hipnc_test_start(char *serv_argv[], char *client_argv[])
> +{
> +
> + if (pid == 0) {
> + if (execv("test/hipnetcat", serv_argv)) {
The conditions can be combined, same below.
> + if (pid == 0) {
> + sleep(1);
What do you sleep()?
> + if (execv("test/hipnetcat", client_argv)) {
Does this work when building out-of-tree?
> + /* check server & client status */
> + for (i = 0; i < TEST_HIPNC_TIMEOUT; i++) {
> + tv.tv_sec = 1;
> + tv.tv_usec = 0;
> + select(0, NULL, NULL, NULL, &tv);
> + round = remain_cld;
> + for (j = 0; j < round; j++) {
> + pid = waitpid(-1, &status, WNOHANG);
> + fail_if(pid > 0 && status != 0,
> + "hipnetcat failed");
No need to break this line.
> --- test/hipnetcat.c 1970-01-01 00:00:00 +0000
> +++ test/hipnetcat.c 2012-02-10 14:23:36 +0000
> @@ -0,0 +1,370 @@
> +
> +/**
> + * @file
> + * The hipnetcat sample program using libhip.
> + *
> + */
pointless empty line
> +#define _BSD_SOURCE
> +
> +#include <arpa/inet.h>
> +#include <errno.h>
> +#include <netdb.h>
> +#include <netinet/in.h>
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <string.h>
> +#include <unistd.h>
nit: group arpa/inet.h and netinet/in.h together
> +static int dst_port = HIP_NAT_UDP_PORT;
> +static unsigned int hipnc_flag = HIPNC_UDP_MODE &
> ~HIPNC_SERVER_MODE & ~HIPNC_IP6;
> +static struct sockaddr_storage local_ss = { 0 };
> +static struct hip_ll hit_list = HIP_LL_INIT;
> +static struct hip_ll locator_list = HIP_LL_INIT;
uncrustify does not align the = here?
> +static int create_hip_socket(unsigned int flag)
> +{
> + return hip_socket((flag & HIPNC_IP6) ? AF_INET6 : AF_INET,
> + (flag & HIPNC_UDP_MODE) ? SOCK_DGRAM : SOCK_STREAM,
> + (flag & HIPNC_UDP_MODE) ? IPPROTO_UDP : IPPROTO_TCP);
You could align the ? and : here, dunno if uncrustify complains though.
> +static int hipnc_run_client(const unsigned int flag, struct sockaddr
> *local_addr,
long line, more below
> + fd = create_hip_socket(flag);
> + HIP_IFEL(fd < 0, -1, "Fail to create hip socket: %s\n", strerror(errno));
HIP_IFEL abuse
> + HIP_IFEL(hip_bind(fd, local_addr, addr_len), -1, "HIP bind error\n");
What is "HIP bind"?
> + if (!(flag & HIPNC_UDP_MODE)) {
> + HIP_IFEL(hip_connect(fd, (struct sockaddr *) peer_hit_saddr,
> + sizeof(struct sockaddr_in6)),
> + -1, "connect()\n");
> + }
> +
> + sprintf(buf, "Hello, sailor!");
:)
> +out_err:
> + if (fd > 0) {
> + hip_close(fd);
> + }
How can fd be <= 0 here?
> + fd = create_hip_socket(flag);
> + HIP_IFEL(fd < 0, -1, "Fail to create hip socket\n", strerror(errno));
FailED and this abuses HIP_IFEL.
> + HIP_IFEL(hip_bind(fd, local_addr, addr_len), -1, "HIP bind error\n");
see above
> +static void usage(void)
> +{
> + printf("HIP netcat program.\n"
> + "Usage: hipnetcat [-hlt] [-p source_port] [-s
> source_ip_address]\n"
> + " [-d dest_port] [peer_identifier[s]]\n\n"
> + " -h: help\n"
> + " -l: Listening mode, hip netcat acts as the server side.\n"
hip? HIP? Please be consistent.
> + " -t: Using TCP as transportation protocol, otherwise UDP will
> be used.\n");
use, transport
> + if (err <= 0) {
> + memset(ss, 0, sizeof(*ss));
> + ss->ss_family = AF_INET6;
> + sa6->sin6_port = htons(port);
> + hipnc_flag |= HIPNC_IP6;
> + err = inet_pton(AF_INET6, ip, &sa6->sin6_addr);
> + }
> +
> + return (err <= 0) ? -1 : 0;
You are checking the same condition a few lines above.
> + /* Parsing input arguments */
> + while ((opt = getopt(argc, argv, "hltp:d:s:")) != -1) {
Redundant comment is redundant.
> + /* parse identifiers */
> + while (optind < argc) {
Wait, what is "optind"?
> + arg = argv[optind++];
> + if (inet_pton(AF_INET, arg, (void *) &inaddr4)) {
> + /* if it is a V4 addr, map it to V6 and add it to locator_list */
> + HIP_DEBUG("%s is parsed to v4", arg);
> + sprintf(addr_buf, "::FFFF:");
> + strcat(addr_buf, arg);
> + inet_pton(AF_INET6, addr_buf, (void *) &inaddr6);
> + HIP_DEBUG(". Map to v6: %s\n", addr_buf);
> + paddr6 = (struct in6_addr *) malloc(sizeof(struct in6_addr));
malloc returns void*, which is compatible with any pointer type.
In C you don't have to cast this.
> + HIP_IFEL(!paddr6, -1, "malloc() failed\n");
HIP_IFEL abuse and nondescript return value
> + memcpy(paddr6, &inaddr6, sizeof(struct in6_addr));
> + hip_ll_add_last(&locator_list, (void *) paddr6);
Why do you need all these void* casts?
> +int main(int argc, char *argv[])
> +{
> + int err = 0;
> + struct sockaddr_in6 peer_hit_saddr = { 0 };
> +
> + /* Parsing input arguments */
> + if (parse_arguments(argc, argv)) {
Redundant comment is redundant.
> + /* libhip init */
> + if (libhipd_init() < 0) {
Redundant ... - oh wait, you know the drill...
> + /* if no errors, finish execution, otherwise we try another hit
> */
HIT
Diego
--
https://code.launchpad.net/~hipl-core/hipl/libhip/+merge/92480
Your team HIPL core team is subscribed to branch lp:hipl.
Other related posts:
