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

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

Closed
wants to merge 3 commits into from

Conversation

yarrick
Copy link
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

It has nothing to do with gnrc stack.
Add l2util as dependency for netif module
Update all callsites
Similar to gnrc_netif_addr_to_str renaming
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 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
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 ;-).

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.

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
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
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
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
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
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
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
Labels
Area: network Area: Networking Area: sys Area: System Area: tests Area: tests and testing framework
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants