[hipl-dev] Re: [Merge] lp:~hipl-core/hipl/libhip into lp:hipl

  • From: Diego Biurrun <diego@xxxxxxxxxx>
  • To: hipl-dev@xxxxxxxxxxxxx
  • Date: Mon, 06 Feb 2012 18:44:06 +0100

 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
>  endif

That 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.la

Dude, 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

Other related posts: