Merge lp:~stefan.goetz-deactivatedaccount/hipl/remove-peername into lp:hipl

[Stefan Götz]

Remove the effectively unused peer_hostname field from the HADB.

The data stored in this field was used exclusively for debugging output. This was achieved at the expense of copying around a fair bit of memory and cluttered function signatures.

This branch removes:
- the peer_hostname fields from HADB-related data structures
- the initialization and copying of the peer_hostname fields
- the debugging statements using the peer_hostname fields
- hostname-related function arguments from affected functions

[Miika Komu (miika-iki)]

Does "hipconf get ha all" still return the hostname of the peer (this is actually a very useful thing)?

[Stefan Götz (stefan.goetz-deactivatedaccount)]

No, the change in conf.c removes exactly this output.

It's just that currently, the peername handling, used only for debugging, is not RFC compliant so I thought removing the whole thing in an RFC compliant way is the easiest option. Achieving proper RFC compliance would require quite a bit of work - that's why I'm looking for alternatives :)

Do you see an alternative that would keep the hostname output for hipconf but would allow us to get rid of the peername field? I guess the only alternative is to use reverse DNS lookups which, of course, will not work for HITs for a long time to come, nor work for LSIs, nor work for unregistered host names which is probably a fairly common case in the scenarios that HIPL is used today. :-(

[Diego Biurrun (diego-biurrun)]

 review: approve

On Tue, Feb 22, 2011 at 07:47:09PM +0000, Stefan Götz wrote:
>
> Remove the effectively unused peer_hostname field from the HADB.

LGTM

Diego

review: Approve

[René Hummen (rene-hummen)]

Looks good to me.

review: Approve

[Miika Komu (miika-iki)]

I disapprove the change. I'll comment this in a moment.

[Miika Komu (miika-iki)]

My disapproval is intentionally informal. As I don't have much time to work on this at the moment and the result is compliant with RFCs, I'll leave this the decision to your conscience.

As you mentioned, look ups for FQDNs do not work *globally* so that's actually a damn good reason to keep it there. How difficult it is actually to correct the FQDN in host id? What's the problem?

It sounds to me that removal of this feature will cause more work for the future. Who ever will be the poor soul to reimplement this will face a hard time in getting it back due to the current strict practices (which I don't object btw). So has the classic "somebody elses problem" or "leave it as exercise for followers" written all over it.

When you are dealing with servers that have a multitude of host associations, the HITs are rather difficult to track. So, even though the FQDN is not trustworthy (until reverse look upped), it's a damn good hint to where to start look it at.

If you're more into scientific references, I think we have already measured with usability tests that users do not understand or don't want to understand the HITs (available in the web as pdf):

http://www.computer.org/portal/web/csdl/doi/10.1109/AICCSA.2009.5069337

So keeping the FQDN there has high value for end-users.

[Stefan Götz (stefan.goetz-deactivatedaccount)]

> So keeping the FQDN there has high value for end-users.

I'll retract this merge proposal and the one for the host-id-hostname-remove branch then.

What is necessary to achieve RFC compliance for the HOST_ID parameter:

incoming HOST_ID parameter:
------------------------------------

- accept DI strings of up to 2^12-1 bytes - this effectively means that we need to move from a fixed size character array in 'struct hip_host_id*' to dynamically allocated memory

- if the DI is an FQDN, it is encoded on the wire in the slightly unusual DNS-style run-length encoding instead of as a simple 0-terminated dotted string. Make sure, HIPL parses and prints such strings correctly (a plain printf() is not enough)

outgoing HOST_ID parameter:
------------------------------------

- put the FQDN, not only the short hostname, on the wire

- use the DNS-style run-length encoding on the wire, not just a 0-terminated dotted string

HADB:
------------------------------------

- decide on an encoding for the FQDN case

General:
------------------------------------

- ensure 0-termination throughout the whole code base

- ensure proper memory management of the DI strings throughout the whole code base

plus all the things I forgot ;-)

Unmerged revisions

5692. By Stefan Götz on 2011-02-22

Remove the effectively unused peer_hostname field from the HADB.

The data stored in this field was used exclusively for debugging output. This
was achieved at the expense of copying around a fair bit of memory and
cluttered function signatures.

=== modified file 'hipd/hadb.c'
--- hipd/hadb.c	2011-02-20 17:40:45 +0000
+++ hipd/hadb.c	2011-02-22 19:46:37 +0000
@@ -106,7 +106,6 @@
     struct in6_addr peer_addr;
     hip_lsi_t       peer_lsi;
     struct in6_addr our_addr;
-    uint8_t         peer_hostname[HIP_HOST_ID_HOSTNAME_LEN_MAX];
 };
 
 /**
@@ -394,7 +393,6 @@
  * @param local_hit local HIT
  * @param peer_hit peer HIT
  * @param peer_lsi peer LSI
- * @param peer_hostname peer host name
  * @param local_nat_udp_port local UDP port
  * @param peer_nat_udp_port peer UDP port
  *
@@ -404,7 +402,6 @@
                                  const hip_hit_t  *local_hit,
                                  const hip_hit_t  *peer_hit,
                                  const hip_lsi_t  *peer_lsi,
-                                 const char *peer_hostname,
                                  const in_port_t *local_nat_udp_port,
                                  const in_port_t *peer_nat_udp_port)
 {
@@ -423,9 +420,6 @@
     if (peer_lsi) {
         HIP_DEBUG_LSI("Peer LSI", peer_lsi);
     }
-    if (peer_hostname) {
-        HIP_DEBUG("Peer hostname: %s\n", peer_hostname);
-    }
 
     if (local_nat_udp_port) {
         HIP_DEBUG("Local NAT traversal UDP port: %d\n", *local_nat_udp_port);
@@ -444,15 +438,13 @@
  * @param  local_addr local address
  * @param  peer_addr peer address
  * @param  peer_lsi optional peer LSI (automatically generated if NULL)x
- * @param  peer_hostname peer hostname
  * @return zero on success or negative on error
  */
 int hip_hadb_add_peer_info_complete(const hip_hit_t *local_hit,
                                     const hip_hit_t *peer_hit,
                                     const hip_lsi_t *peer_lsi,
                                     const struct in6_addr *local_addr,
-                                    const struct in6_addr *peer_addr,
-                                    const char *peer_hostname)
+                                    const struct in6_addr *peer_addr)
 {
     int                    err   = 0;
     struct hip_hadb_state *entry = NULL, *aux = NULL;
@@ -464,7 +456,7 @@
 
     hip_print_debug_info(local_addr, peer_addr,
                          local_hit,  peer_hit,
-                         peer_lsi,   peer_hostname,
+                         peer_lsi,
                          &nat_udp_port_local,
                          &nat_udp_port_peer);
 
@@ -554,8 +546,7 @@
                                              &peer_map->peer_hit,
                                              &peer_map->peer_lsi,
                                              &peer_map->our_addr,
-                                             &peer_map->peer_addr,
-                                             (char *) &peer_map->peer_hostname),
+                                             &peer_map->peer_addr),
              -1,
              "Failed to add peer info\n");
 
