Re: [PATCH] reiserfscore/hashes.c

[Edward Shishkin <edward.shishkin@gmail.com>]

On 11/07/2012 01:37 AM, Michael W. Bombardieri wrote:
> On Tue, Nov 06, 2012 at 08:47:27PM +0100, Edward Shishkin wrote:
>> Hello Michael,
>>
>> Your changes look OK to me.
>>
>> TBH, I don't like the fact that the pow is set to 1 by such tricky ways in
>> the mentioned nested loops: it might indicate mistakes in the original
>> code.
> I agree. Something doesn't look right about the nested loops
> not being entered. This could well point to the code being
> incorrect. IMHO, the developer would not bother adding the
> loops if he didn't want them to be used.
>
>> Generally I think that this hash is not heavily used: everyone uses either
>> r5, or tea hash, so...
> I suppose this would raise the question, is it worth patching
> yura_hash() or is it better to scrap the function altogether
> if the two other hash functions are sufficient?


We can not scrap this, as it will break backward compatibility:
a number of people must be use it. We can only recommend,
or not recommend something..

I suggest to patch this, so it will be clear what is going on here.
You might want also to prepare similar patch for the kernel and
send it to Jeff.

Thanks,
Edward.


>
>> A??ked-by: Edward Shishkin <edward.shishkin@gmail.com>
>>
>> Thanks,
>> Edward.
>> P.S. This is a git repository of Jeff Mahoney, the reiserfs maintainer, so
>> don't forget cc him ;)
>>
>>
>>
>>> - Michael
>>>
>>>
>>> --- hashes.c	Tue Nov  6 23:21:09 2012
>>> +++ hashes_new.c	Tue Nov  6 23:26:26 2012
>>> @@ -175,37 +175,26 @@
>>>   u32 yura_hash (const signed char *msg, int len)
>>>   {
>>> -    int j, pow;
>>> -    u32 a, c;
>>> -    int i;
>>> -
>>> -    for (pow=1,i=1; i < len; i++) pow = pow * 10;
>>> -
>>> -    if (len == 1)
>>> -	a = msg[0]-48;
>>> -    else
>>> -	a = (msg[0] - 48) * pow;
>>> -
>>> -    for (i=1; i < len; i++) {
>>> -	c = msg[i] - 48;
>>> -	for (pow=1,j=i; j < len-1; j++) pow = pow * 10;
>>> -	a = a + c * pow;
>>> -    }
>>> -
>>> -    for (; i < 40; i++) {
>>> -	c = '0' - 48;
>>> -	for (pow=1,j=i; j < len-1; j++) pow = pow * 10;
>>> -	a = a + c * pow;
>>> -    }
>>> -
>>> -    for (; i < 256; i++) {
>>> -	c = i;
>>> -	for (pow=1,j=i; j < len-1; j++) pow = pow * 10;
>>> -	a = a + c * pow;
>>> -    }
>>> -
>>> -    a = a << 7;
>>> -    return a;
>>> +	int i, j, pow;
>>> +	u32 a;
>>> +
>>> +	a = msg[0] - 48;
>>> +	if (len != 1) {
>>> +		for (i = 1, pow = 1; i < len; i++)
>>> +			pow *= 10;
>>> +		a *= pow;
>>> +	}
>>> +	for (i = 1; i < len; i++) {
>>> +		for (j = i, pow = 1; j < len - 1; j++)
>>> +			pow *= 10;
>>> +		a += pow * (msg[i] - 48);
>>> +	}
>>> +	for (; i < 40; i++)
>>> +		a += '0' - 48;
>>> +	for (; i < 256; i++)
>>> +		a += i;
>>> +	a <<= 7;
>>> +	return a;
>>>   }