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

Hi,
thanks for you input René!

On Wed, Aug 10, 2011 at 1:52 PM, René Hummen <email address hidden> wrote:
> 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.
>> === 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.

Fixed.

>> +{
>> + 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.

See my comment below on this.

>> + 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.

Is it even necessary to differentiate between the two cases? Come to think of it an overflow seems very unlikely. I would not count too much on my math skills but by my calculation it would take more than 130 years with an update every second to reach the end of that range as it is unsigned 32bit. My suggestion would be to leave it out. The part of the code dealing with the incoming UPDATE IDs does not seem to care about this either.

>> * 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)".

Fixed.

>> === 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.

Fixed.