@@ -569,13 +560,11 @@
  * @param peer_hit the HIT of the remote host
  * @param peer_addr the address of the remote host
  * @param peer_lsi an optional LSI for the remote host
- * @param peer_hostname an optional host name for the remote host
  * @return zero on success or negative on error
  */
 int hip_hadb_add_peer_info(const hip_hit_t *peer_hit,
                            const struct in6_addr *peer_addr,
-                           const hip_lsi_t *peer_lsi,
-                           const char *peer_hostname)
+                           const hip_lsi_t *peer_lsi)
 {
     int                      err      = 0;
     struct hip_peer_map_info peer_map = { { { { 0 } } } };
@@ -589,7 +578,6 @@
                          NULL,
                          peer_hit,
                          peer_lsi,
-                         peer_hostname,
                          &nat_local_udp_port,
                          &nat_peer_udp_port);
 
@@ -604,11 +592,6 @@
         memcpy(&peer_map.peer_lsi, peer_lsi, sizeof(struct in6_addr));
     }
 
-    if (peer_hostname) {
-        memcpy(peer_map.peer_hostname, peer_hostname,
-               HIP_HOST_ID_HOSTNAME_LEN_MAX - 1);
-    }
-
     HIP_IFEL(hip_select_source_address(&peer_map.our_addr, &peer_map.peer_addr),
              -1, "Cannot find source address\n");
 
