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

gnrc_ndp2: Provide GNRC abstraction layer for NIB for sending #7064

Merged
merged 2 commits into from
Oct 6, 2017

Conversation

miri64
Copy link
Member

@miri64 miri64 commented May 16, 2017

For the new NIB simply using the old sending functions as I previously hoped (and why #6380 exists) isn't possible, since it is too tightly integrated in the old NDP. This PR (apart from a few code and API beatifications) is mostly a copy of the sending and building functions of gnrc_ndp, but it also introduces a set of tests.

The 2 is supposed to be removed as soon as the old NDP is removed

This PR is part of the network layer remodelling effort:
PR dependencies

@miri64 miri64 added Process: API change Integration Process: PR contains or issue proposes an API change. Should be handled with care. GNRC Area: network Area: Networking labels May 16, 2017
@miri64 miri64 requested a review from cgundogan May 16, 2017 08:04
@miri64 miri64 added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label May 16, 2017
miri64 added a commit to miri64/RIOT that referenced this pull request May 16, 2017
miri64 added a commit to miri64/RIOT that referenced this pull request May 22, 2017
miri64 added a commit to miri64/RIOT that referenced this pull request Jun 10, 2017
miri64 added a commit to miri64/RIOT that referenced this pull request Jun 13, 2017
miri64 added a commit to miri64/RIOT that referenced this pull request Jun 13, 2017
miri64 added a commit to miri64/RIOT that referenced this pull request Jun 15, 2017
@aabadie aabadie added this to the Release 2017.10 milestone Jun 23, 2017
@aabadie
Copy link
Contributor

aabadie commented Jun 23, 2017

@miri64, should we postponed this one ? I guess yes.

@miri64
Copy link
Member Author

miri64 commented Jun 23, 2017

The new ND wouldn't make it to 2017.07 anyway, so I guess yes.

miri64 added a commit to miri64/RIOT that referenced this pull request Jun 27, 2017
miri64 added a commit to miri64/RIOT that referenced this pull request Jun 30, 2017
miri64 added a commit to miri64/RIOT that referenced this pull request Aug 1, 2017
Copy link
Member

@cgundogan cgundogan left a comment

Choose a reason for hiding this comment

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

first round ...

*/
#ifndef GNRC_NETTYPE_NDP2
# if defined(MODULE_GNRC_IPV6) || DOXYGEN
# define GNRC_NETTYPE_NDP2 (GNRC_NETTYPE_IPV6)
Copy link
Member

Choose a reason for hiding this comment

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

Is this conditional define necessary? When is NDP being used witout IPv6?

Copy link
Member Author

Choose a reason for hiding this comment

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

In the tests :P

Copy link
Member

Choose a reason for hiding this comment

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

so, maybe mention this fact here? It's really confusing to bend things in the implementation that are oly needed for testing.

*
* @pre `(tgt != NULL) && !ipv6_addr_is_multicast(tgt)`
*
* @see [RFC 4861, section 4.3](https://tools.ietf.org/html/rfc4861#section-4.3")
Copy link
Member

Choose a reason for hiding this comment

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

/"//

* For and unsolicited advertisement, the address whose
* link-layer address has changed.
* May not be NULL and **MUST NOT** be multicast.
* @param[in] flags Neighbor advertisement flags:
Copy link
Member

Choose a reason for hiding this comment

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

indent

Copy link
Member Author

Choose a reason for hiding this comment

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

Not sure, what you mean?

Copy link
Member

Choose a reason for hiding this comment

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

shift everything starting from Neighbor one character to the right. That's what I mean (:

Copy link
Member Author

Choose a reason for hiding this comment

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

Why?

Copy link
Member

Choose a reason for hiding this comment

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

Hm, appears to be a problem in my browser then. It's shifted wrongly here. There's no tab or thin whitespace there, right?

*
* @param[in] cur_hl Default hop limit for outgoing IP packets, 0 if
* unspecified by this router.
* @param[in] flags Flags as defined above.
Copy link
Member

Choose a reason for hiding this comment

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

indent

Copy link
Member Author

Choose a reason for hiding this comment

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

Not sure, what you mean?

* @param[in] pref_ltime Length of time in seconds that addresses using
* @p prefix remain prefered. UINT32_MAX represents
* infinity.
* @param[in] flags Flags as defined above.
Copy link
Member

Choose a reason for hiding this comment

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

indent

Copy link
Member Author

Choose a reason for hiding this comment

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

Not sure, what you mean?

pkt = hdr;
}
}
/* TODO: also check if the node provides proxy servies for tgt */
Copy link
Member

Choose a reason for hiding this comment

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

/servies/sevices/

/* TODO: also check if the node provides proxy servies for tgt */
if ((pkt != NULL) &&
(!gnrc_ipv6_netif_addr_is_non_unicast(tgt) || supply_tl2a)) {
/* TL2A is not supplied and tgt is not anycast */
Copy link
Member

Choose a reason for hiding this comment

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

/TL2AO/TLLAO/

Copy link
Member Author

Choose a reason for hiding this comment

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

DEBUG("ndp2: send router solicitation (iface: %" PRIkernel_pid
", dst: %s)\n", netif->pid,
ipv6_addr_to_str(addr_str, dst, sizeof(addr_str)));
/* add SLLAO => check if there is a fitting source address to target */
Copy link
Member

Choose a reason for hiding this comment

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

note: here you called it SLLAO

Copy link
Member Author

Choose a reason for hiding this comment

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

That's an error :P

bool try_long = false;
int res;
uint16_t l2src_len;
/* maximum address length that fits into a minimum length (8) S/TL2A option */
Copy link
Member

Choose a reason for hiding this comment

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

S/TLLAO

Copy link
Member Author

Choose a reason for hiding this comment

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

TEST_RETRANS_TIMER,
NULL)));
TEST_ASSERT(gnrc_pktbuf_is_sane());
/* check packet meta-data */
Copy link
Member

Choose a reason for hiding this comment

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

please provide a macro or inline function for this meta-data check to reduce code redundancy

Copy link
Member Author

Choose a reason for hiding this comment

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

(this will make the tests harder to debug if they fail, but okay).

Copy link
Member Author

Choose a reason for hiding this comment

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

(done btw)

@miri64
Copy link
Member Author

miri64 commented Aug 18, 2017

Needed to change the API a little bit. I realized that I need access to the NIB internals for the ARO, so I removed it from the nbr_sol_send() function.

miri64 added a commit to miri64/RIOT that referenced this pull request Aug 18, 2017
miri64 added a commit to miri64/RIOT that referenced this pull request Aug 21, 2017
Copy link
Member

@cgundogan cgundogan left a comment

Choose a reason for hiding this comment

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

getting there.

*/

/**
* @defgroup net_gnrc_ndp2 IPv6 Neighbor discovery (v2)
Copy link
Member

Choose a reason for hiding this comment

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

Uppercase d for Discovery, otherwise lowercase n for Neighbor

assert((tgt != NULL) && !ipv6_addr_is_multicast(tgt));
DEBUG("ndp2: building neighbor solicitation message\n");
pkt = gnrc_icmpv6_build(options, ICMPV6_NBR_SOL, 0, sizeof(ndp_nbr_sol_t));
if (pkt != NULL) {
Copy link
Member

Choose a reason for hiding this comment

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

For debugging purposes, I would rather like an early exit in case of pkt == NULL and an appropriate DEBUG statement for that. Here and all following functions.

Copy link
Member Author

Choose a reason for hiding this comment

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

Can we decide on one style please? I think in the end the given style will lead to cleaner code and smaller code size.

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 can add an else-statement at the end with the debug message as a compromise.

Copy link
Member

@cgundogan cgundogan Oct 6, 2017

Choose a reason for hiding this comment

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

The given style very quickly leads to nested if blocks. That's already the case for a lot of places in the existing NDP code. I don't think that an early exit leads to bigger code sizes, but definitely makes the code more readable by removing one block of indentation.

Copy link
Member Author

Choose a reason for hiding this comment

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

Honestly it's a matter of taste. I could change it, but I remember that @OlegHahm was blocking early exits in the review of the old NDP. So which taste should I follow now?

Copy link
Member

Choose a reason for hiding this comment

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

then let's hear @OlegHahm's opinion about this.


static inline size_t _ceil8(uint8_t length)
{
/* NDP options use units of 8 byte for there length field, so round up */
Copy link
Member

Choose a reason for hiding this comment

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

s/there/their/

memcpy(&real_dst, dst, sizeof(real_dst));
adv_flags |= NDP_NBR_ADV_FLAGS_S;
}
/* add SL2AO based on target address */
Copy link
Member

Choose a reason for hiding this comment

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

I don't know what you mean by alternative spellings. SLLAO is the official abbreviation for Source Link-Layer Address Option (cf. Appendix F of RFC4861):

o Clarified router behavior when receiving a Router Solicitation
without Source Link-Layer Address Option (SLLAO).

@miri64
Copy link
Member Author

miri64 commented Oct 6, 2017

Addressed comments.

@miri64
Copy link
Member Author

miri64 commented Oct 6, 2017

Reply to #7064 (comment) in #7064 (comment) btw (why can't it show up under your reply above oO?)

@cgundogan
Copy link
Member

Latest commit is good, please squash!

@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 Oct 6, 2017
@miri64
Copy link
Member Author

miri64 commented Oct 6, 2017

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 Oct 6, 2017
@miri64 miri64 added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Oct 6, 2017
@miri64
Copy link
Member Author

miri64 commented Oct 6, 2017

Fixed issue Murdock had and amended.

@miri64
Copy link
Member Author

miri64 commented Oct 6, 2017

Another try :-)

@miri64
Copy link
Member Author

miri64 commented Oct 6, 2017

And another -.-

@miri64
Copy link
Member Author

miri64 commented Oct 6, 2017

Murdock finally liked it \o/

@cgundogan
Copy link
Member

GO!

@cgundogan cgundogan merged commit 43283ef into RIOT-OS:master Oct 6, 2017
@miri64 miri64 deleted the gnrc_ndp2/api/initial branch October 6, 2017 11:34
@miri64
Copy link
Member Author

miri64 commented Oct 6, 2017

GO!

Not waiting for @OlegHahm's opinion on early exits? Oo.... but okay you are the reviewer, I will not complain :D

@cgundogan
Copy link
Member

Not waiting for @OlegHahm's opinion on early exits? Oo.... but okay you are the reviewer, I will not complain :D

dammit :P

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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 Process: API change Integration Process: PR contains or issue proposes an API change. Should be handled with care.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants