Skip to content

sys/net: Move netif addr<->string functions from gnrc_netif to netif#16965

Closed
yarrick wants to merge 3 commits intoRIOT-OS:masterfrom
yarrick:netif_addr_string
Closed

sys/net: Move netif addr<->string functions from gnrc_netif to netif#16965
yarrick wants to merge 3 commits intoRIOT-OS:masterfrom
yarrick:netif_addr_string

Conversation

@yarrick
Copy link
Copy Markdown
Contributor

@yarrick yarrick commented Oct 7, 2021

Contribution description

Rename gnrc_netif_addr_(to|from)_str to netif_addr_(to|from)_str and move to the more generic netif module.
They are inline functions just calling l2util anyway.

This is a step in having the ifconfig shell command being stack independent.

Testing procedure

  • tests/gnrc_netif should still build and pass.
  • ifconfig shell command with gnrc still working

Issues/PRs references

@github-actions github-actions bot added Area: network Area: Networking Area: sys Area: System Area: tests Area: tests and testing framework labels Oct 7, 2021
yarrick added 3 commits October 8, 2021 01:04
@yarrick
sys/net/netif: Move gnrc_netif_addr_to_str
342a72e 
It has nothing to do with gnrc stack.
Add l2util as dependency for netif module
Update all callsites
@yarrick
sys/net/netif: Move gnrc_netif_addr_from_str
fc61e3a 
Similar to gnrc_netif_addr_to_str renaming
@yarrick
sys/net/netif: Add NETIF_L2ADDR_MAXLEN define
aa45feb 
To be used in stack-independent code

lwIP is set to use the same length as GNRC, so this does not need to
check which stack is in use.
@yarrick yarrick force-pushed the netif_addr_string branch from bf594b4 to aa45feb Compare October 7, 2021 23:04
@yarrick yarrick changed the title Move netif addr<->string functions from gnrc_netif to netif sys/net: Move netif addr<->string functions from gnrc_netif to netif Oct 7, 2021
@miri64
Copy link
Copy Markdown
Member

miri64 commented Oct 8, 2021

By now, the gnrc_netif_addr_*_str functions are just wrappers around l2util_addr_*_str so why not just use l2util_addr_*_str and not change the API of gnrc_netif? Alternatively, we can add a gnrc_netif_addr_*_str wrapper around netif_addr_*_str, but then we have a wrapper of a wrapper, and I don't think that makes sense if non-GNRC stacks can just use l2util_addr_*_str ;-).

miri64
miri64 requested changes Oct 8, 2021
View reviewed changes
Copy link
Copy Markdown
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.

So NACK from me. Either we do the convoluted wrapper thing or we don't go forward with this, as this seems to be a bigger API change than necessary.

@yarrick
Copy link
Copy Markdown
Contributor Author

yarrick commented Oct 8, 2021

Do you mean only switching to directly calling l2util_addr_*_str in what is now sys/shell/commands/sc_gnrc_netif.c ? (It would probably be called something else once it supports more stacks)

If it is used directly there but via the gnrc_netif name inside the gnrc stack that feels odd to me.
Does the gnrc_ prefix add much currently? I did not think so since there is nothing gnrc-specific about it.

@yarrick
Copy link
Copy Markdown
Contributor Author

yarrick commented Oct 11, 2021

Since the gnrc_netif_addr_*_str functions are declared only in the headers it seems this isn't needed for it to work in lwiP (as long as l2util module is included). Skipping for now.

@yarrick yarrick closed this Oct 11, 2021
@yarrick yarrick deleted the netif_addr_string branch October 11, 2021 21:53
@miri64
Copy link
Copy Markdown
Member

miri64 commented Oct 12, 2021

Do you mean only switching to directly calling l2util_addr_*_str in what is now sys/shell/commands/sc_gnrc_netif.c ? (It would probably be called something else once it supports more stacks)

Yes, that's what I meant. Sorry for the late reply!

@miri64
Copy link
Copy Markdown
Member

miri64 commented Oct 12, 2021

If it is used directly there but via the gnrc_netif name inside the gnrc stack that feels odd to me.
Does the gnrc_ prefix add much currently? I did not think so since there is nothing gnrc-specific about it.

The reason for the wrapper is mostly historical. First, it only existed in GNRC, then it was needed elsewhere, which is where l2util_addr_*_str entered the scene.

yarrick added a commit to yarrick/RIOT that referenced this pull request Oct 11, 2023
@yarrick
sys/shell/gnrc_netif: Use l2util versions of link addr helpers
8e37058 
To avoid dependency on gnrc files.
As suggested in RIOT-OS#16965
yarrick added a commit to yarrick/RIOT that referenced this pull request Oct 12, 2023
@yarrick
sys/shell/gnrc_netif: Use l2util versions of link addr helpers
ee6ca2b 
To avoid dependency on gnrc files.
As suggested in RIOT-OS#16965
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@miri64 miri64 miri64 requested changes

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

@cgundogan cgundogan Awaiting requested review from cgundogan

@fjmolinas fjmolinas Awaiting requested review from fjmolinas

@haukepetersen haukepetersen Awaiting requested review from haukepetersen

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

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

@PeterKietzmann PeterKietzmann Awaiting requested review from PeterKietzmann

@smlng smlng Awaiting requested review from smlng

Assignees

No one assigned

Labels

Area: network Area: Networking Area: sys Area: System Area: tests Area: tests and testing framework

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@yarrick @miri64