On Mon, Nov 22, 2010 at 05:21:54PM +0000, René Hummen wrote:
> René Hummen has proposed merging lp:~hipl-core/hipl/mobility-fixes into lp:hipl.
> 
> Requested reviews:
>   HIPL core team (hipl-core)

Looks mostly good, I noticed some issues below.

 review needs-fixing

> --- modules/update/Makefile.am	2010-03-30 08:44:33 +0000
> +++ modules/update/Makefile.am	2010-11-22 17:21:23 +0000
> @@ -1,6 +1,7 @@
>  
>  modules_update_hipd_libhipupdate_la_SOURCES = modules/update/hipd/update.c \
> -                                              modules/update/hipd/update_legacy.c
> +                                              modules/update/hipd/update_legacy.c \
> +                                              modules/update/hipd/update_builder.c

alphabetical order please

> --- modules/update/hipd/update.c	2010-11-12 16:42:54 +0000
> +++ modules/update/hipd/update.c	2010-11-22 17:21:23 +0000
> @@ -616,6 +622,88 @@
>  
>  /**
> + * Retreive a @c LOCATOR ADDRESS ITEM@c from a list.

The second @c looks misplaced, I don't think you wanted to markup "from".
I suggest you run 'make doxygen' and see if this really produces what
you intended to express.

> +static union hip_locator_info_addr *hip_get_locator_item(void *item_list, int idx)

needlessly long line

> +{
> +    int i = 0;
> +    struct hip_locator_info_addr_item *temp;
> +    char *result;
> +    result = (char *) item_list;

pointless void* cast

> +    for (i = 0; i <= idx - 1; i++) {
> +        temp = (struct hip_locator_info_addr_item *) result;
> +        if (temp->locator_type == HIP_LOCATOR_LOCATOR_TYPE_ESP_SPI ||
> +            temp->locator_type == HIP_LOCATOR_LOCATOR_TYPE_IPV6) {
> +            result += sizeof(struct hip_locator_info_addr_item);
> +        } else {
> +            result += sizeof(struct hip_locator_info_addr_item2);
> +        }

The temp assignment looks useless, does the following (untested) work?

for (i = 0; i <= idx - 1; i++) {
    if (result->locator_type == HIP_LOCATOR_LOCATOR_TYPE_ESP_SPI ||
        result->locator_type == HIP_LOCATOR_LOCATOR_TYPE_IPV6) {
        result += sizeof(struct hip_locator_info_addr_item);
    } else {
        result += sizeof(struct hip_locator_info_addr_item2);
    }
}

> +/**
> + * Retrieve the amount the locators inside a LOCATOR parameter.

Doh?

> + * Type 1 and 2 parameters are supported.
> + *
> + * @param locator a LOCATOR parameter
> + * @return the amount of locators

s/amount/number/

> +int hip_get_locator_addr_item_count(const struct hip_locator *locator)
> +{
> +    const char *address_pointer = (const char *) (locator + 1);
> +    int amount                  = 0;
> +    uint8_t type;
> +
> +    while (address_pointer <
> +          ((const char *) locator) + hip_get_param_contents_len(locator)) {
> +        type = ((const struct hip_locator_info_addr_item *)
> +               address_pointer)->locator_type;

address_pointer is never really used as char*, so it should possibly be
one of the types it is cast to.  Also I wonder why you use const for a
variable that is assigned to later in the function.

> +        if (type == HIP_LOCATOR_LOCATOR_TYPE_UDP) {
> +            address_pointer += sizeof(struct hip_locator_info_addr_item2);
> +            amount += 1;
> +        } else if (type == HIP_LOCATOR_LOCATOR_TYPE_ESP_SPI) {
> +            address_pointer += sizeof(struct hip_locator_info_addr_item);
> +            amount += 1;
> +        } else if (type == HIP_LOCATOR_LOCATOR_TYPE_IPV6) {
> +            address_pointer += sizeof(struct hip_locator_info_addr_item);
> +            amount += 1;

The last two if-clauses can be merged.

> --- modules/update/hipd/update.h	2010-11-19 16:10:03 +0000
> +++ modules/update/hipd/update.h	2010-11-22 17:21:23 +0000
> @@ -38,6 +38,66 @@
>  
> +/**
> + * Fixed start of this struct must match to struct hip_peer_addr_list_item
> + * for the part of address item. It is used in hip_update_locator_match().
> + * @todo Maybe fix this in some better way?
> + */

Please elaborate.

> +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 (prefered) */

prefeRRed, same below

> --- modules/update/hipd/update_builder.c	1970-01-01 00:00:00 +0000
> +++ modules/update/hipd/update_builder.c	2010-11-22 17:21:23 +0000
> @@ -0,0 +1,111 @@
> +
> +#include <string.h>
> +
> +#include "lib/core/builder.h"
> +#include "lib/core/ife.h"
> +#include "update_builder.h"
> +
> +
> +/**
> + * build and append a HIP SEQ parameter to a message
> + *
> + * @param msg the message where the parameter will be appended
> + * @param update_id Update ID
> + * @return 0 on success, otherwise < 0.
> + */
> +int hip_build_param_seq(struct hip_common *msg, uint32_t update_id)

Add a stdint.h #include for uint32_t.

> +int hip_build_param_locator(struct hip_common *msg,
> +                            struct hip_locator_info_addr_item *addrs,
> +                            int addr_count)
> +{
> +    int err                          = 0;
> +    struct hip_locator *locator_info = NULL;
> +    int addrs_len = addr_count * (sizeof(struct hip_locator_info_addr_item));

pointless ()

> --- modules/update/hipd/update_builder.h	1970-01-01 00:00:00 +0000
> +++ modules/update/hipd/update_builder.h	2010-11-22 17:21:23 +0000
> @@ -0,0 +1,47 @@
> +
> +#include "lib/core/protodefs.h"
> +#include "update.h"
> +
> +#ifndef MODULES_UPDATE_HIPD_UPDATE_BUILDER_H
> +#define MODULES_UPDATE_HIPD_UPDATE_BUILDER_H
> +
> +int hip_build_param_seq(struct hip_common *msg, uint32_t update_id);
> +int hip_build_param_ack(struct hip_common *msg, uint32_t peer_update_id);
> +int hip_build_param_locator(struct hip_common *msg,
> +                            struct hip_locator_info_addr_item *addrs,
> +                            int addr_count);

Place the multiple inclusion guards before all the code, including
the #includes (pun intended) ;)

Add a stdint.h #include for the uint32_t typedef.

Does your code pass 'make checkheaders'?  (It probably does, just checking)

Diego