Signed integer property queries

[fpagliughi]

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.

[icraggs]

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?

[icraggs]

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.

[fpagliughi]

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.

[fpagliughi]

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.

[icraggs]

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.

[icraggs]

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.

[fpagliughi]

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.