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

gcoap: accept resources in any order #18651

Merged
merged 2 commits into from Sep 28, 2022
Merged

Conversation

benpicco
Copy link
Contributor

Contribution description

coap_match_path() returns the result of strcmp, so 0 is a match, non-zero give the order of the two strings.

For some reason GCoAP things the order is important - which leads to surprising results.

E.g. if you have

static const coap_resource_t _resources[] = {
    { "/fw", COAP_GET | COAP_MATCH_SUBTREE, gcoap_fileserver_handler, SUIT_FW_DIR },
    { "/cmd", COAP_PUT, _coap_cmd_handler, NULL },
    { "/log", COAP_PUT, _coap_log_handler, NULL },
};

The /cmd endpoint will always return 404.

Testing procedure

Issues/PRs references

@github-actions github-actions bot added Area: CoAP Area: Constrained Application Protocol implementations Area: network Area: Networking Area: sys Area: System labels Sep 26, 2022
@benpicco benpicco requested a review from kb2ma September 26, 2022 18:07
@maribu
Copy link
Member

maribu commented Sep 26, 2022

Does the API doc need an update?

@benpicco benpicco added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Sep 26, 2022
@kaspar030
Copy link
Contributor

kaspar030 commented Sep 26, 2022

we could also check the order at the time the listener is registered, warn if it is not ordered, remember that and conditionally break. but hey, it won't matter until the number of endpoints gets larger than what we're doing now.

@benpicco
Copy link
Contributor Author

Also changed it for coap_tree_handler()

@bergzand
Copy link
Member

Is there a test where we can shuffle the resources around a bit?

@maribu maribu added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Sep 27, 2022
@benpicco benpicco merged commit e9b5bd7 into RIOT-OS:master Sep 28, 2022
@benpicco benpicco deleted the gcoap-footgun branch September 28, 2022 08:48
@maribu maribu added this to the Release 2022.10 milestone Oct 14, 2022
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.

None yet

4 participants