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

Add getters for protocol version and name; default to "MQTT" for name #44

Merged
merged 1 commit into from
Nov 12, 2020

Conversation

jeff-hiner
Copy link
Contributor

MQTT broker implementations are required to verify that the protocol name and version are correct before accepting connections, so this PR provides a way to access those fields.

I've also removed a field from ConnectPacket::new that doesn't really make sense: the protocol name for 3.1.1 going forward must always be "MQTT". Users can still use with_level if they want to specify version and name manually (i.e. to use "MQISDP" for a 3.1.0 client, or to provide invalid sequences for broker testing). This is a breaking API change, but updating should be pretty trivial for most users.

Version310 = SPEC_3_1_0,
Version311 = SPEC_3_1_1,
Version50 = SPEC_5_0,
}
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We shouldn't make the protocol level fixed as an enum, it would be nice to define some helper functions like:

impl ProtocolLevel {
    pub fn protocol_3() -> ProtocolLevel {
        ProtocolLevel(0x03)
    }
}

already good enough.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From trying to write a broker I find it's more ergonomic to not even expose those inner const values-- I don't care what the inner u8 is for those and trying to remember which is which gets confusing. I just need to know what protocol version is specified (the number of legal values is very small, and they unfortunately aren't all in a single spec document). It's unfortunately not trivial because the u8 doesn't even map neatly to the MQTT version-- spec 3.1.1 is 0x03, and for some reason they skipped spec 4 and said spec 5.0.0 is ProtocolVersion 0x04 (?) So without this parsing into an enum I'd have to do some sort of lookup by const values. It's just interpretation that I feel works best within this crate rather than the broker.

I could add #[non_exhaustive] to indicate that there may exist more variants than we expose, but in practice virtually every broker and client right now are either 3.1.1 or 5.0.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and for some reason they skipped spec 4 and said spec 5.0.0 is ProtocolVersion 0x04 (?)

https://docs.oasis-open.org/mqtt/mqtt/v5.0/mqtt-v5.0.html#_Toc385349227

Nope. v5.0 is 0x05.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With a bit of a chuckle at my own mistake, I think you just made my point. I don't want to think about this stuff, and the field is essentially a de-facto enumeration of repr u8. Can we just parse it into an enum? It'll make my MQTT broker project a lot easier to write.

@zonyitoo zonyitoo merged commit 87ed144 into zonyitoo:master Nov 12, 2020
@jeff-hiner jeff-hiner deleted the connect_packet branch November 12, 2020 05:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants