Signed integer property queries
fpagliughi opened this issue · 7 comments
On the Paho C++ repo, someone just pointed out that the 2 and 4-byte integer property values are defined to always be unsigned. (According to the v5 spec). Paho C++ #498 .
Somehow the "always unsigned" part escaped me these last few years.
In this C library it is coded correctly in the enum inside MQTTProperty, but the retrieval functions return an int, which is 32-bits on most modern PC compilers.
/**
* Returns the integer value of a specific property. The property given must be a numeric type.
* ...
* @return the integer value of the property. -9999999 on failure.
*/
LIBMQTT_API int MQTTProperties_getNumericValue(MQTTProperties *props, enum MQTTPropertyCodes propid);
/**
* Returns the integer value of a specific property when it's not the only instance.
* ...
* @return the integer value of the property. -9999999 on failure.
*/
LIBMQTT_API int MQTTProperties_getNumericValueAt(MQTTProperties *props, enum MQTTPropertyCodes propid, int index);
This means that larger 4-byte values will be misinterpreted as negative numbers, which is kinda bad. But it also means that the "error" return value of -9999999 falls well within the range of valid possible values, and in fact, any 32-bit int value could be valid.
Shouldn't these functions actually return a long or - better yet - an int64_t value where anything >= 0 would be valid, and anything <0 is an error? You could still use -9999999, or make it -1, maybe.
I guess, technically, this would be a minor breaking change in the API, but not one that would probably break a lot of builds. And it would be worth it to fix the issue.
Should definitely be int64_t rather than long or the traditional types so that its always the correct size on whatever architecture. I'll take a look to see if it needs to go into 1.4, rather than 1.3.x. What would the Rust client think of a type change - any ramifications there?
There are two issues 1) the sign/size of the integer and 2) error reporting.
To allow the code to work on 16 bit systems, the 4 byte fields should be (unsigned) long or uint32_t (at least).
The error reporting would be better done as another parameter - something that wasn't part of the return space.
64 bit minimum would be long long. If we went to uint64_t for now, that would mean changing again for the ideal solution of returning uint32_t and a different error return. That leaves me in favour of doing it properly in 1.4 and leaving 1.3.x for now.
Yes, if we want to have the full range of the returned value (u32) and room for an error value, then we need a type larger than u32.
I would lean toward returning an int64_t where >= 0 is a valid return and -1 is an error. That seems traditional. But then I guess we could keep the -9999999, and just say any <0 is an error.
All that would require is changing the return type from int to int64_t.
As an aside... 16-bit system?!? Are you still testing on Windows 95?!? Hahaha.
Also... I would vote to fix this in 1.3.x if 1.4 is still a while to be release. I'm planning releases for Paho C++ and Rust based on the next 1.3.x, from the develop branch, and it would be nice to have this resolved.
Although, honestly, I agree this isn't a really big deal. The chance of someone having a session expiry interval of exactly 4284967297 seconds (-9999999 as a uint32_t) is probably pretty slim.
There are some people who ask about 16 bit systems :-) No point in deliberately not making it work, but it's not something I thought about previously.
I've put in the change to int64_t for the return value from the functions:
MQTTProperties_getNumericValueAt
MQTTProperties_getNumericValue
I didn't get any build warnings so I think it's good. I might open another issue for 1.4 to ensure that the 4 byte values don't use int - for 16 byte systems.
Nice! Yeah, I'm trying to more precise with the integer sizing lately; using the sized types when I know the required size. uint32_t, etc, rather than int, unsigned, long.