nanocoap: Allow accessing opaque and other written options

[chrysn]

Contribution description

This is an extension to the API of nanocoap that previously did not give the user a means to iterate over all options, or to look into option that do not fit one of the supported forms (extracting numbers or building a single-character-separated string).

The added function can be used to plainly read option values (eg. in evaluating the ETag options of a request), but also to update options previously written (eg. to set an already allocated ETag, or to find which bit to change when setting the "more" flag on a Block2 response after having built the output). It can also be useful when iterating over all options of a message, eg. to build a cache key or when implementing OSCORE.

No method of iteration is provided per se; it is expected that users of that API will reach into .options_len / .options to obtain the count of options and their stored optionnumber. (That would almost have been sufficient to do what this function offers, but the options values lack the information on where the option actually starts, which is usually obtained by the internal _parse_option).

Testing procedure

The second commit adds a resource to the nanocoap_cli test. In that test, a GET to the /req-opts resource now gives output similar to:

$ aiocoap-client "coap://[fe80::acfc:4bff:fe32:4907%tap0]/req-opts?foo=bar"
11: 7265712d6f707473
15: 666f6f3d626172

Issues/PRs references, impact & status

This has not been discussed in a previous issue/PR, but can be seen as the reading counterpart of #11386 -- in combination, one can reproduce arbitrary options from the request message into the response message (eg. the Echo option).

This change has no impact on the output code of existing programs as it purely adds a method. It increases the API surface of nanocoap, and introduces a concept there that was previously not used much (the index into pkt->options) -- it's more common in the internal API to pass around pointers to the start of the option right away.

The change is working as it is, but I'm open to suggestions on how the same goal can be achieved more within the style of nanocoap, eg. by not requiring the user to access the pkt struct fields.

[kb2ma]

@chrysn, thanks for the PR! It's so helpful to learn what you need from the library.

To summarize, I read that the use cases below are the main concern.

• Read opaque option
• Update value after added
• Iterate over options

For the discussion below, it helps to see nanocoap's function documentation grouped by subject. See #10876, which hopefully will be merged soon.

Reading options

I agree we need a way to read an opaque option. Currently nanocoap has a string reader (coap_opt_get_string()). We also need coap_opt_get_uint(), which can easily be built from coap_get_content_type().

Update value after added

I agree on the usefulness for the ETag example, although it would only work with the gcoap API when writing a message because it requires a coap_pkt_t.

Iterate over options

If OSCORE needs an option iterator, we would want to support that. Also, nanocoap generally looks up options by option number, so it would be nice to support both approaches.

Implementation

The current structure of coap_opt_by_index() accesses an option by index in the list of options. However, coap_parse() only adds a coap_opt_pos_t option for each unique option number. For example, it would only add a single entry for Uri-Query. You can see this effect in your aiocoap-client example. Try adding a second query option to the request, and the output still will show a single 15 key in the output.

How about this idea:

ssize_t coap_opt_get_next(const coap_pkt_t *pkt, coap_opt_pos_t *opt, uint8_t *value)

Let the opt param be inout. coap_opt_pos_t includes an option number and a pointer to the option. So on the input, it is a qualifier so we can find by option number and allow for iteration. If the option number is zero, start from the first option. Othewise start from the first option for the given option number. If the pointer is not null, start from that as the last option read so we can iterate over multiple options with the same option number.

For the response, the function sets the option number and pointer to the start of the option. If the requested option is not found, return 0. This means the user doesn't need to query pkt.options_len when iterating. I'd like to avoid exposing that to the user if we can.

The pkt and value options would retain the same meaning as in your coap_opt_by_index(), as would the return value as the length of value. This gives us the ability to read an opaque option and enough information to write the value. I also would allow negative values for the return just in case we want to do that in the future.

Other issues with PR

Example error handling -- Is it useful to reply on error? Why not just print an error message?

[chrysn]

Thanks for your input -- had I read it earlier I wouldn't have stumbled over the trouble with reading options twice myself just now.

I've implemented a coap_opt_get_next roughly as you suggested. There are some areas of public facing interface I'm not yet happy with:

• Checking whether the result is valid requires looking into a *pointer passed rather than inspecting the return value, this feels counter-intuitive. (It might make sense to return the start and set *length if start != NULL).
• The struct needs to be zero-initialized by the caller, and offset being zero is explicitly checked and set to the "right" zero (the offset of after the token). I think it would make sense to have a coap_opt_iter_init(&pkt) function that does that initialization right. That would return a struct, which C allows but is often discouraged -- would that be OK here? (I expect that initialization to be inlined all the time).
• Without using the Packet API, the "reading options" use case is a bit weaker as we're always iterating over the whole thing. If we have coap_opt_iter_init, there could also be a coap_opt_iter_init_with_optno that requires the packet to follow the Packet API and uses the options array to find a proper initialization value quickly.

(I'm not sure about whether the implementation is the best there can be, but that's for later review)

I agree on the usefulness for the ETag example, although it would only work with the gcoap API when writing a message because it requires a coap_pkt_t.

I could use a little more explanation here, I both Packet and Buffer API were part of nanocoap (which is only utilized by gcoap) and both use coap_pkt_t, will that not stay so?

Example error handling -- Is it useful to reply on error? Why not just print an error message?

The "error" label on the goto was probably a misnomer; it's not so much an error reply but more a "indicate that the result was truncated and continue" way out of options that are too long to display in the buffer. (In general I think it makes sense to provide good error responses via CoAP -- whatever that thing will be tested on, I don't know whether it'll have stdio, but it sure will have network, and whoever requested the result will have an easier time correlating the error response with the request than interpreting stderr).

New commits to the PR are coming in, rebasing to master may need some more time unless it's trivial. The changes will need squashing as they'll make little sense like that in the history, does that warrant a "WIP" note in the title?

[chrysn]

Brief update: I've just discovered that the present code fails in presence of message payloads due to my misunderstanding of a helper function; please hold reviews until the next update.

[kb2ma]

Apologies for the delayed response, @chrysn. I tried to code up a function that incorporates your feedback to my suggestions for coap_opt_get_next(). I ended up creating two functions, coap_opt_get_next() for iteration and coap_opt_get_value() to retrieve an opaque value. See my coap/iterate_and_opaque branch.

For coap_opt_get_next(), I agree that it's clunky to require the user to initialize the coap_opt_pos_t, and I also agree that it's more natural for the return value to be the value pointer. nanocoap actually already has a similar internal function, coap_iterate_option(), so I reimplemented coap_opt_get_next() in that style. The init_opt boolean parameter means that the user need not init the coap_opt_pos_t struct in the common case of reading all options. However, if a user needs to start iteration at a particular point within the options, they can do that by setting opt.offset.

coap_opt_get_string() is the only user of coap_iterate_option(). I plan to replace that use with coap_opt_get_next() and remove coap_iterate_option().

Now that coap_opt_get_next() is optimized for iteration, it made sense to create coap_opt_get_value(), which is optimized to retrieve a particular opaque option. The implementation uses a common implementation pattern for these 'get option' functions -- coap_find_option() followed by _parse_option(). So, I plan to refactor the implementation of coap_get_option_uint() to use coap_opt_get_value(), and rename it to coap_opt_get_uint() and make it public. I also plan to refactor coap_get_blockopt() to use coap_opt_get_value().

If this all sounds good to you, then let's close this PR. I'll create a PR for each of the other functions, and make it a high priority to get this merged so you can use them with the OSCORE integration.

[chrysn]

Yes, the iterate_and_opaque branch looks like it could work. I probably won't use coap_opt_get_value like that as it can only work on non-repeatable options, but if the iterator is working well, there may not even be a need for it -- or it can serve as a convenience accessor for opaque non-repeatable options.

Will you open a PR for that branch so I can leave a proper review there?

(And please don't close this PR right away, there's a few things in here I'd like to salvage onto your branch, in particular the test / CLI example, also to make sure I don't miss out the const fix -- I'll close it once they're ported over).

[kb2ma]

Sounds good. I'm on it.

[kb2ma]

See the promised #12074 and #12075.

[smlng]

#12074 is merged, #12075 will follow soonish after rebase.

Can this be closed then? Also @chrysn please check if both PRs do what you require, THX!

[chrysn]

Clearing up what's left of this PR:

• The option iteration is done in net/nanocoap: iterate options #12074.
• The coap_get_token_len constification was taken care of in net/nanocoap: iterate options #12074 as well.
• That PR also contains unit tests, so the integration test is not really necessary any more. (I'd like to see demo apps that make use of it, but the previous nanocoap_cli test won't serve as a good demo.)

All being covered, closing in favor of #12074.