Skip to content

gnrc_ipv6_nib: make configurable via Kconfig again#18360

Merged
maribu merged 1 commit intoRIOT-OS:masterfrom
miri64:gnrc_nib/fix/config-deps
Jul 22, 2022
Merged

gnrc_ipv6_nib: make configurable via Kconfig again#18360
maribu merged 1 commit intoRIOT-OS:masterfrom
miri64:gnrc_nib/fix/config-deps

Conversation

@miri64
Copy link
Copy Markdown
Member

@miri64 miri64 commented Jul 22, 2022

Contribution description

Some CFLAGS snuck into the Makefile.dep of GNRC which makes it impossible to configure gnrc_ipv6_nib via Kconfig. As an added bonus I added the symbol CONFIG_GNRC_IPV6_NIB_ADD_RIO_IN_LAST_RA which was also induced via CFLAGS in GNRC's Makefile.dep to the Kconfig configuration.

Testing procedure

I noticed this by increasing the size of the forwarding table for the border router (e.g. to allow for large networks with RPL storing mode). I added examples/gnrc_border_router/app.config with content

CONFIG_KCONFIG_USEMODULE_GNRC_IPV6_NIB=y
CONFIG_GNRC_IPV6_NIB_NUMOF=50
CONFIG_GNRC_IPV6_NIB_OFFL_NUMOF=50

Trying to make -C examples/gnrc_border_router will get you a bunch of these error messages in current master:

make[4]: *** [/home/mlenders/Repositories/RIOT-OS/RIOT/Makefile.base:146: /home/mlenders/Repositories/RIOT-OS/RIOT/examples/gnrc_border_router/bin/native/gnrc_netif/gnrc_netif.o] Error 1
make[3]: *** [/home/mlenders/Repositories/RIOT-OS/RIOT/Makefile.base:31: ALL--/home/mlenders/Repositories/RIOT-OS/RIOT/sys/net/gnrc/netif] Error 2
In file included from <command-line>:
/home/mlenders/Repositories/RIOT-OS/RIOT/examples/gnrc_border_router/bin/native/generated/autoconf.h:200: error: "CONFIG_GNRC_IPV6_NIB_ADV_ROUTER" redefined [-Werror]
  200 | #define CONFIG_GNRC_IPV6_NIB_ADV_ROUTER 1
      | 
<command-line>: note: this is the location of the previous definition
In file included from <command-line>:
/home/mlenders/Repositories/RIOT-OS/RIOT/examples/gnrc_border_router/bin/native/riotbuild/riotbuild.h:6: error: "CONFIG_GNRC_IPV6_NIB_ADV_ROUTER" redefined [-Werror]
    6 | #define CONFIG_GNRC_IPV6_NIB_ADV_ROUTER 0
      | 
/home/mlenders/Repositories/RIOT-OS/RIOT/examples/gnrc_border_router/bin/native/generated/autoconf.h:200: note: this is the location of the previous definition
  200 | #define CONFIG_GNRC_IPV6_NIB_ADV_ROUTER 1
      | 
cc1: all warnings being treated as errors

This PR fixes that.

Issues/PRs references

None

@miri64 miri64 requested review from benpicco and maribu July 22, 2022 15:07
@github-actions github-actions bot added Area: Kconfig Area: Kconfig integration Area: network Area: Networking Area: sys Area: System labels Jul 22, 2022
maribu
maribu approved these changes Jul 22, 2022
View reviewed changes
Copy link
Copy Markdown
Member

@maribu maribu left a comment

Choose a reason for hiding this comment

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

ACK, trusting your reasoning and testing

@maribu maribu added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Jul 22, 2022
@maribu maribu enabled auto-merge July 22, 2022 15:25
benpicco
benpicco reviewed Jul 22, 2022
View reviewed changes
sys/include/net/gnrc/ipv6/nib/conf.h
(!CONFIG_GNRC_IPV6_NIB_6LR || CONFIG_GNRC_IPV6_NIB_6LBR) && \
!(IS_USED(MODULE_GNRC_DHCPV6_CLIENT_IA_PD) || IS_USED(MODULE_GNRC_UHCPC) || \
IS_USED(MODULE_GNRC_IPV6_AUTO_SUBNETS))
#define CONFIG_GNRC_IPV6_NIB_ADV_ROUTER 1
Copy link
Copy Markdown
Contributor

@benpicco benpicco Jul 22, 2022

Choose a reason for hiding this comment

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

Does it ever make sense to send router advertisements before having a prefix to advertise?

Copy link
Copy Markdown
Member Author

@miri64 miri64 Jul 22, 2022

Choose a reason for hiding this comment

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

Yes, if you do not have any prefixes in your network or don't want to use SLAAC, e.g.

Copy link
Copy Markdown
Member Author

@miri64 miri64 Jul 22, 2022

Choose a reason for hiding this comment

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

Also just supplanting the CFLAGS from the Makefile.dep to the correct place...

Copy link
Copy Markdown
Contributor

@benpicco benpicco Jul 22, 2022

Choose a reason for hiding this comment

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

Not objecting the PR, just wondering - because this regularly bites me when I flash examples/gnrc_networking and wonder why it takes so long to get a prefix, only to remember that I have to manually set -rtr_adv so the node sends router solicitations.

Copy link
Copy Markdown
Member Author

@miri64 miri64 Jul 22, 2022

Choose a reason for hiding this comment

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

Not objecting the PR, just wondering - because this regularly bites me when I flash examples/gnrc_networking and wonder why it takes so long to get a prefix, only to remember that I have to manually set -rtr_adv so the node sends router solicitations.

You could also just use a non-routing host application, such as examples/gcoap or remove the _router in gnrc_ipv6_router_default in the application's Makefile ;-). The problem here isn't that the router tries to advertise, it is that you are using a router when you really want a non-routing host ;-).

benpicco
benpicco reviewed Jul 22, 2022
View reviewed changes
benpicco
benpicco reviewed Jul 22, 2022
View reviewed changes
sys/net/gnrc/network_layer/ipv6/nib/Kconfig

config GNRC_IPV6_NIB_ADD_RIO_IN_LAST_RA
bool "Include a Route Information Option for subnets"
default y if USEMODULE_GNRC_IPV6_AUTO_SUBNETS
Copy link
Copy Markdown
Contributor

@benpicco benpicco Jul 22, 2022

Choose a reason for hiding this comment

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

Suggested change
default y if USEMODULE_GNRC_IPV6_AUTO_SUBNETS
select MODULE_GNRC_IPV6_NIB_RIO
default y if USEMODULE_GNRC_IPV6_AUTO_SUBNETS

Copy link
Copy Markdown
Member Author

@miri64 miri64 Jul 22, 2022

Choose a reason for hiding this comment

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

Mhhh for that we would need dependency handling in Kconfig for GNRC ;-). We don't have that yet. Setting MODULE_GNRC_IPV6_NIB_RIO only would set the symbol MODULE_GNRC_IPV6_NIB_RIO to true AFAIK. Also, the right way to do it with Kconfig would be depends, otherwise you get cascading effects ;-).

Copy link
Copy Markdown
Member Author

@miri64 miri64 Jul 22, 2022

Choose a reason for hiding this comment

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

But maybe @leandrolanzieri can clear this up.

@miri64 miri64 force-pushed the gnrc_nib/fix/config-deps branch from 34aa471 to fe3d620 Compare July 22, 2022 15:52
@maribu maribu merged commit 726a8bd into RIOT-OS:master Jul 22, 2022
@miri64 miri64 mentioned this pull request Jul 23, 2022
Merged
@miri64
Copy link
Copy Markdown
Member Author

miri64 commented Jul 23, 2022

Backport provided in #18362

@miri64 miri64 deleted the gnrc_nib/fix/config-deps branch July 23, 2022 08:39
@miri64 miri64 added Process: needs backport Integration Process: The PR is required to be backported to a release or feature branch Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) labels Jul 23, 2022
This was referenced Aug 5, 2022
Merged
Merged
@maribu maribu added this to the Release 2022.10 milestone Oct 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@benpicco benpicco benpicco left review comments

@maribu maribu maribu approved these changes

@leandrolanzieri leandrolanzieri Awaiting requested review from leandrolanzieri leandrolanzieri is a code owner

@jia200x jia200x Awaiting requested review from jia200x jia200x is a code owner

@MrKevinWeiss MrKevinWeiss Awaiting requested review from MrKevinWeiss MrKevinWeiss is a code owner

@haukepetersen haukepetersen Awaiting requested review from haukepetersen

@PeterKietzmann PeterKietzmann Awaiting requested review from PeterKietzmann

@cgundogan cgundogan Awaiting requested review from cgundogan

Assignees

No one assigned

Labels

Area: Kconfig Area: Kconfig integration Area: network Area: Networking Area: sys Area: System CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Process: needs backport Integration Process: The PR is required to be backported to a release or feature branch Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors)

Projects

None yet

Milestone

Release 2022.10

Development

Successfully merging this pull request may close these issues.

3 participants

@miri64 @benpicco @maribu