Skip to content

Fix subscribe qos fixed header value#272

Closed
picaoao wants to merge 1 commit intoprocessone:developfrom
picaoao:fix-subscribe-qos-fixed-header
Closed

Fix subscribe qos fixed header value#272
picaoao wants to merge 1 commit intoprocessone:developfrom
picaoao:fix-subscribe-qos-fixed-header

Conversation

@picaoao
Copy link
Copy Markdown

@picaoao picaoao commented Dec 7, 2017

MQTT protocol states that qos must be set to 1 in the fixed header of the subscribe frame. Although it was not being a problem when using with emqttd, vernemq was not accepting the frame.

MQTT protocol states that qos must be set to 1 in the fixed header of the subscribe frame.
Although it was not being a problem when using with emqttd, vernemq was not accepting the frame.
@picaoao picaoao force-pushed the fix-subscribe-qos-fixed-header branch from 13082ac to 8724ded Compare December 7, 2017 13:56
@tisba
Copy link
Copy Markdown
Collaborator

tisba commented Jan 19, 2018

@picaoao do you have a reference for that?

I'm not an mqtt expert, but reading http://docs.oasis-open.org/mqtt/mqtt/v3.1.1/os/mqtt-v3.1.1-os.html#_Toc384800437 it looks like the fixed header is indeed fixed and does not contain the QoS flag at all.

Don't get me wrong, I'm just curious to understand the protocol a bit better :)

@tisba tisba changed the base branch from master to develop June 11, 2018 10:47
@tisba
Copy link
Copy Markdown
Collaborator

tisba commented Jun 11, 2018

@picaoao FYI I just changed the target branch to develop.

@nniclausse This seems to be a simple change, but I'm not very familiar with MQTT. Are you? Or do you know how we can figure out a reference that this changes makes sense?

@nniclausse
Copy link
Copy Markdown
Contributor

@tisba: looking at the mqtt_frame code, it seems that the fixed_header built by tsung is indeed wrong. So this patch may fix the problem, but by side effect.
encode_fixed_header should handle subscribe packets differently. The second byte is fixed and always 0010, dup, retain and qos are meaningless in this case.

@tisba
Copy link
Copy Markdown
Collaborator

tisba commented Aug 22, 2018

#327 also addresses the changes in this PR and in addition fixes the subscribe control packet as @nniclausse also pointed out.

I'm closing this in favour of #327.

@tisba tisba closed this Aug 22, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants