Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update 6P for better conformance with IETF RFC 8480 #987

Open
wants to merge 29 commits into
base: develop
from

Conversation

@yatch
Copy link
Member

yatch commented Jun 24, 2019

This PR updates 6P implementation for better conformance with IETF RFC 8480. Yet, some of behaviors defined in RFC 8480 are not implemented on purpose. For these, I put comments explaining why, to provide code-readers with clearer ideas behind.

In addition, this PR introduces error handler to sixtop_sf_t, which is used to inform a corresponding scheduling function of scheduling consistency detected on receiving a request. This change requires existing scheduling functions to have that handler; at least putting NULL at the end of sixtop_sf_t definitions is necessary.

New behavioral changes are tested manly by test-sixtop-sf-error-handler.c and test-sixp.c.

Once this PR is merged, I will update the wiki.

Thank @debug-ito for his inputs and preliminary reviews.

@yatch

This comment has been minimized.

Copy link
Member Author

yatch commented Jun 24, 2019

I'm going to implement MSF on top of this.

@yatch

This comment has been minimized.

Copy link
Member Author

yatch commented Jun 24, 2019

'coap-lwm2m' test failed for some reason: https://travis-ci.org/contiki-ng/contiki-ng/jobs/549757701

I don't think the changes in this PR caused the failure. I will re-run the test to see if the test failure persists.

* RC_ERR_SEQNUM. in this case, we don't want to allocate
* another transaction for this new request.
*/
goto err_seqnum;

This comment has been minimized.

Copy link
@debug-ito

debug-ito Jun 25, 2019

Contributor

Jumping into another conditional branch looks tricky to me. I would refactor the procedure under err_seqnum label into a function, and call it at the two places.

This comment has been minimized.

Copy link
@yatch

yatch Jun 25, 2019

Author Member

I take your comment.

type == SIXP_PKT_TYPE_RESPONSE &&
sixp_trans_get_cmd(trans) != SIXP_PKT_CMD_CLEAR &&
(code.value != SIXP_PKT_RC_ERR_VERSION ||
code.value != SIXP_PKT_RC_ERR_SFID) &&
(nbr = sixp_nbr_alloc(dest_addr)) == NULL) {
LOG_ERR("6P: sixp_output() fails because of no memory for another nbr\n");

This comment has been minimized.

Copy link
@debug-ito

debug-ito Jun 25, 2019

Contributor

The condition (code.value != SIXP_PKT_RC_ERR_VERSION || code.value != SIXP_PKT_RC_ERR_SFID) is equal to !(code.value == SIXP_PKT_RC_ERR_VERSION && code.value == SIXP_PKT_RC_ERR_SFID), which is always true. It's a mistake, right?

I guess the correct condition is:

nbr == NULL &&
type == SIXP_PKT_TYPE_RESPONSE &&
sixp_trans_get_cmd(trans) != SIXP_PKT_CMD_CLEAR &&
code.value != SIXP_PKT_RC_ERR_VERSION &&
code.value != SIXP_PKT_RC_ERR_SFID &&
(nbr = sixp_nbr_alloc(dest_addr)) == NULL

This comment has been minimized.

Copy link
@yatch

yatch Jun 25, 2019

Author Member

Great catch! Fixing it.

Copy link
Member Author

yatch left a comment

I will push a revised version.

* RC_ERR_SEQNUM. in this case, we don't want to allocate
* another transaction for this new request.
*/
goto err_seqnum;

This comment has been minimized.

Copy link
@yatch

yatch Jun 25, 2019

Author Member

I take your comment.

type == SIXP_PKT_TYPE_RESPONSE &&
sixp_trans_get_cmd(trans) != SIXP_PKT_CMD_CLEAR &&
(code.value != SIXP_PKT_RC_ERR_VERSION ||
code.value != SIXP_PKT_RC_ERR_SFID) &&
(nbr = sixp_nbr_alloc(dest_addr)) == NULL) {
LOG_ERR("6P: sixp_output() fails because of no memory for another nbr\n");

This comment has been minimized.

Copy link
@yatch

yatch Jun 25, 2019

Author Member

Great catch! Fixing it.

@yatch yatch force-pushed the yatch:pr/sixtop-update branch 2 times, most recently from dd0bb5f to 440f315 Jun 25, 2019
@yatch yatch force-pushed the yatch:pr/sixtop-update branch 3 times, most recently from 72fb0ad to 953802c Jul 3, 2019
@yatch yatch force-pushed the yatch:pr/sixtop-update branch from 953802c to 065d224 Nov 14, 2019
yatch added 12 commits Apr 22, 2019
We change the behavior to take seqno in the last complete transaction
even if it ends with an 'ERR' return code except for RC_ERR_VERSION
and RC_ERR_SFID.
What RFC 8480 describes for seqno management could cause the initiator
to have two consecutive requests having the same seqno, which would
bring a confusion to a SF if a response to the first request arrives
after the initiator sends the second one. That delayed response will
be treated as a legitimate response to the second request, but it is
not true.

To prevent that, next_seqno is incremented just after having a request
ready to send.
@yatch yatch force-pushed the yatch:pr/sixtop-update branch from 065d224 to 39c9b77 Nov 30, 2019
@yatch

This comment has been minimized.

Copy link
Member Author

yatch commented Dec 1, 2019

This version of 6P works flawlessly with MSF (#1000) 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.