@@ -628,10 +611,9 @@
  */
 int hip_add_peer_map(const struct hip_common *input)
 {
-    const struct in6_addr *hit           = NULL, *ip = NULL;
-    const hip_lsi_t       *lsi           = NULL;
-    const char            *peer_hostname = NULL;
-    int                    err           = 0;
+    const struct in6_addr *hit = NULL, *ip = NULL;
+    const hip_lsi_t       *lsi = NULL;
+    int                    err = 0;
 
     hit = hip_get_param_contents(input, HIP_PARAM_HIT);
 
@@ -639,8 +621,6 @@
 
     ip = hip_get_param_contents(input, HIP_PARAM_IPV6_ADDR);
 
-    peer_hostname = hip_get_param_contents(input, HIP_PARAM_HOSTNAME);
-
     if (!ip && (!lsi || !hit)) {
         HIP_ERROR("handle async map: no ip and maybe no lsi or hit\n");
         err = -ENODATA;
@@ -651,11 +631,7 @@
         HIP_DEBUG_LSI("lsi value is\n", lsi);
     }
 
-    if (peer_hostname) {
-        HIP_DEBUG("Peer hostname value is %s\n", peer_hostname);
-    }
-
-    err = hip_hadb_add_peer_info(hit, ip, lsi, peer_hostname);
+    err = hip_hadb_add_peer_info(hit, ip, lsi);
 
     if (err) {
         HIP_ERROR("Failed to insert peer map (%d)\n", err);
@@ -682,9 +658,6 @@
     entry->hastate       = HIP_HASTATE_INVALID;
     entry->purge_timeout = HIP_HA_PURGE_TIMEOUT;
 
-    /* Initialize the peer host name */
-    memset(entry->peer_hostname, '\0', HIP_HOST_ID_HOSTNAME_LEN_MAX);
-
     entry->peer_addresses_old = hip_linked_list_init();
 
     /* Randomize inbound SPI */
@@ -1254,7 +1227,6 @@
     ipv6_addr_copy(&hid.ip_peer, &entry->peer_addr);
     ipv4_addr_copy(&hid.lsi_our, &entry->lsi_our);
     ipv4_addr_copy(&hid.lsi_peer, &entry->lsi_peer);
-    memcpy(&hid.peer_hostname, &entry->peer_hostname, HIP_HOST_ID_HOSTNAME_LEN_MAX);
 
     /** @todo Modularize heartbeat */
 #if 0
@@ -1277,7 +1249,7 @@
     /* does not print heartbeat info, but I do not think it even should -Samu*/
     hip_print_debug_info(&hid.ip_our,   &hid.ip_peer,
                          &hid.hit_our,  &hid.hit_peer,
-                         &hid.lsi_peer, (char *) &hid.peer_hostname,
+                         &hid.lsi_peer,
                          &hid.nat_udp_port_local, &hid.nat_udp_port_peer);
 
     err = hip_build_param_contents(msg, &hid, HIP_PARAM_HA_INFO,

=== modified file 'hipd/hadb.h'
--- hipd/hadb.h	2011-01-17 12:09:48 +0000
+++ hipd/hadb.h	2011-02-22 19:46:37 +0000
@@ -82,15 +82,13 @@
 
 int hip_hadb_add_peer_info(const hip_hit_t *peer_hit,
                            const struct in6_addr *peer_addr,
-                           const hip_lsi_t *peer_lsi,
-                           const char *peer_hostname);
+                           const hip_lsi_t *peer_lsi);
 
 int hip_hadb_add_peer_info_complete(const hip_hit_t *local_hit,
                                     const hip_hit_t *peer_hit,
                                     const hip_lsi_t *peer_lsi,
                                     const struct in6_addr *local_addr,
-                                    const struct in6_addr *peer_addr,
-                                    const char *peer_hostname);
+                                    const struct in6_addr *peer_addr);
 
 int hip_del_peer_info_entry(struct hip_hadb_state *ha);
 int hip_del_peer_info(hip_hit_t *, hip_hit_t *);

=== modified file 'hipd/input.c'
--- hipd/input.c	2011-02-22 19:12:13 +0000
+++ hipd/input.c	2011-02-22 19:46:37 +0000
@@ -764,12 +764,6 @@
     /* we have to convert the compressed on the wire format into the internal host id representation */
     HIP_IFEL(hip_build_host_id_from_param((const struct hip_host_id *) param, &peer_host_id),
              -1, "Failed to convert host id parameter \n");
-    /* copy hostname to hadb entry if local copy is empty */
-    if (strlen((char *) (ctx->hadb_entry->peer_hostname)) == 0) {
-        memcpy(ctx->hadb_entry->peer_hostname,
-               hip_get_param_host_id_hostname(&peer_host_id),
-               HIP_HOST_ID_HOSTNAME_LEN_MAX - 1);
-    }
     HIP_IFE(hip_get_param_host_id_di_type_len(&peer_host_id, &str, &len) < 0, -1);
     HIP_DEBUG("Identity type: %s, Length: %d, Name: %s\n",
               str,

=== modified file 'hipd/netdev.c'
--- hipd/netdev.c	2011-02-22 19:12:13 +0000
+++ hipd/netdev.c	2011-02-22 19:46:37 +0000
@@ -906,8 +906,7 @@
     hip_nat_status = ha_nat_mode;
 
     /* To make it follow the same route as it was doing before locators */
-    HIP_IFEL(hip_hadb_add_peer_info(dst_hit, dst_addr,
-                                    dst_lsi, NULL), -1,
+    HIP_IFEL(hip_hadb_add_peer_info(dst_hit, dst_addr, dst_lsi), -1,
              "map failed\n");
 
     /* restore nat status */

=== modified file 'hipd/opp_mode.c'
--- hipd/opp_mode.c	2011-02-21 18:26:05 +0000
+++ hipd/opp_mode.c	2011-02-22 19:46:37 +0000
@@ -115,8 +115,7 @@
                                              &ctx->input_msg->hits,
                                              NULL,
                                              &ctx->dst_addr,
-                                             &ctx->src_addr,
-                                             NULL),
+                                             &ctx->src_addr),
              -1, "Failed to insert peer map\n");
 
     HIP_IFEL(!(ctx->hadb_entry = hip_hadb_find_byhits(&ctx->input_msg->hits,

=== modified file 'hipd/user.c'
--- hipd/user.c	2011-02-15 17:19:38 +0000
+++ hipd/user.c	2011-02-22 19:46:37 +0000
@@ -477,8 +477,7 @@
         }
 
         if (!opp_mode) {
-            HIP_IFEL(hip_hadb_add_peer_info(dst_hit, dst_ip,
-                                            NULL, NULL),
+            HIP_IFEL(hip_hadb_add_peer_info(dst_hit, dst_ip, NULL),
                      -1, "Error on adding server "  \
                          "HIT to IP address mapping to the hadb.\n");
 
@@ -516,8 +515,7 @@
                                                      &opp_hit,
                                                      NULL,
                                                      &src_ip,
-                                                     dst_ip,
-                                                     NULL),
+                                                     dst_ip),
                      -1,
                      "failed to add peer information to hadb\n");
 

=== modified file 'lib/core/conf.c'
--- lib/core/conf.c	2011-02-20 17:25:58 +0000
+++ lib/core/conf.c	2011-02-22 19:46:37 +0000
@@ -475,7 +475,6 @@
     HIP_INFO(" Local NAT traversal UDP port: %d\n", ha->nat_udp_port_local);
     HIP_INFO_IN6ADDR(" Peer  IP", &ha->ip_peer);
     HIP_INFO(" Peer  NAT traversal UDP port: %d\n", ha->nat_udp_port_peer);
-    HIP_INFO(" Peer  hostname: %s\n", &ha->peer_hostname);
     if (ha->heartbeats_on > 0 && ha->state == HIP_STATE_ESTABLISHED) {
         HIP_DEBUG(" Heartbeat %.3f ms mean RTT, "
                   "%.3f ms std dev,\n"

=== modified file 'lib/core/state.h'
--- lib/core/state.h	2011-02-06 09:03:39 +0000
+++ lib/core/state.h	2011-02-22 19:46:37 +0000
@@ -309,8 +309,6 @@
     HIP_HASHTABLE *peer_addr_list_to_be_added;
     /** For storing retransmission related data. */
     struct hip_msg_retrans hip_msg_retrans;
-    /** peer hostname */
-    uint8_t peer_hostname[HIP_HOST_ID_HOSTNAME_LEN_MAX];
     /** Counters of heartbeats (ICMPv6s) */
     int                    heartbeats_sent;
     struct statistics_data heartbeats_statistics;
@@ -358,7 +356,6 @@
     struct in6_addr ip_peer;
     hip_lsi_t       lsi_our;
     hip_lsi_t       lsi_peer;
-    uint8_t         peer_hostname[HIP_HOST_ID_HOSTNAME_LEN_MAX];
     int             state;
     int             heartbeats_on;
     int             heartbeats_sent;