review needs-fixing On Tue, Dec 20, 2011 at 11:43:26AM +0000, David Martin wrote: > David Martin has proposed merging lp:~martin-lp/hipl/hipl_retransmissions into lp:hipl. > > --- hipd/hadb.c 2011-11-17 11:15:47 +0000 > +++ hipd/hadb.c 2011-12-20 11:41:33 +0000 > @@ -693,17 +693,25 @@ > } > > - entry->hip_msg_retrans.count = 0; > > /* Initialize module states */ You should remove an excess empty line here. > --- hipd/input.c 2011-12-09 09:32:56 +0000 > +++ hipd/input.c 2011-12-20 11:41:33 +0000 > @@ -1877,3 +1873,117 @@ > + > +/** > + * Clear the given retransmission. > + * i.e. set the remaining retransmissions to zero and zero the buffer. > + * > + * @param retrans The retransmission to be cleared. > + */ > +void hip_clear_retransmission(struct hip_msg_retrans *const retrans) > +{ > + if (!retrans) { > + return; > + } > + > + retrans->count = 0; > + if (retrans->buf) { > + memset(retrans->buf, 0, HIP_MAX_NETWORK_PACKET); sizeof(retrans->buf) does not work? > +/** > + * Update our buffered retransmissions after receiving the given packet. > + * This functions removes invalid retransmissions after a state change caused > + * by a received packet. For example after successfully receiving and handling > + * an I2 or R2 packet all our buffered packets are dropped. > + * > + * Call this function after successful handling of the incoming packet. > + * > + * @param packet_type The packet type of the control message (RFC 5201, 5.3.) > + * @param ha_state The host association state (RFC 5201, 4.4.1.) > + * @param ctx The packet context of the incoming transmission. > + * > + * @return always 0 > + */ Why always return 0? > +int hip_update_retransmissions(UNUSED const uint8_t packet_type, > + UNUSED const enum hip_state ha_state, > + struct hip_packet_context *const ctx) > +{ > + struct hip_msg_retrans *retrans; > + > + HIP_ASSERT(ctx); > + HIP_ASSERT(ctx->input_msg); > + > + /* When receiving an I1 there usually is no established state yet. */ > + if (!ctx->hadb_entry) { > + return 0; > + } > + > + for (unsigned int i = 0; i < HIP_RETRANSMIT_QUEUE_SIZE; i++) { > + retrans = &ctx->hadb_entry->hip_msg_retrans[i]; > + > + if (retrans->count == 0) { > + continue; > + } > + > + switch (hip_get_msg_type(ctx->input_msg)) { > + case HIP_I1: > + switch (hip_get_msg_type(retrans->buf)) { > + case HIP_I1: > + /* We only remove our sent I1s when we have got a greater HIT > + * than the peer (ref. RFC 5201, 4.4.2). */ > + if (hip_hit_is_bigger(&ctx->hadb_entry->hit_our, &ctx->hadb_entry->hit_peer)) { > + hip_clear_retransmission(retrans); > + } > + break; > + } > + break; This nested switch statement for only one case looks pretty weird IMO. > --- hipd/maintenance.c 2011-12-16 13:37:33 +0000 > +++ hipd/maintenance.c 2011-12-20 11:41:33 +0000 > @@ -92,53 +93,48 @@ > static int handle_retransmission(struct hip_hadb_state *entry, > void *current_time) > { > + int err = 0, i = 0; > + struct hip_msg_retrans *retrans; > + time_t *now = (time_t *) current_time; One can of course wonder why current_time is not the right type to begin with, but in any case there is no need to cast a void* pointer. > + for (i = 0; i < HIP_RETRANSMIT_QUEUE_SIZE; i++) { > + retrans = &entry->hip_msg_retrans[(entry->next_retrans_slot + i) % > + HIP_RETRANSMIT_QUEUE_SIZE]; > + > + /* check if the last transmission was at least RETRANSMIT_WAIT seconds ago */ > + if (*now - HIP_RETRANSMIT_WAIT > retrans->last_transmit) { > + if (retrans->count > 0) { > + /* @todo: verify that this works over slow ADSL line */ > + if (hip_send_pkt(&retrans->saddr, > + &retrans->daddr, > + (entry->nat_mode ? hip_get_local_nat_udp_port() : 0), pointless () > + entry->peer_udp_port, > + retrans->buf, > + entry, 0) < 0) { > + HIP_ERROR("Failed to retransmit packet of type %d.\n", > + hip_get_msg_type(retrans->buf)); > + err = -1; > + } Shouldn't you return here? > + /* Set entry state, if previous state was unassociated > + * and type is I1. */ > + if (!err && hip_get_msg_type(retrans->buf) == HIP_I1 && > + entry->state == HIP_STATE_UNASSOCIATED) { > + HIP_DEBUG("Resent I1 succcesfully\n"); > + entry->state = HIP_STATE_I1_SENT; > + } > + > + retrans->count--; > + /* set the last transmission time to the current time value */ > + time(&retrans->last_transmit); redundant_comment++; > --- hipd/output.c 2011-11-10 10:53:21 +0000 > +++ hipd/output.c 2011-12-20 11:41:33 +0000 > @@ -1066,7 +1066,7 @@ > err = hip_send_pkt(&ctx->dst_addr, &ctx->src_addr, > ctx->hadb_entry->nat_mode ? hip_get_local_nat_udp_port() : 0, > ctx->hadb_entry->peer_udp_port, ctx->output_msg, > - ctx->hadb_entry, 1); > + ctx->hadb_entry, 0); This is no longer a retransmission? > @@ -1118,22 +1118,25 @@ > const struct hip_common *msg, > struct hip_hadb_state *entry) > { > - int len = hip_get_msg_total_len(msg); > + int len = hip_get_msg_total_len(msg); > + struct hip_msg_retrans *retrans; > > if (!entry) { > return 0; > } > > + retrans = &entry->hip_msg_retrans[entry->next_retrans_slot]; > + > /* Not reusing the old entry as the new packet may have different length */ > + memset(retrans->buf, 0, HIP_MAX_NETWORK_PACKET); see above > + memcpy(retrans->buf, msg, len); > + memcpy(&retrans->saddr, src_addr, sizeof(struct in6_addr)); > + memcpy(&retrans->daddr, peer_addr, sizeof(struct in6_addr)); > --- lib/core/state.h 2011-12-16 13:37:33 +0000 > +++ lib/core/state.h 2011-12-20 11:41:33 +0000 > @@ -100,6 +100,10 @@ > HIP_HA_STATE_VALID = 1, > }; > > +/* Set the maximum number of queued retransmissions, i.e. a value of 3 buffers > + * the 3 most recent transmissions for retransmissions. */ > +#define HIP_RETRANSMIT_QUEUE_SIZE 3 Just a simple the maximum number of retransmissions to queue should be enough IMO - no need to explain what a queue is. > --- modules/update/hipd/update.c 2011-12-16 13:37:33 +0000 > +++ modules/update/hipd/update.c 2011-12-20 11:41:33 +0000 > @@ -364,7 +364,7 @@ > ctx->hadb_entry->peer_udp_port, > ctx->output_msg, > ctx->hadb_entry, > - 1); > + 0); > } This is no longer a retransmission? Diego