[hipl-dev] Re: [Merge] lp:~hipl-core/hipl/hipv2-modularization into lp:hipl
- From: Diego Biurrun <diego@xxxxxxxxxx>
- To: hipl-dev@xxxxxxxxxxxxx
- Date: Thu, 12 Jul 2012 18:43:59 +0200
review needs-fixing
On Sat, Jul 07, 2012 at 01:55:22PM +0000, Xin wrote:
> --- doc/HOWTO.xml.in 2012-05-12 07:08:30 +0000
> +++ doc/HOWTO.xml.in 2012-07-07 13:54:21 +0000
> @@ -2607,6 +2607,49 @@
>
> + <section id="ch_dual_version_support_functionality">
> + <title>Provided functionality</title>
> + <para>HIPL dual-version support allows a host to run HIPv1 and HIPv2 as
> + a dual stack. Based on the version number of received I1 messages, the
> host
> + automatically determines the HIP version for this peer. The version is
> + decided separately for each host association, which means the host can
> + have a HIPv1 association with one peer, while maintaining a HIPv2
> + association with another peer.</para>
> + <para>HIPL dual-version support also enables a host to choose the default
> + HIP version for HIP association initiation. This behavior can be
> configured
> + via both the hipd configuration file and the hipconf command line tool.
> + </para>
I think you mean "HIP dual-version support" and not "HIPL" here, since
you are referring to the protocol, not the Linux implementation.
> + <para>The configuration parameter:
> <emphasis>hip-default-version</emphasis>
> + in the hipd.conf file accepts two value: 1 and 2, which stands for HIPv1
two value_s_, which stand_ for
> + and HIPv2 respectively. If the hipd cannot find this parameter in the
> + configuration file, HIPv1 will be used as a default value.
s/as a/as/
> + <para>Modification of the configuration parameter: <emphasis>
> + hip-default-version</emphasis> in the hipconf command line takes effect
> + immediately and no restart of the daemon is required. Any HA before
> + the modification keeps unchanged and all new HIP initiation messages
s/keeps/remains/
> + (the I1 message) will be switched to the version given by the hipconf
(I1 messages)
> --- hipd/hipd.conf 2011-12-12 14:26:13 +0000
> +++ hipd/hipd.conf 2012-07-07 13:54:21 +0000
> @@ -19,3 +19,4 @@
> debug medium # debug verbosity: all, medium, low or none
> +default-hip-version 1 # default HIP version number for the I1 message.
> (1=HIPV1, 2=HIPV2)
HIPv1, HIPv2
> --- libcore/builder.c 2012-05-12 10:21:32 +0000
> +++ libcore/builder.c 2012-07-07 13:54:21 +0000
> @@ -286,6 +318,28 @@
>
> /**
> + * Get the HIP version number from a HIP message header
> + *
> +
> +/**
> + * Set HIP version number to a given HIP message header
> + *
End sentences in periods please.
> @@ -1556,6 +1611,68 @@
>
> +/**
> + * Fetch the content of a HIP list-like parameter to a specified buffer.
> + *
> + * If the buffer size is smaller than the size of all items in the list, the
> + * result in the buffer will be truncated to first N items
> + * (N <= @c item_number). If the @c item_size is 2 bytes or 4 bytes, result
> + * items will be transformed from network byte order to host byte order.
> + *
> + * @param param pointer to the HIP list parameter.
> + * @param buffer the buffer for holding items of the list parameter.
> + * @param item_number a value-result argument. The caller must initialize it
> to
> + * the number of items @c buffer can hold; On return, it
> + * will contains the number of items in the given HIP list
will contain
> + * parameter.
> + * @param item_size the size of each item in bytes.
> + */
> +void hip_get_list_from_param(const struct hip_tlv_common *const param,
> + void *const buffer, int *const item_number,
> + const int item_size)
> +{
> + int plen = hip_get_param_contents_len(param);
> + int actual_count = plen / item_size;
> +
> + const void *p = param + 1;
> +
> + if (actual_count > *item_number) {
> + HIP_DEBUG("The buffer size is smaller than the size of list
> parameter.\n");
> + actual_count = *item_number;
> + } else {
> + *item_number = actual_count;
> + }
> + memcpy(buffer, p, actual_count * item_size);
> +
> + /* network to host byte order transform */
> + convert_byte_order(buffer, actual_count, item_size, false);
> +}
Returning the number of items in the buffer seems much more natural.
The API for this function feels weird.
> --- libcore/transform.c 2011-08-15 14:11:56 +0000
> +++ libcore/transform.c 2012-07-07 13:54:21 +0000
> -hip_transform_suite hip_select_esp_transform(const struct hip_esp_transform
> *ht)
> +hip_transform_suite hip_select_esp_transform(const struct hip_tlv_common
> *param)
> {
> + /* the first item in the list is a reserved field and should be ignored
> */
> + if (--item_number < 1) {
> + HIP_ERROR("Non ESP suite id can be found\n");
"can be found" ???
> --- libhipl/input.c 2012-05-12 10:21:32 +0000
> +++ libhipl/input.c 2012-07-07 13:54:21 +0000
> @@ -557,6 +560,23 @@
>
> + /* If the HIP version of this hadb entry hasn't been set, we set it
> + * now so handler functions can access it later from packet context.
from the
> + * If the hadb entry already records its HIP version, we check
> whether
> + * the version of the current receiving message equals the version in
> + * hadb and drop those messages with invalid version. */
> + if (ctx->hadb_entry->hip_version == 0) {
> + ctx->hadb_entry->hip_version = msg_hip_version;
> + HIP_DEBUG("Set HIP version of the hadb entry to %d\n",
> + msg_hip_version);
> + } else if (ctx->hadb_entry->hip_version != msg_hip_version) {
> + HIP_ERROR("The version of the inbound message (V%d) doesn't
> match"
> + "the version of the corresponding hadb record (V%d)\n",
You probably want a space between "match" and "the".
> --- libhipl/output.c 2012-05-12 10:21:32 +0000
> +++ libhipl/output.c 2012-07-07 13:54:21 +0000
> @@ -409,7 +411,7 @@
> /********** HIP transform. **********/
> HIP_IFE(!(param = hip_get_param(ctx->input_msg,
> HIP_PARAM_HIP_TRANSFORM)),
> -ENOENT);
> - HIP_IFEL((transform_hip_suite = hip_select_hip_transform((const struct
> hip_hip_transform *) param)) == 0,
> + HIP_IFEL((transform_hip_suite = hip_select_hip_transform(param)) == 0,
> -EINVAL, "Could not find acceptable HIP transform suite.\n");
>
> @@ -483,7 +485,7 @@
> /* Select only one transform */
> - HIP_IFEL((transform_esp_suite = hip_select_esp_transform((const struct
> hip_esp_transform *) param)) == 0,
> + HIP_IFEL((transform_esp_suite = hip_select_esp_transform(param)) == 0,
> -1, "Could not find acceptable hip transform suite\n");
You are eliminating silly casts - lovely :-)
Diego
Other related posts:
