Hi
there is a function generating checksum:

unsigned short foo (unsigned short *addr, int len) {
unsigned short result;
unsigned int sum = 0;

while (len > 1) {
sum += *addr++;
len -= 2;
}

if (len == 1) {
sum += *(unsigned char *) addr;
}
// some other stuff
}

which I suppose to be called like
unsigned short addr = 0xabcd;
foo(&a, 5);

having unsigned short size equals 2 bytes
I don't understand why the function is going to iterate over other bytes more than 2. Where could those extra bytes come from?

On 20/08/2023 11:46, Denis U wrote:
> Hi
> there is a function generating checksum:
>
> unsigned short foo (unsigned short *addr, int len) {
> unsigned short result;
> unsigned int sum = 0;
>
> while (len > 1) {
> sum += *addr++;
> len -= 2;
> }
>
> if (len == 1) {
> sum += *(unsigned char *) addr;
> }
> // some other stuff
> }
>
> which I suppose to be called like
> unsigned short addr = 0xabcd;
> foo(&a, 5);
>
> having unsigned short size equals 2 bytes
> I don't understand why the function is going to iterate over other bytes more than 2. Where could those extra bytes come from?
>
>

The checksum is not for a 2-byte short - it is for checksuming over an
array of 2-byte short values. Checksums are typically used for
integrity checks for data that is stored or transmitted on networks,
serial lines, etc. The idea is that you transmit the data and the
checksum, and on reception you can re-calculate the checksum from the
received data and compare to the received checksum. If there is a
mismatch, something has been corrupted in the transfer - if there is a
match, the transfer was probably fine.

So you would call "foo" as :

unsigned short data[] = { 0xabcd, 0x1234, 0x9421 };
unsigned short cs = foo(data, 6);

David Brown <david.brown@hesbynett.no> writes:

> On 20/08/2023 11:46, Denis U wrote:
>> Hi
>> there is a function generating checksum:
>> unsigned short foo (unsigned short *addr, int len) {
>> unsigned short result;
>> unsigned int sum = 0;
>> while (len > 1) {
>> sum += *addr++;
>> len -= 2;
>> }
>> if (len == 1) {
>> sum += *(unsigned char *) addr;
>> }
>> // some other stuff
>> }
>> which I suppose to be called like
>> unsigned short addr = 0xabcd;
>> foo(&a, 5);
>> having unsigned short size equals 2 bytes
>> I don't understand why the function is going to iterate over other bytes more than 2. Where could those extra bytes come from?
>>
>
> The checksum is not for a 2-byte short - it is for checksuming over an
> array of 2-byte short values.

Given the check for an odd length, it's clearly intended to produce a
16-bit checksum for an arbitrary object of any length -- not just for
some array of short values.

But if that is the case (and what's the extra test for otherwise) then
it's a badly written. short is not a predictable length (but the
function may date from before the days of uint16_t) and there are
alignment issues.

To the OP: where is this from? Are you supposed to be correcting it, or
is this supposed to be an example of good C from which you should learn?

Consider using something like this:

uint_least16_t checksum(const char *addr, size_t len)
{
uint_fast16_t sum = 0;
while (len > 1) {
uint16_t tmp;
memcpy(&tmp, addr, sizeof tmp);
sum += tmp;
len -= 2;
}
return sum + (len ? *(unsigned char *)addr : 0);
}

--
Ben.

On 20/08/2023 15:39, Ben Bacarisse wrote:
> David Brown <david.brown@hesbynett.no> writes:
>
>> On 20/08/2023 11:46, Denis U wrote:
>>> Hi
>>> there is a function generating checksum:
>>> unsigned short foo (unsigned short *addr, int len) {
>>> unsigned short result;
>>> unsigned int sum = 0;
>>> while (len > 1) {
>>> sum += *addr++;
>>> len -= 2;
>>> }
>>> if (len == 1) {
>>> sum += *(unsigned char *) addr;
>>> }
>>> // some other stuff
>>> }
>>> which I suppose to be called like
>>> unsigned short addr = 0xabcd;
>>> foo(&a, 5);
>>> having unsigned short size equals 2 bytes
>>> I don't understand why the function is going to iterate over other bytes more than 2. Where could those extra bytes come from?
>>>
>>
>> The checksum is not for a 2-byte short - it is for checksuming over an
>> array of 2-byte short values.
>
> Given the check for an odd length, it's clearly intended to produce a
> 16-bit checksum for an arbitrary object of any length -- not just for
> some array of short values.
>
> But if that is the case (and what's the extra test for otherwise) then
> it's a badly written. short is not a predictable length (but the
> function may date from before the days of uint16_t) and there are
> alignment issues.

I agree entirely - I just wanted to keep things simple at the start, and
then see where the OP wanted to go from there.

>
> To the OP: where is this from? Are you supposed to be correcting it, or
> is this supposed to be an example of good C from which you should learn?
>
> Consider using something like this:
>
> uint_least16_t checksum(const char *addr, size_t len)
> {
> uint_fast16_t sum = 0;
> while (len > 1) {
> uint16_t tmp;
> memcpy(&tmp, addr, sizeof tmp);
> sum += tmp;
> len -= 2;
> }
> return sum + (len ? *(unsigned char *)addr : 0);
> }
>

If someone is learning C, I'd assume they are interested in more
practical C. Types like "uint_least16_t" have theoretical advantages
over the fixed size types, but are almost never seen in real-world C
code. If the OP is interested in C as a language and knowing more about
it, then more detail will be appropriate. Even then, I'd question the
appropriateness of returning uint_least16_t here. (And did you skip
incrementing addr as a debugging exercise for the OP? :-) )

uint16_t checksum_16bit_le(const uint8_t *addr, size_t len)
{ uint16_t sum = 0;
while (len > 1) {
uint16_t tmp = *addr++;
tmp += (*addr++) << 8;
sum += tmp;
len -= 2;
}
if (len) {
sum += *addr;
}
return sum;
}

This version gives the same checksum on little-endian and big-endian
processors, unlike the original one - but that is almost certainly what
you want for a checksum, as the checksum algorithm is usually defined
independently of the system it is run on.

And if the OP wants to know about good checksum algorithms, this is not
a good one.

On Sunday, 20 August 2023 at 14:39:34 UTC+1, Ben Bacarisse wrote:
> David Brown <david...@hesbynett.no> writes:
>
> > On 20/08/2023 11:46, Denis U wrote:
> >> Hi
> >> there is a function generating checksum:
> >> unsigned short foo (unsigned short *addr, int len) {
> >> unsigned short result;
> >> unsigned int sum = 0;
> >> while (len > 1) {
> >> sum += *addr++;
> >> len -= 2;
> >> }
> >> if (len == 1) {
> >> sum += *(unsigned char *) addr;
> >> }
> >> // some other stuff
> >> }
> >> which I suppose to be called like
> >> unsigned short addr = 0xabcd;
> >> foo(&a, 5);
> >> having unsigned short size equals 2 bytes
> >> I don't understand why the function is going to iterate over other bytes more than 2. Where could those extra bytes come from?
> >>
> >
> > The checksum is not for a 2-byte short - it is for checksuming over an
> > array of 2-byte short values.
> Given the check for an odd length, it's clearly intended to produce a
> 16-bit checksum for an arbitrary object of any length -- not just for
> some array of short values.
>
> But if that is the case (and what's the extra test for otherwise) then
> it's a badly written. short is not a predictable length (but the
> function may date from before the days of uint16_t) and there are
> alignment issues.
>
> To the OP: where is this from? Are you supposed to be correcting it, or
> is this supposed to be an example of good C from which you should learn?
>
> Consider using something like this:
>
> uint_least16_t checksum(const char *addr, size_t len)
> {
> uint_fast16_t sum = 0;
> while (len > 1) {
> uint16_t tmp;
> memcpy(&tmp, addr, sizeof tmp);
> sum += tmp;
> len -= 2;
> }
> return sum + (len ? *(unsigned char *)addr : 0);
> }
>
unsigned short checksum(const unsigned char *addr, int len)
{ unsigned short sum = 0;
int i;
for ( i = 0; i <len-1; i+ 2)
{
sum += addr[i+1] * 256 + addr[i];
}
if (i < len)
sum += addr[i];

return sum & 0xFFFF;
}

