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

Hi,

On Thu, Dec 22, 2011 at 8:42 AM, Miika Komu <email address hidden> wrote:
> On 22/12/11 00:22, Diego Biurrun wrote:
>>
>> On Wed, Dec 21, 2011 at 11:32:26AM +0100, Diego Biurrun 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/output.c 2011-11-10 10:53:21 +0000
>>>> +++ hipd/output.c 2011-12-20 11:41:33 +0000
>>>> @@ -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));
>>
>>
>> The comment I intended to add here was that this looks as if it can be
>> done by assignments instead of memcpy.
>
>
> if you refer to IPv6 addresses, they have to be copied (no such thing as
> 128-bit integer). Anyway, the most elegant way is to use ipv6_addr_copy()
> instead of memcpy().

I don't think you can assign the buffer as they are independent. The retrans buffer slots are allocated at startup and the message comes wherever the caller allocates it from.

I've removed the needless variable indirection of the int len though and replaced the address memcpy() calls with ipv6_addr_copy().