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.

Sign up for GitHub

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

Jump to bottom

ipv6: rpl: add source routing header for RPL #4774

Merged
merged 2 commits into from
Feb 27, 2016
Merged

ipv6: rpl: add source routing header for RPL #4774

merged 2 commits into from
Feb 27, 2016

Conversation

cgundogan
Copy link
Member

see https://tools.ietf.org/html/rfc6554

This PR only adds the parsing of the header, but does not integrate it into RPL.

@cgundogan cgundogan added Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation Area: network Area: Networking Type: new feature The issue requests / The PR implemements a new feature for RIOT labels Feb 9, 2016
@cgundogan cgundogan added the State: WIP State: The PR is still work-in-progress and its code is not in its final presentable form yet label Feb 9, 2016
@cgundogan
Copy link
Member Author

needs some re-work after an offline discussion with @authmillenon

@Yonezawa-T2
Copy link
Contributor

The call graph is following:

ipv6_ext_rh_next_hop
  gnrc_ndp_node_next_hop_l2addr
    _next_hop_l2addr in gnrc_ipv6.c
      _send in gnrc_ipv6.c
  gnrc_sixlowpan_nd_next_hop_l2addr
    _next_hop_l2addr in gnrc_ipv6.c
      _send in gnrc_ipv6.c

_send is called when the destination is not me. In source routing scenario, however, the destination is always me.
gnrc_ipv6_ext_demux would be better place to process source routing header, rather than _next_hop_l2addr.

Yonezawa-T2
Yonezawa-T2 reviewed Feb 10, 2016
View reviewed changes
sys/net/gnrc/routing/rpl/srh/gnrc_rpl_srh.c

rh->seg_left--;
i = n - rh->seg_left;
memcpy(&addr.u8[pref_elided], &addr_vec[(i - 1) * addr_len], addr_len);
Copy link
Contributor

Choose a reason for hiding this comment

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

We should check addr_vec[(i - 1) * addr_len] does not access outside of the IPv6 packet.

Copy link
Contributor

Choose a reason for hiding this comment

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

CompE should be used when this is the last segment.

@jnohlgard jnohlgard mentioned this pull request Feb 10, 2016
Open
9 tasks
@cgundogan
Copy link
Member Author

@Yonezawa-T2 thanks for the review. I addresse some of your and @authmillenon's comments

@cgundogan cgundogan removed the State: WIP State: The PR is still work-in-progress and its code is not in its final presentable form yet label Feb 10, 2016
Yonezawa-T2
Yonezawa-T2 reviewed Feb 10, 2016
View reviewed changes
sys/net/gnrc/network_layer/ipv6/ext/gnrc_ipv6_ext.c
@@ -36,6 +41,29 @@ bool gnrc_ipv6_ext_demux(kernel_pid_t iface, gnrc_pktsnip_t *pkt,
case PROTNUM_IPV6_EXT_HOPOPT:
case PROTNUM_IPV6_EXT_DST:
case PROTNUM_IPV6_EXT_RH:
pkt = gnrc_pktbuf_start_write(pkt);
Copy link
Contributor

Choose a reason for hiding this comment

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

gnrc_pktbuf_start_write may return NULL.

@Yonezawa-T2
Copy link
Contributor

I will review rest of the patch next week.

@cgundogan cgundogan force-pushed the pr/rpl/srh branch 3 times, most recently from bcee99c to ec8770d Compare February 10, 2016 11:46
@cgundogan
Copy link
Member Author

@Yonezawa-T2 addressed your comments

@cgundogan cgundogan added the CI: needs squashing Commits in this PR need to be squashed; If set, CI systems will mark this PR as unmergable label Feb 10, 2016
Yonezawa-T2
Yonezawa-T2 reviewed Feb 16, 2016
View reviewed changes
sys/include/net/ipv6/ext/rh.h
@@ -46,14 +58,16 @@ typedef struct __attribute__((packed)) {
} ipv6_ext_rh_t;

/**
* @brief Extract next hop from the routing header of an IPv6 packet.
* @brief Process the routing header of an IPv6 packet.
*
* @param[in] ipv6 An IPv6 packet.
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be [in, out].

@Yonezawa-T2 Yonezawa-T2 mentioned this pull request Feb 16, 2016
Merged
Yonezawa-T2
Yonezawa-T2 reviewed Feb 16, 2016
View reviewed changes
sys/net/gnrc/network_layer/ipv6/ext/gnrc_ipv6_ext.c
@@ -36,6 +41,32 @@ bool gnrc_ipv6_ext_demux(kernel_pid_t iface, gnrc_pktsnip_t *pkt,
case PROTNUM_IPV6_EXT_HOPOPT:
case PROTNUM_IPV6_EXT_DST:
case PROTNUM_IPV6_EXT_RH:
if ((tmp = gnrc_pktbuf_start_write(pkt)) == NULL) {
DEBUG("ipv6: could not get a copy of pkt\n");
gnrc_pktbuf_release(pkt);
Copy link
Contributor

Choose a reason for hiding this comment

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

return false; EDIT: addressed below

@cgundogan
Copy link
Member Author

rebased and addressed comments of @Yonezawa-T2

Yonezawa-T2
Yonezawa-T2 reviewed Feb 17, 2016
View reviewed changes
sys/net/gnrc/network_layer/ipv6/ext/gnrc_ipv6_ext.c
ipv6_ext_t *ext;
unsigned int offset = 0;
ipv6_hdr_t *hdr;
int res;

ext = ((ipv6_ext_t *)(((uint8_t *)pkt->data) + sizeof(ipv6_hdr_t)));
Copy link
Contributor

Choose a reason for hiding this comment

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

ext is also updated by ipv6_ext_rh_process.
Please insert
ext = ((ipv6_ext_t *)(((uint8_t *)pkt->data) + sizeof(ipv6_hdr_t) + offset));
before res = ipv6_ext_rh_process(hdr, (ipv6_ext_rh_t *)ext);

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't understand why we need to update ext before the call to ipv6_ext_rh_process?
ext will be updated for each header extension (that is not a routing header extension) that was encountered before. Once we reach the routing header extension, ext should point to the right place? That's why nh is updated before getting the next ext in line ~79.

Copy link
Contributor

Choose a reason for hiding this comment

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

If pkt is referred from multiple pointers, gnrc_pktbuf_start_write duplicates the packet data:

pkt

ext must point to the new byte array.

Note that the buffer referred from ext is updated by this line:
https://github.com/RIOT-OS/RIOT/pull/4774/files#diff-6906777c73d641182b398573d3bdba0bR87

Copy link
Member Author

Choose a reason for hiding this comment

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

addressed in cgundogan@4b18686

@cgundogan
Copy link
Member Author

@Yonezawa-T2 addressed some of your comments in the last commit

@cgundogan
Copy link
Member Author

@Yonezawa-T2 addressed your last comment

@Yonezawa-T2 Yonezawa-T2 added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Feb 22, 2016
@Yonezawa-T2
Copy link
Contributor

ACK and go when Travis is happy.

@OlegHahm
Copy link
Member

Please squash first.

@OlegHahm
Copy link
Member

@cgundogan, please squash

@OlegHahm OlegHahm added the Community: Hack'n'ACK candidate This PR is a candidate for review and discussion during one of RIOT's monthly Hack'n'ACK parties label Feb 27, 2016
@cgundogan
Copy link
Member Author

squashed

@cgundogan cgundogan removed the CI: needs squashing Commits in this PR need to be squashed; If set, CI systems will mark this PR as unmergable label Feb 27, 2016
OlegHahm added a commit that referenced this pull request Feb 27, 2016
@OlegHahm
Merge pull request #4774 from cgundogan/pr/rpl/srh
b40b630 
ipv6: rpl: add source routing header for RPL
@OlegHahm OlegHahm merged commit b40b630 into RIOT-OS:master Feb 27, 2016
@cgundogan cgundogan deleted the pr/rpl/srh branch February 28, 2016 12:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@Yonezawa-T2 Yonezawa-T2

Labels
Area: network Area: Networking CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Community: Hack'n'ACK candidate This PR is a candidate for review and discussion during one of RIOT's monthly Hack'n'ACK parties Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation Type: new feature The issue requests / The PR implemements a new feature for RIOT
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@cgundogan @Yonezawa-T2 @OlegHahm