The question is what to do for endiannness.
Likely you really want the same result, regardless of the endianness of the platform.
And adding the last byte as a low value implies little-endian.

On Sun, 20 Aug 2023 07:20:07 -0700 (PDT)
Malcolm McLean <malcolm.arthur.mclean@gmail.com> wrote:
> unsigned short checksum(const unsigned char *addr, int len)
> {
> unsigned short sum = 0;
> int i;
> for ( i = 0; i <len-1; i+ 2)

i += 2

> {
> sum += addr[i+1] * 256 + addr[i];
> }
> if (i < len)
> sum += addr[i];
>
> return sum & 0xFFFF;
> }

Regarding unsigned short sum = 0;

You want unsigned int not unsigned short ; the latter may be promoted
to int for the arithmetic in which case your code will likely cause
undefined behaviour. For the same reason you want

sum += (unsigned int) addr[i+1] * 256 + addr[i];

> The question is what to do for endiannness. Likely you really want the same
> result, regardless of the endianness of the platform. And adding the last
> byte as a low value implies little-endian.

Ben Bacarisse <ben.usenet@bsb.me.uk> writes:

> David Brown <david.brown@hesbynett.no> writes:
>
>> On 20/08/2023 11:46, Denis U wrote:
>>
>>> Hi
>>> there is a function generating checksum:
>>> unsigned short foo (unsigned short *addr, int len) {
>>> unsigned short result;
>>> unsigned int sum = 0;
>>> while (len > 1) {
>>> sum += *addr++;
>>> len -= 2;
>>> }
>>> if (len == 1) {
>>> sum += *(unsigned char *) addr;
>>> }
>>> // some other stuff
>>> }
>>> which I suppose to be called like
>>> unsigned short addr = 0xabcd;
>>> foo(&a, 5);
>>> having unsigned short size equals 2 bytes
>>> I don't understand why the function is going to iterate over other
>>> bytes more than 2. Where could those extra bytes come from?
>>
>> The checksum is not for a 2-byte short - it is for checksuming over an
>> array of 2-byte short values.
>
> Given the check for an odd length, it's clearly intended to produce a
> 16-bit checksum for an arbitrary object of any length -- not just for
> some array of short values.
>
> But if that is the case (and what's the extra test for otherwise) then
> it's a badly written. short is not a predictable length (but the
> function may date from before the days of uint16_t) and there are
> alignment issues.
>
> To the OP: where is this from? Are you supposed to be correcting it, or
> is this supposed to be an example of good C from which you should learn?
>
> Consider using something like this:
>
> uint_least16_t checksum(const char *addr, size_t len)
> {
> uint_fast16_t sum = 0;
> while (len > 1) {
> uint16_t tmp;
> memcpy(&tmp, addr, sizeof tmp);
> sum += tmp;
> len -= 2;
> }
> return sum + (len ? *(unsigned char *)addr : 0);
> }

Bleah.

unsigned short
checksum( const char *bytes, size_t n ){
unsigned short t;
size_t k = sizeof t;
unsigned r;

for( r = 0; n >= k; bytes += k, n -= k ){
memcpy( &t, bytes, k );
r += t;
}

if( n > 0 ){
t = 0;
memcpy( &t, bytes, n );
r += t;
}

return r;
}

David Brown <david.brown@hesbynett.no> writes:

> On 20/08/2023 15:39, Ben Bacarisse wrote:
>> David Brown <david.brown@hesbynett.no> writes:
>>
>>> On 20/08/2023 11:46, Denis U wrote:
>>>> Hi
>>>> there is a function generating checksum:
>>>> unsigned short foo (unsigned short *addr, int len) {
>>>> unsigned short result;
>>>> unsigned int sum = 0;
>>>> while (len > 1) {
>>>> sum += *addr++;
>>>> len -= 2;
>>>> }
>>>> if (len == 1) {
>>>> sum += *(unsigned char *) addr;
>>>> }
>>>> // some other stuff
>>>> }
>>>> which I suppose to be called like
>>>> unsigned short addr = 0xabcd;
>>>> foo(&a, 5);
>>>> having unsigned short size equals 2 bytes
>>>> I don't understand why the function is going to iterate over other bytes more than 2. Where could those extra bytes come from?
>>>>
>>>
>>> The checksum is not for a 2-byte short - it is for checksuming over an
>>> array of 2-byte short values.
>> Given the check for an odd length, it's clearly intended to produce a
>> 16-bit checksum for an arbitrary object of any length -- not just for
>> some array of short values.
>> But if that is the case (and what's the extra test for otherwise) then
>> it's a badly written. short is not a predictable length (but the
>> function may date from before the days of uint16_t) and there are
>> alignment issues.
>
> I agree entirely - I just wanted to keep things simple at the start, and
> then see where the OP wanted to go from there.

I suspect it's a drive-by posting.

>> To the OP: where is this from? Are you supposed to be correcting it, or
>> is this supposed to be an example of good C from which you should learn?
>> Consider using something like this:
>> uint_least16_t checksum(const char *addr, size_t len)
>> {
>> uint_fast16_t sum = 0;
>> while (len > 1) {
>> uint16_t tmp;
>> memcpy(&tmp, addr, sizeof tmp);
>> sum += tmp;
>> len -= 2;
>> }
>> return sum + (len ? *(unsigned char *)addr : 0);
>> }
>>
>
> If someone is learning C, I'd assume they are interested in more practical
> C. Types like "uint_least16_t" have theoretical advantages over the fixed
> size types, but are almost never seen in real-world C code. If the OP is
> interested in C as a language and knowing more about it, then more detail
> will be appropriate. Even then, I'd question the appropriateness of
> returning uint_least16_t here. (And did you skip incrementing addr as a
> debugging exercise for the OP? :-) )

No, just tying off the top of my head.

> uint16_t checksum_16bit_le(const uint8_t *addr, size_t len)
> {
> uint16_t sum = 0;
> while (len > 1) {
> uint16_t tmp = *addr++;
> tmp += (*addr++) << 8;
> sum += tmp;
> len -= 2;
> }
> if (len) {
> sum += *addr;
> }
> return sum;
> }
>
>
> This version gives the same checksum on little-endian and big-endian
> processors, unlike the original one - but that is almost certainly what you
> want for a checksum, as the checksum algorithm is usually defined
> independently of the system it is run on.

There's no specification. It's all guesswork about what sort of
checksum this is. I agree they are often externally defined, but they
are never externally definedto be what the OP's code implements, so who
knows what's wanted?

--
Ben.

On 20/08/2023 17:07, Ben Bacarisse wrote:

> There's no specification. It's all guesswork about what sort of
> checksum this is. I agree they are often externally defined, but they
> are never externally definedto be what the OP's code implements, so who
> knows what's wanted?
>

I guess we will just have to wait and see if he comes back - then we'll
hopefully know much more.

David Brown <david.brown@hesbynett.no> writes:

> On 20/08/2023 17:07, Ben Bacarisse wrote:
>
>> There's no specification. It's all guesswork about what sort of
>> checksum this is. I agree they are often externally defined, but they
>> are never externally definedto be what the OP's code implements, so who
>> knows what's wanted?
>
> I guess we will just have to wait and see if he comes back - then we'll
> hopefully know much more.

I'd like to know the context. Is it "the teacher gave up this function
to use" or is it "I work for the MOD on missile control systems and I
have to write a checksum function"?

--
Ben.