Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Signed integer property queries #1493

Open
fpagliughi opened this issue Jun 13, 2024 · 7 comments
Open

Signed integer property queries #1493

fpagliughi opened this issue Jun 13, 2024 · 7 comments
Milestone

Comments

@fpagliughi
Copy link
Contributor

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
Copy link
Contributor

icraggs commented Jul 9, 2024

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
Copy link
Contributor

icraggs commented Jul 12, 2024

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
Copy link
Contributor Author

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
Copy link
Contributor Author

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
Copy link
Contributor

icraggs commented Jul 13, 2024

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 icraggs added this to the 1.3.14 milestone Jul 13, 2024
@icraggs
Copy link
Contributor

icraggs commented Jul 13, 2024

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
Copy link
Contributor Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants