net/nanocoap: iterate options

[kb2ma]

Contribution description

Provides the ability to iterate over the options in a (nano|g)coap message. For each option, provides the length of the value and a pointer to the start of the value. This PR replaces #11437 as a closer fit with the existing nanocoap API. We also created #12075 to access the value for a specific option number similarly as an array of bytes.

To quote #11437, the ability to parse over the options allows "...to build a cache key or when implementing OSCORE." See #11761 for OSCORE implementation planning.

The PR also includes a couple of prerequisite commits for const access and improvement to an internal function for parsing an option.

Thanks to @chrysn for discussion and working through to this implementation.

In a follow on PR we plan to replace use of the nanocoap internal function coap_iterate_option() with the coap_opt_get_next() here, which provides a superset of its capabilities.

Testing procedure

Added to the tests-nanocoap unit tests.

Issues/PRs references

Partially replaces #11437

[kb2ma]

@chrysn, you'll notice I reverted to the original approach where the function returns the option length. This approach is much more congruent with the rest of the nanocoap API than to return the pointer to the option value, including the new coap_opt_get_opaque() in #12075.

Notice you don't need to reference the value pointer to verify the result. The return value is signed, and it returns a negative value when the requested option is not found.

[smlng]

otherwise this looks good to me

[smlng]

tested ACK, looks good to me.

[kb2ma]

@chrysn, I would appreciate any comment before we start to build on coap_opt_get_next() internally.

[chrysn]

Sorry, I only got around to looking through this now. I'd have preferred to have dedicated functions for setting up an iterator and looping through it (rather than passing an init_opt boolean), but that's probably a matter of preference and otherwise looks good to me (not starting to go in full review mode as it's already merged).

[chrysn]

Working through #12075 alerted me to the struct details of the iterator being exposed here rather than being an implementation detail that could change later. In the light of extensibility towards consuming OSCORE packages, those details should be hidden.

Most fields in the iterator are only relevant to construction (for which dedicated functions can be defined), but the option number (or delta to whatever the initial value was at) is advertised for being picked out of the struct. I suggest this be populated into a field similar to &value.

[kb2ma]

Thanks, @chrysn. Your comments forced me to think concretely about how we can evolve iteration with coap_opt_get_next(). I'm actually OK with it as is. Sorry this will be a little long winded.

I'm not concerned that we are packing the option number and the offset into a single parameter. We're trying to perform a task that's more complex than coap_opt_get_opaque() in a constrained environment. At the same time, the simplest case of just iterating through all of the options is easy, as in test_nanocoap__options_iterate().

I agree though that it's tricky to initialize the offset to get to a particular option from the result of coap_opt_get_opaque(). So, we could extend coap_opt_get next() so that the final parameter is unsigned to provide more choices for initialization, where the choices are:

• COAP_OPTGET_FIRST -- get the first option
• COAP_OPTGET_NEXT -- goto offset in coap_opt_pos_t
• COAP_OPTGET_OPTNUM -- find opt_num in coap_opt_pos_t, internally with coap_find_option(). In this case, the output value of opt->opt_num also is the option number, as if the iteration started from the first option. Nice!

I am hesitant to add a coap_opt_seek() function because it's only paired with coap_opt_get_next(), and adds yet another function to an already large API.

As I think about OSCORE, it seems like we're going to need another function anyway. It may be that we want to read options from some buffer in a different location, like the inner options in the payload. In that case we can use something like:

ssize_t coap_opt_get_next_buf(uint8_t *buf, unsigned len, coap_optpos_t *opt, uint8_t **value, unsigned init_opt

So now the offset in the coap_optpos_t is relative to the start of some buffer rather than the start of the coap_pkt_t buffer, which is OK. In this case coap_opt_get_next() could probably become an inline function around coap_opt_get_next_buf().

At any rate, if this approach becomes too painful, we can always create a coap_opt_get_next2() with different parameters. ;-)

[chrysn]

On Thu, Sep 12, 2019 at 03:52:22AM -0700, Ken Bannister wrote: I'm not concerned that we are packing the option number and the offset into a single parameter. We're trying to perform a task that's more complex than coap_opt_get_opaque() in a constrained environment.

My concern was not so much about the option number being a delta or an option number, but more about whether it makes sense to access it as a struct member rather than via an accessor. But (at least before we go to extensibility, see below) that's more of an API design question (like the below with intitializer-and-simple-next vs. next-that-can-initialize), and if this fits the RIOT style, so be it.

- COAP_OPTGET_FIRST -- get the first option - COAP_OPTGET_NEXT -- goto offset in coap_opt_pos_t - COAP_OPTGET_OPTNUM -- find opt_num in coap_opt_pos_t, internally with coap_find_option(). In this case, the output value of opt->opt_num still will be the delta from the previous option.

Good idea, that makes the "get next option" part more like a general tool.

As I think about OSCORE, it seems like we're going to need another function anyway. It may be that we want to read options from some buffer in a different location, like the inner options in the payload. In that case we can use something like:

That could be a helper usable by the OSCORE library, but it probably won't use it in its first iteration given that not all libraries can supply it easily. My concern for extensibility is rather that for an OSCORE message, the struct to iterate over would be differently shaped as it'll contain two cursors (one inside and one outside), and that having a client reach into its opt_num may not be so easy to work with any more. But we'll know more about that with the upcoming proposal on deep-integration. (ie. I'm happy with COAP_OPT_GET_ initializers, but not so much with users accessing its opt_num value. It's probably workable just a bit harder, we'll see that then.)

[kb2ma]

Sounds good. I don't plan to make any changes ATM. Another way to start iteration at a particular option is just to use coap_opt_get_next() until you get to that option number.

Let's see how it goes.