[PATCH] Add generic LE/BE load/store uint type convertors and use them in msgb

[☎]

06.03.2014 12:28, Holger Hans Peter Freyther пишет:
> On Thu, Mar 06, 2014 at 11:52:10AM +0100, ☎ wrote:
>> Thanks for comments - all either fixed or comments added to clarify.
> 
> let me have another look. On general comment. The amount of parameters
> you pass to the method are a lot. Would you be able to be woken up in
> the middle of the day/night (whatever is less comfortable) and would
> you know the parameters? Did you consider passing them as a struct?

That's why I referenced corresponding standard in the comment. I do not see how
adding struct will help with remembering parameter names at night - I think it will
increase code complexity and decrease readability. Besides those are parameters to
internal-only functions - you are not supposed to work with this code without reading
corresponding standard beforehand.

> 
>> +static void check_ls_64(uint8_t bytes)
>> +{
>> +	/* calculate various adjustment constants (number of bits, bytes, octets etc.)
>> +		based on number of bytes in type we actually test */
> 
> 
> I was more thinking about what exactly do you want to test? Encode/Decode
> being compatible with each other? Corner cases? This is very difficult to
> understand code and I don't see your intend. I wonder/guess that there is
> a more simple approach to it.

I'll add more comments to clarify - in general, I do not see better approach for
serialize/deserialize kind of functions than trying to read/write and compare the
results.

> 
> coding style. :)
> 

Would you mind to be more specific? I know that it's linux-kernel style but it would
greatly help if you spend 3 more seconds to add few words to clarify what exactly
you're unhappy about - for example in this particular case I've been confused by the
undescriptive comments from previous email.

> 
>> +void _kasumi_kgcore(uint8_t CA, uint8_t cb, uint32_t cc, uint8_t cd, const uint8_t *ck, uint8_t *co, uint16_t cl);
> 
> sorry. I didn't see that the first time. _NAME is reversed by the system.
> 

Is there some general way to mark function as internal-use only? For example the only
reason I'm exposing _kasumi* is to be able to use them in test/kasumi_test code.


>> +inline static uint16_t _kasumi_FI(uint16_t I, uint16_t skey)
> 
> _kasumi is reserved for gcc/glibc. :)
> 
>> +	static const uint16_t S7[] = {
>> +	};
>> +	static const uint16_t S9[] = {
>> +	};
> 
> these tables were copied from the spec?

Yes, sure. The test vectors in kasumi_test were taken from the spec as well.



-- 
best regards,
Max, http://fairwaves.ru