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

Hi,

On Thu, Dec 22, 2011 at 1:31 PM, Diego Biurrun <email address hidden> wrote:
> On Thu, Dec 22, 2011 at 10:05:21AM +0000, David Martin wrote:
>> On Wed, Dec 21, 2011 at 11:32 AM, Diego Biurrun <email address hidden> wrote:
>> > 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/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?
>>
>> Nope. It's just a pointer so that would not make much sense.
>
> Sorry, sizeof(*retrans->buf) of course, I make this mistake all the time :-/
>
>> But even referencing the struct does not seem right:
>>
>> debug(hipd/input.c:1891@hip_clear_retransmission): HIP_MAX_NETWORK_PACKET: 2048
>> debug(hipd/input.c:1892@hip_clear_retransmission): sizeof(retrans->buf): 8
>> debug(hipd/input.c:1893@hip_clear_retransmission): sizeof(*retrans->buf): 40
>
> buf is a pointer to struct hip_common, which is
>
> struct hip_common {
> uint8_t payload_proto;
> uint8_t payload_len;
> uint8_t type_hdr;
> uint8_t ver_res;
> uint16_t checksum;
> uint16_t control;
> struct in6_addr hits; /**< Sender HIT */
> struct in6_addr hitr; /**< Receiver HIT */
> } __attribute__((packed));
>
> so I guess the 40 bytes is about right.
>
> So why 2048? It looks as though you are zeroing way beyond the field
> you intend to initialize ...

You are right and wrong. 40 bytes is wrong as this would only be the size of the message header. You have to keep in mind that there's payload as well. You are right as 2048 is the maximum size and a sort of pessimistic approach. I've changed it so that it zeros only the actual message size. The queuing function uses hip_get_msg_total_len() to determine how many bytes to copy into the buffer. I'm using the same now to determine how many bytes to zero. They are the only two functions actually changing the buffer so this is good now.

>> >> +/**
>> >> + * 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?
>>
>> Nothing unforeseen may happen that I deem important enough to break
>> the handling of packets. I would make it void but the handle functions
>> have to return an int.
>
> OK, a short comment explaining this would be nice.

Fixed.