nanocoap: add coap_build_reply_header()#20284
Conversation
github-actions
bot
added
Area: network
benpicco
force-pushed
the
coap_build_reply_header
branch
from
c561699 to
7c4faab
Compare
mguetschow
left a comment
mguetschow
left a comment
There was a problem hiding this comment.
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.
From the coap_build_hdr docs:
Caller must ensure
hdrcan 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.
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.
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.
benpicco
force-pushed
the
coap_build_reply_header
branch
2 times, most recently
from
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.
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.
mguetschow
left a comment
mguetschow
left a comment
There was a problem hiding this comment.
LGTM then, thanks for bearing with me!
benpicco
force-pushed
the
coap_build_reply_header
branch
from
668a1d9 to
c71b5ae
Compare
|
Thank you for the review! |
benpicco
added
the
CI: ready for build
4 participants
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