[PATCH 3/3] Add Kasumi cipher implementation

Sylvain Munaut 246tnt at gmail.com
Sun Jun 15 20:49:45 UTC 2014


Hi Max,

> +
> +#pragma once

All the other files use the classic #ifdef #define, so either replace
all before that, or stick to the current way of doing it.

> +
> +#include <stdint.h>
> +
> +/*! \brief Single iteration of KASUMI cipher
> + * \param[in] P Block, 64 bits to be processed in this round
> + * \param[in] KLi1 Expanded subkeys
> + * \param[in] KLi2 Expanded subkeys
> + * \param[in] KOi1 Expanded subkeys
> + * \param[in] KOi2 Expanded subkeys
> + * \param[in] KOi3 Expanded subkeys
> + * \param[in] KIi1 Expanded subkeys
> + * \param[in] KIi2 Expanded subkeys
> + * \param[in] KIi3 Expanded subkeys
> + * \returns processed block of 64 bits
> + */

We usually put the documentation only in the .c file and not the
header. (except for stuff that's only in the header obviously).

If it's picked up by doxygen all the same then I don't have big
objections against putting it in the header (to be checked though) if
you insist, but then be consistent ... you have some of the doc
duplicated for some of the methods


> +uint64_t _kasumi(uint64_t P, const uint16_t *KLi1, const uint16_t *KLi2, const uint16_t *KOi1, const uint16_t *KOi2, const uint16_t *KOi3, const uint16_t *KIi1, const uint16_t *KIi2, const uint16_t *KIi3);

Either the methods are meant to be part of the public API and then
they should carry the osmo prefix.
Or they're meant to be internal, then the symbol should not be
exported in the map file and the header should not be installed.


> +inline int _compare_mem(uint8_t * x, uint8_t * y, size_t len)
> +{
> +       if (0 != memcmp(x, y, len)) {
> +               printf ("X: %s\t", osmo_hexdump_nospc(x, len));
> +               printf ("Y: %s\n", osmo_hexdump_nospc(y, len));
> +               return 0;
> +       }
> +       return 1;
> +}

Not of much importance here I admit (and not a blocker), but for
future reference, make sure all that can be static is marked static.
'inline' doesn't make it static so you'll end up with both the
function being inlined at every use in the file and a symbol
_compare_mem that can be called from other .o (and obviously won't be
and so the linker will strip it but still it's a good habit to take).


Cheers,

   Sylvain




More information about the baseband-devel mailing list