nanocoap: introduce coap_opt_remove() by miri64 · Pull Request #17881 · RIOT-OS/RIOT

miri64 · 2022-03-29T16:00:42Z

Contribution description

In some cases it might be necessary to remove an already added option. In fact, due to the delta-based construction of the CoAP option header, it oftentimes is easier to add a later-to-be-added option with slack and then remove it if it isn't used after all (e.g. the ETag option). This PR provides a function to remove an option from a completed packet.

Testing procedure

A unittest is provided with tests/unittests so run:

make -C tests/unittests/ tests-nanocoap flash-only test

for a board of your choice (preferably something else than native which I tested for)

Issues/PRs references

None.

chrysn

(Didn't look at the test yet, that's a bit hard to process.)

sys/include/net/nanocoap.h

sys/net/application_layer/nanocoap/nanocoap.c

miri64 · 2022-03-29T21:50:56Z

(Didn't look at the test yet, that's a bit hard to process.)

Anything I can do to make it better processable?

chrysn · 2022-03-29T22:05:23Z

Anything I can do to make it better processable?

I think not, I just had to bite through setup.

What I think is worth covering is the removal of options with different deltas. An easy way to do this based on the existing code is to run test_nanocoap__option_remove several times with a "stride" parameter, where option n * stride is set with a value of n. Good values for stride would be 1 (as is now), 8 (as that makes a 1+data option fall into a 2+data case), 32 (staying at 2+data) and 200 (making 2+data fall into 3+data).

miri64 · 2022-03-30T09:24:20Z

and 200 (making 2+data fall into 3+data).

That seems to remain at 2+data. From RFC 7252, section 3.1 I assume you meant 269?

chrysn · 2022-03-30T09:26:56Z

If of two delta=200 options the first is removed, the delta starts exceeding the 1+1+data range, and goes into the 1+2+data range. (Can also test with strides of 300 to test 1+2+data -> 1+2+data, but I expect that test to be less insightful).

miri64 · 2022-03-30T09:32:13Z

If of two delta=200 options the first is removed, the delta starts exceeding the 1+1+data range, and goes into the 1+2+data range. (Can also test with strides of 300 to test 1+2+data -> 1+2+data, but I expect that test to be less insightful).

Ahh yeah, got it. Just got confused with the initial option length.

miri64 · 2022-03-30T09:35:14Z

Mh... not sure if this is right now. The returned lengths coap_opt_remove() should be different, especially in the delta change range. But it somehow isn't. Will debug!

miri64 · 2022-03-30T13:14:54Z

Mh... not sure if this is right now. The returned lengths coap_opt_remove() should be different, especially in the delta change range. But it somehow isn't. Will debug!

Done!

chrysn · 2022-03-30T13:18:37Z

sys/include/net/nanocoap.h

+ * @brief   Removes an option previously added with another function in this
+ *          [group](@ref net_nanocoap_opt_add)


Suggested change

* @brief Removes an option previously added with another function in this

* [group](@ref net_nanocoap_opt_add)

* @brief Removes an option previously added with another function in [the

* coap_opt_add_… group](@ref net_nanocoap_opt_add)

Addressed in 3f74a33

chrysn · 2022-03-30T13:31:23Z

For the public record (and especially implementers looking for that in this context): If an option is added where it is not known in advance how long it will be (i.e. when adding an option with some worst-case length for possible later removal), one might need a function, maybe called coap_opt_shorten, coap_opt_truncate or coap_opt_trim, that is similar to this one.

That function would share some properties with this one, but is generally easier to implement as the following option just needs to be moved (and not reencoded, possibly with a differently sized delta).

The present function could be generalized, or a shortening function could be added -- but that is out of scope here. (miri64 might have pointers to a simplified special-case / limited length version she might have around later).

chrysn

Fine with the code so far, now going through the test.

sys/net/application_layer/nanocoap/nanocoap.c

chrysn · 2022-03-30T14:43:12Z

Test looks good and covers enough that I don't feel the need to give this any additional local testing.

Please squash after addressing the comments.

miri64 · 2022-03-31T07:48:21Z

Please squash after addressing the comments.

Done

chrysn

All looking good; the only review step I did not do is testing, but the provided test is thorough enough that I wouldn't know what else to test anyway, and it will be run by CI before merging.

Thanks, and ACK.

miri64 · 2022-03-31T08:12:09Z

[…] the only review step I did not do is testing, […]

I set the appropriate labels ;-)

miri64 · 2022-03-31T09:38:55Z

Failing test is unrelated.

miri64 added Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation Area: CoAP Area: Constrained Application Protocol implementations labels Mar 29, 2022

miri64 requested review from PeterKietzmann, cgundogan and haukepetersen as code owners March 29, 2022 16:00

github-actions bot added Area: network Area: Networking Area: sys Area: System Area: tests Area: tests and testing framework labels Mar 29, 2022

miri64 requested a review from chrysn March 29, 2022 16:05

chrysn reviewed Mar 29, 2022

View reviewed changes

chrysn reviewed Mar 30, 2022

View reviewed changes

chrysn added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR CI: run tests If set, CI server will run tests on hardware for the labeled PR labels Mar 30, 2022

chrysn reviewed Mar 30, 2022

View reviewed changes

miri64 mentioned this pull request Mar 30, 2022

gcoap: add nanocoap_cache support for clients #17888

Merged

miri64 added 2 commits March 31, 2022 09:47

nanocoap: introduce coap_opt_remove()

00fdd8a

tests: add test for nanocoap_opt_remove()

68f5514

miri64 force-pushed the nanocoap/enh/opt_remove branch from 16e9d38 to 68f5514 Compare March 31, 2022 07:48

chrysn approved these changes Mar 31, 2022

View reviewed changes

miri64 added the Reviewed: 1-fundamentals The fundamentals of the PR were reviewed according to the maintainer guidelines label Mar 31, 2022

chrysn merged commit d6115e3 into RIOT-OS:master Mar 31, 2022

miri64 deleted the nanocoap/enh/opt_remove branch March 31, 2022 20:07

OlegHahm added this to the Release 2022.04 milestone Apr 8, 2022

		* @brief Removes an option previously added with another function in this
		* [group](@ref net_nanocoap_opt_add)

Conversation

miri64 commented Mar 29, 2022

Contribution description

Testing procedure

Issues/PRs references

Uh oh!

chrysn left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

miri64 commented Mar 29, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

chrysn commented Mar 29, 2022

Uh oh!

miri64 commented Mar 30, 2022

Uh oh!

chrysn commented Mar 30, 2022

Uh oh!

miri64 commented Mar 30, 2022

Uh oh!

miri64 commented Mar 30, 2022

Uh oh!

miri64 commented Mar 30, 2022

Uh oh!

chrysn Mar 30, 2022

Choose a reason for hiding this comment

Uh oh!

miri64 Mar 30, 2022

Choose a reason for hiding this comment

Uh oh!

chrysn commented Mar 30, 2022

Uh oh!

chrysn left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

chrysn commented Mar 30, 2022

Uh oh!

miri64 commented Mar 31, 2022

Uh oh!

chrysn left a comment

Choose a reason for hiding this comment

Uh oh!

miri64 commented Mar 31, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

miri64 commented Mar 31, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

miri64 commented Mar 29, 2022 •

edited

Loading

miri64 commented Mar 31, 2022 •

edited

Loading