Merge lp:~martin-lp/hipl/update_ack_handling into lp:hipl
Status: | Superseded |
---|---|
Proposed branch: | lp:~martin-lp/hipl/update_ack_handling |
Merge into: | lp:hipl |
Diff against target: |
112 lines (+58/-10) 2 files modified
modules/update/hipd/update.c (+50/-8) modules/update/hipd/update.h (+8/-2) |
To merge this branch: | bzr merge lp:~martin-lp/hipl/update_ack_handling |
Related bugs: |
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Christof Mroz | Needs Information | ||
René Hummen | Needs Fixing | ||
Review via email: mp+71027@code.launchpad.net |
This proposal has been superseded by a proposal from 2011-08-11.
Description of the change
This is a relatively small change but it addresses dealing with update
acknowledgments and it would be fatal to introduce bugs here.
The situation: previously only acknowledgments for the most recent sent
Update packet were accepted. This is a problem when both hosts initiate
an update at the same time.
The typical update sequence between host1 <-> host2 looks like this:
U1 (SEQ x) ->
U2 (SEQ y, ACK x) <-
U3 (ACK y) ->
If both hosts initiate simultaneously things go wrong:
U1 (SEQ x) ->
U1 (SEQ y) <-
U2 (SEQ x+1, ACK y) ->
U2 (SEQ y+1, ACK x) <-
if hosts only store the most recent sent update packet SEQ it breaks now
as host1 gets an ACK for x but expects x+1 (as this is the most recent).
As a consequence the packets get dropped and the update fails.
This branch introduces a lower boundary for outgoing Update packet IDs.
Valid received ACKs are the outstanding ones in
[lower_boundary, most recent sent].
Please check if I missed anything or if there is a flaw in the logic.
Actual tests or reproduction of the problem can be done with
tcpdump + wireshark or just by debug output.
- initiate BEX between two hosts (eg. by ping)
- run tools/hipconf manual-update at the same time on both hosts
review needs-fixing
I just have a few comments inline.
On 10.08.2011, at 13:16, David Martin wrote: /code.launchpad .net/~martin- lp/hipl/ update_ ack_handling/ +merge/ 71027
> 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:/
> === modified file 'modules/ update/ hipd/update. c' update/ hipd/update. c 2011-07-18 16:41:13 +0000 update/ hipd/update. c 2011-08-10 11:16:05 +0000
> --- modules/
> +++ modules/
> @@ -157,6 +158,41 @@ update_ ack_id_ bounds( struct update_state *const state,
> }
>
> /**
> + * 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_
> + 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) && update_ id_out_ lower_bound && get_out_ id(state) ) || >update_ id_out_ lower_bound > hip_update_ get_out_ id(state) && update_ id_out_ lower_bound || get_out_ id(state) ))) { update_ id_out_ lower_bound = ack_peer_update_id;
> + ack_peer_update_id >= state->
> + ack_peer_update_id <= hip_update_
> + (state-
> + (ack_peer_update_id >= state->
> + ack_peer_update_id <= hip_update_
> + state->
> + 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. update_ id); get_out_ id(localstate) ) { and_update_ ack_id_ bounds( localstate, ack_peer_ update_ id)) {
> *
> * @param packet_type the packet type
> @@ -216,12 +252,12 @@
> HIP_DEBUG("ACK parameter found with peer Update ID %u.\n",
> ack_peer_
>
> - // we only want acks for our most current update
> - if (ack_peer_update_id != hip_update_
> - HIP_DEBUG("Update ID (%u) in the ACK parameter is not "
> - "equal to the last outgoing Update ID (%u). "
> + if (!check_
> + HIP_DEBUG("Update ID (%u) in the ACK parameter is not one of the "
> + "yet to be acknowledged Update IDs (%u-%u). ...