net/nanocoap: fix string option separator write handling#10223
Merged
miri64 merged 1 commit intoRIOT-OS:masterfrom Nov 22, 2018
Merged
net/nanocoap: fix string option separator write handling#10223miri64 merged 1 commit intoRIOT-OS:masterfrom
miri64 merged 1 commit intoRIOT-OS:masterfrom
Conversation
haukepetersen
requested changes
Nov 21, 2018
Contributor
haukepetersen
left a comment
There was a problem hiding this comment.
The proposed changes are valid and work as far as I can tell (after some testing).
Just this one question on optimization of the code, else I think the PR is ready to be merged.
haukepetersen
approved these changes
Nov 22, 2018
Contributor
haukepetersen
left a comment
There was a problem hiding this comment.
ACK, please squash
Assumed initial character was a separator when writing the option, and skipped over it.
081b640 to
fa77929
Compare
Member
Author
|
Yay, our automated overlords have been appeased! I will rebase #9156 after the merge. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Contribution description
Both the nanocoap
coap_opt_put_string()andcoap_opt_add_string()functions assume the first character in the string is a separator, and skip over it when writing the (first) option. This behavior likely resulted from our initial handling of Path strings. For a path we usually send the full concatenated path as a single string, which always starts with a '/', at least for a Uri-Path option.However, for a Uri-Query we usually send each query individually, for example in the
cord_common_add_qstring()function, and we don't expect to start the query key with an '&'.The error is not expressed currently in the cord module because gcoap uses an older mechanism to write a query option. I'm trying to update how gcoap writes options in some other work, and encountered the problem.
Testing procedure
Probably the easiest way to test is to copy/paste the included unit test, test_nanocoap__get_query() into the tests-nanocoap unit test in master. It tries to add the query string 'ab=cde'. You'll see the test fails because the length of the option is short by one. It neglects to write the first character in the key, so the option contains 'b=cde'.