nanocoap: Add functions to handle etag options

[bergzand]

Contribution description

This adds 3 functions for adding entity-tags to coap packets. The first
simply adds the option to the coap packet with the supplied etag value.
The second functionallow for adding a dummy etag value which is to be
replaced later by the last function. This allows for allocating space
for the option up front when the etag value itself is generated based on
the payload.

I also did a quick replace of coap_opt_add_opaque(pdu, COAP_OPT_ETAG…) in other sources.

Testing procedure

Added the option to one of the handlers of the gcoap_block_server example to test this.

The response result should look like this in wireshark:

Issues/PRs references

None

[chrysn]

On Tue, Oct 17, 2023 at 01:18:21PM -0700, Koen Zandberg wrote: You mean that my screenshot of wireshark is not good enough? :laughing:

Could have been clearer on severity: I'd prefer if there was a test, but the fact that it doesn't fault or corrupt the message is probably good enough practically -- hence the over-all ACK. A test would just be a nice touch.

[miri64]

This makes this function basically unusable for cache re-validation at the client. Unless the user somehow looks up the cache entry and fetches the exact length the server supplied. But for that they need a somewhat complete request (ignoring questions which are not accounted for in cache key validation), which causes a bootstrapping problem. Its why I did the complicated dance of adding a full-length COAP_ETAG_LENGTH_MAX dummy ETag in 728c7d6 first only to use memmove later to reduce the size if necessary.

[bergzand]

I'm not sure what to do here. For me the use case here was limited to known etag lengths for generating server responses.
I think it would significantly increase complexity of replacing the etag if you want to relax this condition. It would also mean informing the user of the function with how many bytes were removed from the packet.

At this point it might be easier to have a generic coap_opt_insert_opaque()

[chrysn]

Such applications can still use coap_opt_add_etag. When building a response from a cache or from a response received from the network, I'd expect that building the response only happens when the input is fully available, and thus the ETag can be calculated. Only when the ETag is calculated on the fly while populating the payload, then the two-part dummy steps are needed. (But to be fair, I don't quite understand what the nanocoap cache ETag tricks do in your linked commit in the first place).

[miri64]

(But to be fair, I don't quite understand what the nanocoap cache ETag tricks do in your linked commit in the first place).

It assumes that the client user does not need to know about the cache. So to allow for cache validation (i.e. get the ETag from the cache) it adds first a dummy ETag option of full length on request initialization and then populates it once it has a cache hit before send. Since that ETag comes from the server, it can be of any length, so the dummy option needs to be resized (after cache key calculation, so basically after the whole message is already constructed).

[bergzand]

I think I managed to rewrite this to allow for arbitrary etag sizes in the replace code, as long as the initial etag was larger than the new etag. The added complexity is not as much as I initially expected, but I'm rather curious whether I'm covering all edge cases. I'm also not very happy with the footgun that might be the return value

The example code does seem to still work though.

[riot-ci]

Murdock results

✔️ PASSED

feaa274 fixup! fixup! fixup! fixup! fixup! fixup! nanocoap: Add functions to handle etag options

Success	Failures	Total	Runtime
7937	0	7937	17m:26s

Artifacts

• Documentation preview

Code got changed sufficiently to warrant another look

[miri64]

Could this move pkt->payload before the CoAP header or is there a sanity check for that beforehand?

[bergzand]

The gut feeling says no, all pointer values here are grabbed from parse functions already in nanocoap.

The only way I see this happening is when the payload ptr is already incorrect or when the dummy etag value was shorter than the new one.

[miri64]

[…] or when the dummy etag value was shorter than the new one.

Sounds like there should be a check at least a (pkt->payload + len_diff) < opt check, just to avoid stupidity ;-)

[bergzand]

Sounds like there should be a check at least a (pkt->payload + len_diff) < opt check, just to avoid stupidity ;-)

API misuse should already be covered by the assert(opt_len >= len). I can add another assert, however I think this mostly results in sanity testing nanocoap internal functions.

But maybe a second look from somebody else well versed in pointer magic might help here as maybe I'm missing an edge case.

[miri64]

Yes, I think an assert would be enough in this case, just to make sure the pre-conditions are met.

[bergzand]

Added

[miri64]

The payload length should not be affected by the option shortening, so please remove this line ;-)

[bergzand]

Well spotted, fixed!

[bergzand]

@miri64 I don't really feel confident in patching the rest of the cache code to also use the coap_opt_replace_etag() there. Do you have a patch for me or do we leave that as is for now?

[miri64]

@miri64 I don't really feel confident in patching the rest of the cache code to also use the coap_opt_replace_etag() there. Do you have a patch for me or do we leave that as is for now?

I don't have a patch, so let's leave it as is for now. Make very ephemeral mental note to fix this at some point.

[Teufelchen1]

Cheekily taking that audacity to ping;
@chrysn is the unittest sufficient? Can your comment be resolved?
@miri64 You also have two unresolved comments that seem to be addressed?
@bergzand beside the unresolved comments; Is anything else blocking you here?

[chrysn]

From my side all is resolved. It was an error of mine in the first place to use the "private" form in the wrappers, wjich has long been fixed now.