Wed 26 Jul 2017 10:56:43 AM UTC, comment #8:
Fixed using Art's patch
|
Wed 26 Jul 2017 07:31:24 AM UTC, comment #7:
Pushed Art's patch
Wait for feedback on COUNTER64 before closing this - I'm sure it'll be negative, because pointer++ on an u32_t* won't work for 64 bit values on big endian machines :-)
|
Wed 26 Jul 2017 06:50:27 AM UTC, comment #6:
Thanks for the patch, we'll test it.
While reading the code, I found that COUNTER64 probably doesn't work for big endian, too. However, this is unimplemented yet, so not easy to test.
If you find the time, I'd be happy if you could test COUNTER64 by applying this patch:
(file #41338)
|
Tue 25 Jul 2017 10:56:56 PM UTC, comment #5:
A patch with no reference to byte order: tested on a big endian machine.
/**
- Decodes integer into s32_t.
*
- @param pbuf_stream points to a pbuf stream
- @param len length of the coded integer field
- @param value return host order integer
- @return ERR_OK if successful, ERR_ARG if we can't (or won't) decode
*
- @note ASN coded integers are always signed!
*/
err_t
snmp_asn1_dec_s32t(struct snmp_pbuf_stream pbuf_stream, u16_t len, s32_t value)
{
u8_t data;
if ((len > 0) && (len < 5)) {
PBUF_OP_EXEC(snmp_pbuf_stream_read(pbuf_stream, &data));
if (data & 0x80) {
/* negative, start from -1 */
*value = (-1 << 8) | data;
} else {
/* positive, start from 0 */
*value = data;
}
len--;
/* shift in the remaining value */
while (len > 0) {
PBUF_OP_EXEC(snmp_pbuf_stream_read(pbuf_stream, &data));
value = (value << 8) | data;
len--;
}
return ERR_OK;
}
return ERR_VAL;
}
|
Tue 25 Jul 2017 08:17:01 PM UTC, comment #4:
Art, can you please provide a patch that fixes this?
|
Tue 25 Jul 2017 07:57:41 PM UTC, comment #3:
I'd very much appreciate to have this function changed. There should be no dependencies to BYTE_ORDER unless absolutely necessary!
|
Tue 25 Jul 2017 04:59:34 PM UTC, comment #2:
We (Campbell Scientific) are using the Renesas processors, most recently the RX series, Rx63N, in our measurement and control systems (dataloggers). Over the years we have built up our own operating system and at some point a few years ago gravitated to the LwIP stack. The Renesas processors (originally Hitachi) followed Motorola's big endian model, though their more recent processors can optionally be run as little also. We have stuck to big endian. Renesas does have a development platform that you might want to look into.
As for the code issue, the function could have been written in such a way to avoid checking BYTE_ORDER by simply calculating what value is locally and then passing that value back via the pointer at the bottom of the function. Instead it chose to use a pointer to value and to use that pointer during the creation of the value, one byte at a time, and therefore had to know the byte order of the machine. It would be better, like you suggest to leave out BYTE_ORDER altogether by using a local uint32 localvalue, calculate the value with it, and the at the end do *value = localvalue.
|
Mon 24 Jul 2017 08:03:23 PM UTC, comment #1:
I'd really love to get hold of a big endian test platform! Unfortunately, I only have access to little endian systems. Which platform are you using to check this?
From reading the code I get confused. Shouldn't the host system do the correct thing without checking BYTE_ORDER at all?
|
Thu 20 Jul 2017 09:32:57 PM UTC, original submission:
In this function:
err_t
snmp_asn1_dec_s32t(struct snmp_pbuf_stream pbuf_stream, u16_t len, s32_t value)
this code is wrong for BIG_ENDIAN machines:
#if BYTE_ORDER == LITTLE_ENDIAN
*value <<= 8;
#endif
#if BYTE_ORDER == BIG_ENDIAN
*value >>= 8;
#endif
For both little and big endian, it should be *value <<= 8;
For big endian machines, *value always ends up with only the least significant byte.
|