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

sys/net/gcoap: get rid of API abuse #21125

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

maribu
Copy link
Member

@maribu maribu commented Jan 9, 2025

Contribution description

Calling coap_get_token() and coap_get_token_length() on an (mostly) uninitialized coap_pkt_t did work so far due to implementation details matching the expectations, but this is not backed up by any API contract.

This fixes the API abuse by introducing and using a new API that does read a token and token length from a CoAP over UDP packet out of a buffer. This now provides the behavior expected by the caller and commits to it via API contract.

Testing procedure

No change in behavior when using GCoAP.

Issues/PRs references

Not abusing the API by calling an (mostly) unitialized coap_pkt_t is useful for adding other CoAP formats (e.g. as used for CoAP over TCP and CoAP over WebSocket), as coap_get_token() will look into a different location depending on the format: For that, adding information needed to deduce the CoAP format to the coap_pkt_t allows consistent use of coap_get_token().

@github-actions github-actions bot added Area: network Area: Networking Area: CoAP Area: Constrained Application Protocol implementations Area: sys Area: System labels Jan 9, 2025
@mguetschow mguetschow added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Jan 10, 2025
@maribu maribu force-pushed the sys/net/gcoap/fix-api-abuse branch from f64472c to cbd44bb Compare January 10, 2025 16:23
@maribu
Copy link
Member Author

maribu commented Jan 10, 2025

I was so free to squash directly

@mguetschow
Copy link
Contributor

Sure! There's actually still the same problem in the other doc comments.

@maribu maribu force-pushed the sys/net/gcoap/fix-api-abuse branch from cbd44bb to b7a8b01 Compare January 10, 2025 16:25
sys/include/net/nanocoap.h Outdated Show resolved Hide resolved
sys/include/net/nanocoap.h Outdated Show resolved Hide resolved
sys/include/net/nanocoap.h Outdated Show resolved Hide resolved
@maribu
Copy link
Member Author

maribu commented Jan 10, 2025

Wait, there actually already is coap_hdr_get_token_len().

@maribu maribu force-pushed the sys/net/gcoap/fix-api-abuse branch from 178cb6a to 20d7bd3 Compare January 10, 2025 16:45
@maribu maribu force-pushed the sys/net/gcoap/fix-api-abuse branch from 14d3575 to 482a91d Compare January 10, 2025 20:10
@riot-ci
Copy link

riot-ci commented Jan 10, 2025

Murdock results

FAILED

590ca5d sys/net/gcoap: get rid of API abuse

Success Failures Total Runtime
9142 0 10128 13m:52s

Artifacts

Calling `coap_get_token()` and `coap_get_token_length()` on an
(mostly) uninitialized `coap_pkt_t` did work so far due to
implementation details matching the expectations, but this is not
backed up by any API contract.

This fixes the API abuse by introducing and using a new API that does
read a token and token length from a CoAP over UDP packet out of a
buffer. This now provides the behavior expected by the caller and
commits to it via API contract.

Co-authored-by: mguetschow <[email protected]>
Co-authored-by: benpicco <[email protected]>
@maribu maribu force-pushed the sys/net/gcoap/fix-api-abuse branch from 482a91d to 590ca5d Compare January 10, 2025 20:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: CoAP Area: Constrained Application Protocol implementations Area: network Area: Networking Area: sys Area: System CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants