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

Add additional check for ALOC as destination address of CoAP request messages. #1164

Merged
merged 1 commit into from
Jan 18, 2017

Conversation

xiaom-GitHub
Copy link
Contributor

Interop with NXP, notice that when OT as commissioner and NXP as Leader, OT sends the MGMT_COMM_SET.req to Leader's ALOC address, NXP node will response CoAP message with its RLOC unicast address, this will cause CoAP message retransmission due to the src/dst address mismatches. (Depends on the port, message id and token, so remove the address comparison). Thus Harness cannot find the correct packets, influences 9.2.2-commissioner and 9.2.4-commissioner. Screenshot as below:
coap_retransmit

Another enhancement is to select Leader's RLOC unicast address instead of ALOC as source address to response messages. Please have a check.

@codecov-io
Copy link

codecov-io commented Jan 17, 2017

Current coverage is 71.69% (diff: 80.00%)

Merging #1164 into master will decrease coverage by 1.69%

@@             master      #1164   diff @@
==========================================
  Files           128        128           
  Lines         17631      18792   +1161   
  Methods        2496       2637    +141   
  Messages          0          0           
  Branches       2128       2273    +145   
==========================================
+ Hits          12940      13473    +533   
- Misses         3890       4455    +565   
- Partials        801        864     +63   

Powered by Codecov. Last update 3f243ed...8b8c95a

if (((aRequestMetadata.mDestinationAddress == aMessageInfo.GetPeerAddr()) ||
aRequestMetadata.mDestinationAddress.IsMulticast()) &&
(aRequestMetadata.mDestinationPort == aMessageInfo.GetPeerPort()))
if (aRequestMetadata.mDestinationPort == aMessageInfo.GetPeerPort())
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would consider using IsAnycastRoutingLocator as an additional check for destination address instead of removing address checks.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree. Can we special case the anycast address as the original code did with the multicast address?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated and tested. Please have a check. Thanks.

@jwhui jwhui added this to the 0.01.00 (Initial Official Release) milestone Jan 17, 2017
 - Add additional check for ALOC as the destination address of CoAP request messages.
 - Leader uses RLOC instead of ALOC as source address to response messages.
@xiaom-GitHub xiaom-GitHub changed the title Only check src port and dst port when receiving CoAP messages. Add additional check for ALOC as destination address of CoAP request messages. Jan 18, 2017
Copy link
Contributor

@librasungirl librasungirl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍

Copy link
Member

@jwhui jwhui left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍

@jwhui jwhui merged commit 419d251 into openthread:master Jan 18, 2017
@xiaom-GitHub xiaom-GitHub deleted the pr/allow_diff_src_ip_for_coap branch January 18, 2017 05:20
amejias pushed a commit to Zolertia/openthread that referenced this pull request Mar 8, 2017
- Add additional check for ALOC as the destination address of CoAP request messages.
 - Leader uses RLOC instead of ALOC as source address to response messages.
vaas-krish pushed a commit to vaas-krish/openthread that referenced this pull request May 23, 2017
…ub_02712ff to master

* commit '6572d839fe0879af1ee11f97902f0193827106c5':
  Fix a python sniffer stop method (Remove deadlock). (openthread#1163)
  Python Sniffer Implementation for Windows (openthread#1147)
  Fix few bugs: (openthread#1164)
  Fix Route TLV handling during reattach. (openthread#1168)
  Convert `MeshHeader` constructors to fail-able initializers (openthread#1157)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants