Re: Small patch for libicmp/icmp_cksum.c

[Paul Jarc]

address@hidden wrote:
>> * This won't have the same effect as the original code on a big-endian
>>   system.
>
> Could you point out what exactly I broke?

Suppose wp points to the byte 42.  The old code:
     *(u_char *)&answer = *(u_char*)wp;
     sum += answer;
So *(u_char*)wp is a u_char with the value 42.  This is copied to the
byte (u_char) at the address &answer, which is the first byte of the
answer object.  The second byte of answer is zero, due to the earlier
initialization.  On a big-endian system, the first byte is the high
byte, so (assuming u_short is two 8-bit bytes) the value in answer is
now 42*256=10752.  That value is added to sum.

Your new code:
   sum += (unsigned short)*(u_char *)w;
As before, *(u_char*)wp is a u_char with the value 42.  Casting (to
unsigned short, in this case) only changes the type of this entity,
not the value.  (Avoid the trap of thinking that a cast reinterprets
the bits as if they were an object of a different type; it doesn't.)
So this only adds 42 to sum.

> I played with the original function in BSD ping, then wondered what
> GNU ping uses and found the same function.  At least it looked the
> same to me,

I'm not familiar with the BSD code.  I can say that your patch
definitely introduces behavior different from that of the current GNU
code.  But if yours is the same as BSD's, it's possible that BSD's is
correct and the existing GNU behavior is buggy.  (Or perhaps that
either behavior would work.)

> so I sent the patch as a suggestion to get rid of two (in my eyes)
> unneccesary variable assignments.

The one at the end is indeed unnecessary; a cast will have the same
effect as the assignment in that case.  (And the cast is also
unnecessary, since it is implicit because of the function's return
type.)  But it may still be worth keeping for the sake of readability;
that's up to the inetutils maintainers.


paul