Code review comment for lp:~hipl-core/hipl/hipv2-dh-ecdh

Hi,

On 09/13/2012 06:33 AM, Christof Mroz wrote:
> Review: Needs Information
>
> Looks good overall, I found one potential bug though (the thing about possibly mismatched indices below), but I'm not sure.
> The other comments are not critical.
>
>> === modified file 'libcore/builder.c'
>> --- libcore/builder.c 2012-07-13 13:16:17 +0000
>> +++ libcore/builder.c 2012-09-08 08:34:18 +0000
>> @@ -119,7 +119,8 @@
>> * @param content the buffer to hold all the items
>> * @param count the number of items in the buffer
>> * @param item_size the size of each item in bytes. The function only supports
>> - * items in 2 bytes or 4 bytes.
>> + * byte conversion for items in 2 bytes or 4 bytes. For items
>> + * with other size, no conversion will be applied.
>> * @param flag the flag to decide the byte conversion order
>> */
>> static void convert_byte_order(void *content, unsigned count,
>> @@ -128,14 +129,12 @@
>> uint32_t (*f32)(uint32_t) = (flag == CBO_HTON) ? htonl : ntohl;
>> uint16_t (*f16)(uint16_t) = (flag == CBO_HTON) ? htons : ntohs;
>>
>> - HIP_ASSERT(item_size == sizeof(uint16_t) || item_size == sizeof(uint32_t));
>> -
> Why do you let this pass now? Does it make sense to specify anything other than 16 or 32 bit?

I use this function to build the DH_GROUP_LIST parameter, which is a
list of 8bit items, so I remove this assertion because otherwise you
need to do something like:

if (item_size != sizeof(uint16_t) || item_size != sizeof(uint32_t)) {
     convert_byte_order(...);
}

I think this extra if statement is unnecessary since in the
convert_byte_order function we have checked it already.

====

Thanks for your other comments. I recently have stacked many mails and
realized that I have blocked many processes. My problem is that I have
went back to China and started to work for a startup company. We have a
quite tight development timetable on which I have to spend most of my
time. I will try to fix the HIPL ECDH branch as soon as possible, but it
is really difficult for me to give a time guarantee. If it is urgent,
maybe someone can continue this work since the branch is already owned
by hipl-core,

--
Best regards,
Xin Gu