nanocoap: add coap_iterate_uri_query() #20197
Conversation
There was a problem hiding this comment.
See added comments on the implementation.
I'm not sure this is the right abstraction level. For those who need all the options, a loop using coap_opt_get_opaque is straightforward enough to do where it is needed. For users who really want to parse this data, in most cases they already know the keys they are looking for (and may even opt to ignore keys they do not know, even though I do not condone that use of query parameters) -- and for those, I think something getopt-style would be more convenient. A la
int32_t value = 3600;
coap_get_uri_query_i32(pkt, "lt", *value);
which would return 0 on OK, or -ENOENT when not found, or something else if there are multiple occurrences, but is still usable in the absence of error handling.
|
Expecting the user to iterate With this we can just call |
|
Slightly off-topic:
Feels a bit odd that riot has a "uri parser" module but this PR adds a function with "uri" and "query" in its name that is not in that module nor does it use functions of that module. Don't get me wrong, I think this PR is fine in this regard - Just makes me wonder a bit.
|
|
On Tue, Dec 19, 2023 at 06:51:43AM -0800, benpicco wrote:
While I like the idea of querying the desired key directly, it would involve iterating the packet multiple times if we are interested in multiple keys.
We're talking about, typically, 3-5 checks that on 1-3 options in a
message, which are already in parsed form for iteration. Does that make
that much of a difference, especially when it saves you the cost of
copying out all the options all the time?
|
The long answer to this is in #13827, which could be a good base for a consistent interface for URIs and locations both on clients and servers, as well as references -- but that's still waiting for the spec to stabilize. |
|
We'd still have to copy the value with (I don't want a separate function for very possible type of value, as value is always a string the function should return a string) |
|
Could you add two tests for the cases of returning In general, the unit tests lack in quality but a refactor is out of scope for this PR. |
8b5bbae to
cbad490
Compare
|
I think even with #20273 this PR is still useful. |
|
Squash'n rebase? |
fe4b00e to
418b668
Compare
|
@Teufelchen1 ping 😉 |
|
Squash please! |
6acdd0c to
09fb152
Compare
Contribution description
This adds a convenience function to iterate over a packets's URI query options.
Options have the format
key=value, but options that only consist of a key may also exist.For consistency, in this case the value is set to
"1".Testing procedure
I extended the
tests-nanocoapunit test with the new function.Issues/PRs references