Code review comment for lp:~martin-lp/hipl/update_ack_handling

review needs-fixing

I just have a few comments inline.

On 10.08.2011, at 13:16, David Martin wrote:
> David Martin has proposed merging lp:~martin-lp/hipl/update_ack_handling into lp:hipl.
>
> Requested reviews:
> HIPL core team (hipl-core)
>
> For more details, see:
> https://code.launchpad.net/~martin-lp/hipl/update_ack_handling/+merge/71027

> === modified file 'modules/update/hipd/update.c'
> --- modules/update/hipd/update.c 2011-07-18 16:41:13 +0000
> +++ modules/update/hipd/update.c 2011-08-10 11:16:05 +0000

> @@ -157,6 +158,41 @@
> }
>
> /**
> + * Verify the validity of the ACK ID of a received Update packet and update the
> + * range in which Update packets will be accepted accordingly.
> + *
> + * @param state The update state of the respective HA.
> + * @param ack_peer_update_id The received ACK ID.
> + *
> + * @return true if received ACK is valid
> + * false else
> + */
> +static bool check_and_update_ack_id_bounds(struct update_state *const state,
> + uint32_t ack_peer_update_id)

const uint32_t ack_peer_update_id would be more const correct.

> +{
> + HIP_ASSERT(state);
> +
> + /* two cases:
> + * oldest not yet acknowledged SEQ <= latest sent Update SEQ
> + * => received ACK must be in those bounds to be valid
> + * oldest not yet acknowledged SEQ > latest sent Update SEQ (counter overflow)
> + * => received ACK must be greater than oldest SEQ or smaller than latest
> + * sent Update ID to be valid
> + */

This comment is hard to understand without knowledge about your internal seq/ack data structure. Please describe how the two fields update_id_out_lower_bound and update_id_out are used in a normal update situation.

> + if ((state->update_id_out_lower_bound <= hip_update_get_out_id(state) &&
> + ack_peer_update_id >= state->update_id_out_lower_bound &&
> + ack_peer_update_id <= hip_update_get_out_id(state)) ||
> + (state->update_id_out_lower_bound > hip_update_get_out_id(state) &&
> + (ack_peer_update_id >= state->update_id_out_lower_bound ||
> + ack_peer_update_id <= hip_update_get_out_id(state)))) {
> + state->update_id_out_lower_bound = ack_peer_update_id;
> + return true;
> + } else {
> + return false;
> + }

Please break this down into a if () ... else if () ... else for higher readability.

> * Check if UPDATE sequence and acknowledgment numbers are as expected.
> *
> * @param packet_type the packet type
> @@ -216,12 +252,12 @@
> HIP_DEBUG("ACK parameter found with peer Update ID %u.\n",
> ack_peer_update_id);
>
> - // we only want acks for our most current update
> - if (ack_peer_update_id != hip_update_get_out_id(localstate)) {
> - HIP_DEBUG("Update ID (%u) in the ACK parameter is not "
> - "equal to the last outgoing Update ID (%u). "
> + if (!check_and_update_ack_id_bounds(localstate, ack_peer_update_id)) {
> + HIP_DEBUG("Update ID (%u) in the ACK parameter is not one of the "
> + "yet to be acknowledged Update IDs (%u-%u). "

Let's call this "current update ID window (%u-%u)".

> "Dropping the packet.\n",
> ack_peer_update_id,
> + localstate->update_id_out_lower_bound,
> hip_update_get_out_id(localstate));
> err = -1;
> goto out_err;
> @@ -487,10 +523,11 @@
> return -1;
> }
>
> - update_state->update_state = 0;
> - update_state->valid_locators = 0;
> - update_state->update_id_out = 0;
> - update_state->update_id_in = 0;
> + update_state->update_state = 0;
> + update_state->valid_locators = 0;
> + update_state->update_id_out = 0;
> + update_state->update_id_out_lower_bound = 0;
> + update_state->update_id_in = 0;
>
> return lmod_add_state_item(state, update_state, "update");
> }
>
> === modified file 'modules/update/hipd/update.h'
> --- modules/update/hipd/update.h 2011-07-28 18:11:17 +0000
> +++ modules/update/hipd/update.h 2011-08-10 11:16:05 +0000
> @@ -83,6 +83,10 @@
>
> /** Stored outgoing UPDATE ID counter. */
> uint32_t update_id_out;

Please update the doxygen comment of update_id_out to better reflect the difference between the two update_id_out* values.

> + /** Stored oldest not yet acknowledged outgoing UPDATE ID counter. */
> + uint32_t update_id_out_lower_bound;

--
Dipl.-Inform. Rene Hummen, Ph.D. Student
Chair of Communication and Distributed Systems
RWTH Aachen University, Germany
tel: +49 241 80 20772
web: http://www.comsys.rwth-aachen.de/team/rene-hummen/