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

  • From: Diego Biurrun <diego@xxxxxxxxxx>
  • To: hipl-dev@xxxxxxxxxxxxx
  • Date: Thu, 10 May 2012 23:55:33 +0200

 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, &params, 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, &params, 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, &params, 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, &params, flags);
> +                HIP_IFEL(err < 0 && err != -EWAITBEX && err != 
> -EBEXESTABLISHED,
> +                         -1, "hipl_sendmsg_internal() failed\n");
> +            }
> +            err = hipl_sendmsg_internal(hsock, &params, 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

Other related posts: