[PATCH] Add A5 and GEA ciphers

[☎]

Thank you for review, comments are inline, rewrite in progress :)

07.04.2013 22:26, Sylvain Munaut пишет:
>
>
> 1) Why do you need to add stddef.h ?

Required for size_t.

>   Also before of spacing change,
> you're removing an empty line that was there to separate the includes
> from the function comment.
>
> 2) wrt to :
>
> +uint16_t osmo_get2bytes(const uint8_t *a);
> +void osmo_64pack2pbit(uint64_t in, pbit_t *out);
>
> Theses essentially look like integer accessors, one is a read for
> uint16_t in big endian and the other a store for uint64_t in little
> endian, but same basic idea. But you're using completely different
> naming schemes for both ... I think they should reflect that their
> name should reflect the LE/BE part and the unaligned part as well, I
> even think there are already macro somewhere for that.  I would also
> add other formats like LE/BE 16/32/64 bits store/load all at once
> rather than each format when needed. If we add accessort, might as
> well add all the basic types. And finally those should really be
> inline functions in the .h, no point of doing a function call for
> that.

Is there some library I can use for that? It's indeed fairly trivial accessors but I
didn't managed to find those in osmocom code.

>
> 3) In include/osmocom/gsm/gsm_utils.h, for ms_a5n_support  you take
> only one cm argument ... that means the app needs to know if it should
> give cm3 or cm2, I would take cm2 and cm3 separately and the app just
> has to give both. I might also extend ms_cm2_a5n_support and
> ms_cm3_a5n_support to return -1 in case it couldn't be determined
> (because the required test isn't in that classmark), but that last
> point is just a suggestion.

Will fix, thanks.

>
> 4) TAB vs SPACE indentation !

Doh! Could you remind me - what was the magic tool which takes care of indentation
for kernel devs? Osmocom uses linux kernel code style, is it?

>
> 5) I would split the patch further between CM functions / buffere
> reversion / accessorts.
>
> 6) Is ROL16 really something we exepect to do at a lot of places ?
> just asking ... in anycase, also should probably be inline.

I thought it might but it turned out to be used only in single file. Should I hide it
away?

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