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

net/gnrc_netif: opt of _ipv6_get_iid() #10455

Merged
merged 1 commit into from
Nov 27, 2018

Conversation

haukepetersen
Copy link
Contributor

Contribution description

While integrating NimBLE into GNRC, I am working my way through gnrc_netif, trying to implement it for NimBLE. Looking at the gnrc_netif code, there is quite some room for optimization...

I could not find anything about this on a quick search - so if I am addressing something that is already addressed somewhere else, simply ignore my renting and close this PR :-)

This PR focuses on the gnrc_netif_ipv6_get_iid() function: why are we duplicating all the generating code here, even on multiple layers? Event inside this one function there is a whole code block of 8 lines duplicated...

So to row back: as far as I can see, almost all(?) netdev implementations are actually allowing to get the IID through the NETOPT_IPV6_IID option. So why don't we just use that here?

This PR takes a first step, by using the NETOPT_IPV6_IID option to get the IID from the netdev device. If successful, we are done. If not, we proceed with the existing code.

As a follow up I would imagine, we should be able to get rid of the whole else part by only depending on getting NETOPT_IPV6_IID, right?! This would save quite some unnecessary link layer dependencies into netif. And not to mention some saved ROM :-)

Testing procedure

Run some tests using the gnrc_networking example for different link layer devices, checking the generated link local addresses carefully.

Issues/PRs references

none

@haukepetersen haukepetersen added Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation Area: network Area: Networking labels Nov 22, 2018
@haukepetersen haukepetersen requested a review from miri64 November 22, 2018 14:31
@miri64
Copy link
Member

miri64 commented Nov 22, 2018

The idea behind gnrc_netif_ipv6_get_iid() is to remove that code out of the netdev layer, as IMHO it doesn't belong there but in the interface layer.

@miri64
Copy link
Member

miri64 commented Nov 22, 2018

I mean, why do device drivers need to implement an IPv6-interface-specific function?

@haukepetersen
Copy link
Contributor Author

hm, I do disagree! Of course one can argue if it belongs into the link layer code / device driver or better somewhere higher as in netif.

But I see some good reasons to leave it in the netdev implementations:

  • netif is not used by all network stacks, leading to potential code duplication (again)
  • getting the IID via netdev is code-wise the much cleaner approach, just look at the ifdef-hell in the current gnrc_netif code...
  • having this function in the netdev implementations does not mean each driver has to implement it -> see netdev_ieee802154...

@haukepetersen
Copy link
Contributor Author

I mean, why do device drivers need to implement an IPv6-interface-specific function?

Academically speaking this might be true, but in practical software design, especially looking at our architecture, it makes much sense to put it somewhere inside the netdev implementations.

@miri64
Copy link
Member

miri64 commented Nov 27, 2018

I've thought about it and I think you are right in the long term :-/.

Copy link
Member

@miri64 miri64 left a comment

Choose a reason for hiding this comment

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

ACK. Tested if auto address configuration for samr21-xpro and native still works.

@@ -887,7 +895,8 @@ int gnrc_netif_ipv6_get_iid(gnrc_netif_t *netif, eui64_t *eui64)
break;
}
}
#endif
#endif /* GNRC_NETIF_L2ADDR_MAXLEN > 0 */

Copy link
Member

Choose a reason for hiding this comment

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

Unrelated change, but ok.

@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 Nov 27, 2018
@miri64 miri64 merged commit 48aeba5 into RIOT-OS:master Nov 27, 2018
@miri64 miri64 deleted the opt_gnrc_netifgetiid branch November 27, 2018 21:34
@aabadie aabadie added this to the Release 2019.01 milestone Dec 2, 2018
@miri64
Copy link
Member

miri64 commented Dec 3, 2018

Note: I think I'm going to revert this change after all, once #10524 is merged for two reasons:

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 Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants