-
Notifications
You must be signed in to change notification settings - Fork 2k
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
nanocoap: add coap_build_reply_header() #20284
Conversation
c561699
to
7c4faab
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like a nice addition to me, but I'm maybe not experienced enough with CoAP and its RIOT implementation to judge whether it adds the right abstraction layer.
Some comments on the code follow.
? COAP_TYPE_ACK | ||
: COAP_TYPE_NON; | ||
|
||
bufpos += coap_build_hdr(buf, type, coap_get_token(pkt), coap_get_token_len(pkt), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From the coap_build_hdr
docs:
Caller must ensure
hdr
can hold the header and the full token!
You also don't check whether buf
is large enough to hold the content type (format) option. *payload_len_max
might underflow.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You also don't check whether buf is large enough to hold the content type (format) option.
Our CoAP API is unfortunately kind of insane in that it expects you to write to a buffer and then check after the fact that it overflowed.
There is no way to detect if the result of coap_put_option_ct()
will fit into the supplied buffer beforehand.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Our CoAP API is unfortunately kind of insane in that it expects you to write to a buffer and then check after the fact that it overflowed.
Oh wow, that's crazy o.O Any plans for this to be fixed on our roadmap?
There is no way to detect if the result of
coap_put_option_ct()
will fit into the supplied buffer beforehand.
According to the RFC it can at most be 2B long if I read it correctly.
8930092
to
5d6ae60
Compare
? COAP_TYPE_ACK | ||
: COAP_TYPE_NON; | ||
|
||
bufpos += coap_build_hdr(buf, type, coap_get_token(pkt), coap_get_token_len(pkt), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Our CoAP API is unfortunately kind of insane in that it expects you to write to a buffer and then check after the fact that it overflowed.
Oh wow, that's crazy o.O Any plans for this to be fixed on our roadmap?
There is no way to detect if the result of
coap_put_option_ct()
will fit into the supplied buffer beforehand.
According to the RFC it can at most be 2B long if I read it correctly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM then, thanks for bearing with me!
668a1d9
to
c71b5ae
Compare
Thank you for the review! |
Contribution description
The
coap_build_reply()
function is kind of awkward to use.coap_reply_simple()
is nicer, but always requires keeping a separate buffer for the payload that then gets copied into the reply buffer anyway.Add a new function
coap_build_reply_header()
that covers all uses ofcoap_build_reply()
but is simpler to use: It builds the response header in the response buffer and sets the payload pointer to after the header, so a user can write the payload directly into the response buffer without having to copy it.Testing procedure
coap_reply_simple()
has been re-implemented using the new function.Issues/PRs references