Merge lp:~henrik-ziegeldorf/hipl/locator-type1-fix into lp:hipl

[Henrik Ziegeldorf]

This branch fixes BUG#790487 by including the ESP SPI in type 1 locators.
Additionally, the code for building the locator parameter is refactored and improved.

[lp:~henrik-ziegeldorf/hipl/locator-type1-fix]

6128. By Henrik Ziegeldorf on 2011-11-24

Fixed misleading comment.

[Diego Biurrun (diego-biurrun)]

 review approve

On Thu, Nov 24, 2011 at 01:36:51PM +0000, Henrik Ziegeldorf wrote:
> Henrik Ziegeldorf has proposed merging lp:~henrik-ziegeldorf/hipl/locator-type1-fix into lp:hipl.

LGTM

> --- modules/update/hipd/update.h 2011-08-18 10:11:46 +0000
> +++ modules/update/hipd/update.h 2011-11-24 13:35:59 +0000
> @@ -91,12 +91,18 @@
>
> +/**
> + * type 1 locator item
> + */
> struct hip_locator_info_addr_item {
> uint8_t traffic_type;
> uint8_t locator_type;
> uint8_t locator_length;
> uint8_t reserved; /**< last bit is P (preferred) */
> uint32_t lifetime;
> + /* The locator field comprises of the ESP SPI and address (32 bit, IPv6 or IPv4-in-IPv6)

comprises the

> --- modules/update/hipd/update_builder.c 2011-08-15 14:11:56 +0000
> +++ modules/update/hipd/update_builder.c 2011-11-24 13:35:59 +0000
> @@ -33,10 +33,16 @@
>
> +#include "hipd/hadb.h"
> +#include "hipd/hipd.h"
> #include "lib/core/builder.h"
> #include "lib/core/ife.h"
> +#include "lib/core/list.h"
> +#include "lib/core/prefix.h"
> #include "update_builder.h"
>
> +#define HIP_LOCATOR_TRAFFIC_TYPE_DUAL 0
> +#define HIP_LOCATOR_TRAFFIC_TYPE_SIGNAL 1

enum? just a suggestion, feel free to ignore ...

Diego

review: Approve

[Christof Mroz (christof-mroz)]

One more minor thing:

On 24.11.2011 14:36, Henrik Ziegeldorf wrote:
> + int err = 0, i = 0, count = 0, addrs_len;

i and count are unsigned, and addrs_len is size_t.

[Diego Biurrun (diego-biurrun)]

On Thu, Nov 24, 2011 at 01:36:51PM +0000, Henrik Ziegeldorf wrote:
> Henrik Ziegeldorf has proposed merging lp:~henrik-ziegeldorf/hipl/locator-type1-fix into lp:hipl.
>
> --- modules/update/hipd/update_builder.c 2011-08-15 14:11:56 +0000
> +++ modules/update/hipd/update_builder.c 2011-11-24 13:35:59 +0000
> @@ -80,22 +86,25 @@
>
> - HIP_IFE(!(locator_info = malloc(sizeof(struct hip_locator) + addrs_len)), -1);
> + addrs_len = address_count * sizeof(struct hip_locator_info_addr_item);
> + HIP_IFEL(!(locator_info = malloc(sizeof(struct hip_locator) + addrs_len)),
> + -1, "Could not allocate space for locator parameter\n");

You could return -ENOMEM here instead. Yes, that already applies to
many many parts of HIPL.

Diego

[Henrik Ziegeldorf (henrik-ziegeldorf)]

> One more minor thing:
>
> On 24.11.2011 14:36, Henrik Ziegeldorf wrote:
> > + int err = 0, i = 0, count = 0,
> addrs_len;
>
> i and count are unsigned, and addrs_len is size_t.

Please include the corresponding hunk of the change, so I'm able to quickly locate the place in the code.

[Henrik Ziegeldorf (henrik-ziegeldorf)]

> + HIP_IFEL(!(locator_info = malloc(sizeof(struct hip_locator) + addrs_len)),
> + -1, "Could not allocate space for locator parameter\n");
>
>
> Is locator_info only an intermediate struct, which is copied into the packet and then forgotten? If > so, then it could probably be allocated on the stack, thus saving you the free() afterwards.
>
> Unless the structs are prohibitively large, of course. I'll trust your judgement here.

You're right. However, there'll be a subsequent branch for refactoring the locator code. I'll probably address it there.

I've fixed all other issues.

If there are no further comments by tomorrow, I'll effect the merge.

[lp:~henrik-ziegeldorf/hipl/locator-type1-fix]

6129. By Henrik Ziegeldorf on 2011-11-24

Use enum instead of defines.

6130. By Henrik Ziegeldorf on 2011-11-24

cosmetics: spelling

6131. By Henrik Ziegeldorf on 2011-11-24

Return -ENOMEM instead of -1.

6132. By Henrik Ziegeldorf on 2011-11-24

Format fix.

6133. By Henrik Ziegeldorf on 2011-11-24

addrs_len is of type size_t

6134. By Henrik Ziegeldorf on 2011-11-24

count is unsigned

6135. By Henrik Ziegeldorf on 2011-11-24

cosmetics: fix format

[Christof Mroz (christof-mroz)]

On 24.11.2011 15:53, Henrik Ziegeldorf wrote:
>> One more minor thing:
>>
>> On 24.11.2011 14:36, Henrik Ziegeldorf wrote:
>>> + int err = 0, i = 0, count = 0,
>> addrs_len;
>>
>> i and count are unsigned, and addrs_len is size_t.
>
> Please include the corresponding hunk of the change, so I'm able to quickly locate the place in the code.

Sorry for that. Here's the hunk i meant:

> === modified file 'modules/update/hipd/update_builder.c'
> --- modules/update/hipd/update_builder.c 2011-08-15 14:11:56 +0000
> +++ modules/update/hipd/update_builder.c 2011-11-24 13:35:59 +0000
> @@ -80,22 +86,25 @@
> + int err = 0, i = 0, count = 0, addrs_len;

[Miika Komu (miika-iki)]

I noticed one function with one free() was removed. Where's the memory deallocated now?

You talk about refactoring but did you *test* handovers?

review: Needs Information

[Henrik Ziegeldorf (henrik-ziegeldorf)]

> I noticed one function with one free() was removed. Where's the memory
> deallocated now?

Nowhere. That part was obsolete and has been removed during refactoring. Before, a temporary, intermediate message was used to build the parameter into and copy it from there later. All in all it was pretty useless and misleading.

> You talk about refactoring but did you *test* handovers?

Yes, on my virtual machines.

=== modified file 'modules/update/hipd/update.c'
--- modules/update/hipd/update.c	2011-10-25 21:14:16 +0000
+++ modules/update/hipd/update.c	2011-11-24 15:19:25 +0000
@@ -707,13 +707,12 @@
  */
 int hip_trigger_update(struct hip_hadb_state *const hadb_entry)
 {
-    struct hip_common                 *locator_update_packet = NULL;
-    struct hip_common                 *locator_msg           = NULL;
-    struct hip_locator_info_addr_item *locators              = NULL;
-    struct update_state               *localstate            = NULL;
-    struct in6_addr                    local_addr;
-    int                                err        = 0;
-    const int                          retransmit = 1;
+    struct hip_common   *locator_update_packet = NULL;
+    struct hip_common   *locator_msg           = NULL;
+    struct update_state *localstate            = NULL;
+    struct in6_addr      local_addr;
+    int                  err                   = 0;
+    const int            retransmit            = 1;
 
     localstate = lmod_get_state_item(hadb_entry->hip_modular_state, "update");
 
@@ -732,13 +731,7 @@
                                       hadb_entry->spi_inbound_current),
              -1, "Building of ESP_INFO param failed\n");
 
-    HIP_IFEL(!(locator_msg = hip_msg_alloc()),
-             -ENOMEM, "Out of memory while allocation memory for the packet\n");
-    HIP_IFE(hip_create_locators(locator_msg, &locators), -1);
-
-    HIP_IFEL(hip_build_param_locator(locator_update_packet,
-                                     locators,
-                                     address_count),
+    HIP_IFEL(hip_build_param_locator(locator_update_packet),
              -1, "failed to build locator parameter\n");
 
     localstate->update_id_out++;

=== modified file 'modules/update/hipd/update.h'
--- modules/update/hipd/update.h	2011-08-18 10:11:46 +0000
+++ modules/update/hipd/update.h	2011-11-24 15:19:25 +0000
@@ -91,12 +91,18 @@
     /* fixed part ends */
 } __attribute__((packed));
 
+/**
+ * type 1 locator item
+ */
 struct hip_locator_info_addr_item {
     uint8_t         traffic_type;
     uint8_t         locator_type;
     uint8_t         locator_length;
     uint8_t         reserved; /**< last bit is P (preferred) */
     uint32_t        lifetime;
+    /* The locator field comprises the 32-bit ESP SPI and address (IPv6 or IPv4-in-IPv6)
+     * (c.f. http://tools.ietf.org/html/rfc5206#section-4.2) */
+    uint32_t        esp_spi;
     struct in6_addr address;
 }  __attribute__((packed));
 

=== modified file 'modules/update/hipd/update_builder.c'
--- modules/update/hipd/update_builder.c	2011-08-15 14:11:56 +0000
+++ modules/update/hipd/update_builder.c	2011-11-24 15:19:25 +0000
@@ -32,11 +32,20 @@
 
 #include <arpa/inet.h>
 #include <string.h>
+#include <errno.h>
 
+#include "hipd/hadb.h"
+#include "hipd/hipd.h"
 #include "lib/core/builder.h"
 #include "lib/core/ife.h"
+#include "lib/core/list.h"
+#include "lib/core/prefix.h"
 #include "update_builder.h"
 
+enum hip_locator_traffic_type {
+    HIP_LOCATOR_TRAFFIC_TYPE_DUAL,
+    HIP_LOCATOR_TRAFFIC_TYPE_SIGNAL
+};
 
 /**
  * build and append a HIP SEQ parameter to a message
@@ -80,22 +89,27 @@
 }
 
 /**
- * build a HIP locator parameter
+ * Build a HIP locator parameter.
  *
  * @param msg           the message where the REA will be appended
- * @param addrs         list of addresses
- * @param addr_count number of addresses
  * @return 0 on success, otherwise < 0.
  */
-int hip_build_param_locator(struct hip_common *const msg,
-                            const struct hip_locator_info_addr_item *const addrs,
-                            const int addr_count)
+int hip_build_param_locator(struct hip_common *const msg)
 {
-    int                 err          = 0;
-    struct hip_locator *locator_info = NULL;
-    int                 addrs_len    = addr_count * sizeof(struct hip_locator_info_addr_item);
+    int                                err           = 0, i = 0;
+    unsigned int                       count         = 0;
+    size_t                             addrs_len;
+    struct hip_locator                *locator_info  = NULL;
+    struct hip_locator_info_addr_item *loc_info_item = NULL;
+    struct hip_hadb_state             *ha            = NULL;
+    LHASH_NODE                        *item          = NULL, *tmp = NULL;
+    struct netdev_address             *n;
 
-    HIP_IFE(!(locator_info = malloc(sizeof(struct hip_locator) + addrs_len)), -1);
+    addrs_len = address_count * sizeof(struct hip_locator_info_addr_item);
+    HIP_IFEL(!(locator_info = malloc(sizeof(struct hip_locator) + addrs_len)),
+             -ENOMEM, "Could not allocate space for locator parameter\n");
+    HIP_IFEL(!(ha = hip_hadb_find_byhits(&msg->hits, &msg->hitr)),
+             -1, "Could not retrieve HA\n");
 
     hip_set_param_type((struct hip_tlv_common *) locator_info, HIP_PARAM_LOCATOR);
 
@@ -103,7 +117,27 @@
                                sizeof(struct hip_locator),
                                addrs_len);
 
-    memcpy(locator_info + 1, addrs, addrs_len);
+    /* build all locator info items from cached addresses */
+    loc_info_item = (struct hip_locator_info_addr_item *) (locator_info + 1);
+    list_for_each_safe(item, tmp, addresses, i) {
+        n = list_entry(item);
+        HIP_DEBUG_IN6ADDR("Add address:", hip_cast_sa_addr(((struct sockaddr *) &n->addr)));
+        HIP_ASSERT(!ipv6_addr_is_hit(hip_cast_sa_addr((struct sockaddr *) &n->addr)));
+        memcpy(&loc_info_item[count].address,
+               hip_cast_sa_addr((struct sockaddr *) &n->addr),
+               sizeof(struct in6_addr));
+        if (n->flags & HIP_FLAG_CONTROL_TRAFFIC_ONLY) {
+            loc_info_item[count].traffic_type = HIP_LOCATOR_TRAFFIC_TYPE_SIGNAL;
+        } else {
+            loc_info_item[count].traffic_type = HIP_LOCATOR_TRAFFIC_TYPE_DUAL;
+        }
+        loc_info_item[count].locator_type   = HIP_LOCATOR_LOCATOR_TYPE_ESP_SPI;
+        loc_info_item[count].locator_length = sizeof(struct in6_addr) / 4;
+        loc_info_item[count].reserved       = 0;
+        loc_info_item[count].esp_spi        = htonl(ha->spi_inbound_current);
+        count++;
+    }
+
     HIP_IFE(hip_build_param(msg, locator_info), -1);
 
 out_err:

=== modified file 'modules/update/hipd/update_builder.h'
--- modules/update/hipd/update_builder.h	2011-08-15 14:11:56 +0000
+++ modules/update/hipd/update_builder.h	2011-11-24 15:19:25 +0000
@@ -41,8 +41,6 @@
 int hip_build_param_seq(struct hip_common *const msg, const uint32_t update_id);
 int hip_build_param_ack(struct hip_common *const msg,
                         const uint32_t peer_update_id);
-int hip_build_param_locator(struct hip_common *const msg,
-                            const struct hip_locator_info_addr_item *const addrs,
-                            const int addr_count);
+int hip_build_param_locator(struct hip_common *const msg);
 
 #endif /* HIP_MODULES_UPDATE_HIPD_UPDATE_BUILDER_H */

=== modified file 'modules/update/hipd/update_locator.c'
--- modules/update/hipd/update_locator.c	2011-08-15 14:11:56 +0000
+++ modules/update/hipd/update_locator.c	2011-11-24 15:19:25 +0000
@@ -34,107 +34,15 @@
 #include <string.h>
 #include <openssl/lhash.h>
 
-#include "config.h"
-#include "hipd/hipd.h"
 #include "hipd/maintenance.h"
 #include "lib/core/builder.h"
 #include "lib/core/debug.h"
 #include "lib/core/ife.h"
-#include "lib/core/list.h"
-#include "lib/core/prefix.h"
 #include "lib/core/protodefs.h"
 #include "update_builder.h"
 #include "update.h"
 #include "update_locator.h"
 
-
-#define HIP_LOCATOR_TRAFFIC_TYPE_DUAL    0
-#define HIP_LOCATOR_TRAFFIC_TYPE_SIGNAL  1
-
-/**
- * build a LOCATOR parameter for an UPDATE packet
- *
- * @param msg the LOCATOR parameter will be appended to this UPDATE message
- * @return zero on success on negative on failure
- */
-int hip_build_locators_old(struct hip_common *const msg)
-{
-    int                                err = 0, i = 0, count = 0;
-    int                                addr_max;
-    struct netdev_address             *n;
-    LHASH_NODE                        *item = NULL, *tmp = NULL;
-    struct hip_locator_info_addr_item *locs = NULL;
-
-    if (address_count == 0) {
-        HIP_DEBUG("Host has only one or no addresses no point "
-                  "in building LOCATOR2 parameters\n");
-        goto out_err;
-    }
-
-    addr_max = address_count;
-
-    HIP_IFEL(!(locs = calloc(addr_max, sizeof(struct hip_locator_info_addr_item))),
-             -1, "Malloc for LOCATORS type1 failed\n");
-
-    HIP_DEBUG("there are %d type 1 locator item\n", addr_max);
-
-    list_for_each_safe(item, tmp, addresses, i) {
-        n = list_entry(item);
-        HIP_DEBUG_IN6ADDR("Add address:",
-                          hip_cast_sa_addr(((struct sockaddr *) &n->addr)));
-        HIP_ASSERT(!ipv6_addr_is_hit(hip_cast_sa_addr((struct sockaddr *) &n->addr)));
-        memcpy(&locs[count].address, hip_cast_sa_addr((struct sockaddr *) &n->addr),
-               sizeof(struct in6_addr));
-        if (n->flags & HIP_FLAG_CONTROL_TRAFFIC_ONLY) {
-            locs[count].traffic_type = HIP_LOCATOR_TRAFFIC_TYPE_SIGNAL;
-        } else {
-            locs[count].traffic_type = HIP_LOCATOR_TRAFFIC_TYPE_DUAL;
-        }
-        locs[count].locator_type   = HIP_LOCATOR_LOCATOR_TYPE_ESP_SPI;
-        locs[count].locator_length = sizeof(struct in6_addr) / 4;
-        locs[count].reserved       = 0;
-        count++;
-    }
-
-    HIP_DEBUG("locator count %d\n", count);
-
-    HIP_IFEL(count == 0, -1, "No locators to build\n");
-
-    err = hip_build_param_locator(msg, locs, count);
-
-out_err:
-    free(locs);
-    return err;
-}
-
-/**
- * build locators in an UPDATE message
- *
- * @param locator_msg the message where the LOCATOR should be appended
- * @param locators an extra pointer that will point to the LOCATOR
- * @return zero on success or negative on failure
- */
-int hip_create_locators(struct hip_common *const locator_msg,
-                        struct hip_locator_info_addr_item **locators)
-{
-    int                 err = 0;
-    struct hip_locator *loc = NULL;
-
-    hip_msg_init(locator_msg);
-    HIP_IFEL(hip_build_user_hdr(locator_msg,
-                                HIP_MSG_SET_LOCATOR_ON, 0), -1,
-             "Failed to add user header\n");
-    HIP_IFEL(hip_build_locators_old(locator_msg),
-             -1,
-             "Failed to build locators\n");
-    loc = hip_get_param_readwrite(locator_msg, HIP_PARAM_LOCATOR);
-    hip_print_locator_addresses(locator_msg);
-    *locators = (struct hip_locator_info_addr_item *) (loc + 1);
-
-out_err:
-    return err;
-}
-
 /**
  * Retrieve a locator address item from a list.
  *

=== modified file 'modules/update/hipd/update_locator.h'
--- modules/update/hipd/update_locator.h	2011-04-20 09:18:09 +0000
+++ modules/update/hipd/update_locator.h	2011-11-24 15:19:25 +0000
@@ -29,11 +29,6 @@
 #include "lib/core/protodefs.h"
 #include "update.h"
 
-int hip_build_locators_old(struct hip_common *const msg);
-
-int hip_create_locators(struct hip_common *const locator_msg,
-                        struct hip_locator_info_addr_item **locators);
-
 union hip_locator_info_addr *hip_get_locator_item(void *item_list,
                                                   int idx);