Simon Josefsson simon@josefsson.org writes:
It is a hash function, just not a cryptographic hash function.
Below is an updated patch. What do you think?
Looks pretty good. I'm not sure about naming.
Besides that, a few minor comments. Some of them boil downto simply inlining current _salsa20 into salsa20_core, and have salsa20_hash call salsa20_core rather than _salsa20. Do you see any drawbacks with that?
+static void +_salsa20 (unsigned rounds,
uint32_t *x)
No need to use _ in the name of a static-declared function.
- unsigned i;
- for (i = 0; i < rounds; i += 2)
- {
QROUND(x[0], x[4], x[8], x[12]);QROUND(x[5], x[9], x[13], x[1]);QROUND(x[10], x[14], x[2], x[6]);QROUND(x[15], x[3], x[7], x[11]);QROUND(x[0], x[1], x[2], x[3]);QROUND(x[5], x[6], x[7], x[4]);QROUND(x[10], x[11], x[8], x[9]);QROUND(x[15], x[12], x[13], x[14]);- }
Any reason not to do the final addition here? Would need separate input and output arguments, and be organized as
x = input (memcpy) do the rounds dst = input + x (loop)
We need a single temporary array somewhere if we want to allow in-place operation, and this may be that right place. And then we have implemented all of salsa20_core.
+void +salsa20_hash (unsigned rounds,
uint8_t *dst,const uint8_t *src)+{
- uint32_t x[SALSA20_INPUT_LENGTH];
- unsigned i;
- for (i = 0; i < SALSA20_INPUT_LENGTH; i++)
x[i] = LE_READ_UINT32(&src[i * 4]);- _salsa20 (rounds, x);
- for (i = 0; i < SALSA20_INPUT_LENGTH; i++)
- {
uint32_t t = x[i] + LE_READ_UINT32(&src[i * 4]);
Seems unnecessary to convert (using LE_READ_UINT32) the same data twice.
+void +salsa20_core (unsigned rounds,
uint32_t *dst,const uint32_t *src)+{
- uint32_t x[SALSA20_INPUT_LENGTH];
- unsigned i;
- for (i = 0; i < SALSA20_INPUT_LENGTH; i++)
- x[i] = src[i];
This should be plain memcpy.
Regards, /Niels