-
Notifications
You must be signed in to change notification settings - Fork 8.2k
net: Namespace network symbols to avoid conflicts with Posix/libc #99169
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
base: main
Are you sure you want to change the base?
net: Namespace network symbols to avoid conflicts with Posix/libc #99169
Conversation
As midi2 is provided by networking subsystem it should not depend on any features provided by Posix. Convert Posix poll API calls to zsock poll ones. There is no functionality changes, only naming changes. Signed-off-by: Jukka Rissanen <[email protected]>
Rename network symbols i.e., add net_, NET_ or ZSOCK_ prefixes to those network symbols that can be found in Posix or libc. This way we can avoid circular dependency issues. Signed-off-by: Jukka Rissanen <[email protected]>
Allow user to continue use various network APIs without adding either net_, NET_ or ZSOCK_ prefixes. Signed-off-by: Jukka Rissanen <[email protected]>
The union field should not be named the same as the actual struct, otherwise we cannot implement the compatibility header file for network namespacing. So rename in_addr to __in_addr and in6_addr to __in6_addr fields. Signed-off-by: Jukka Rissanen <[email protected]>
Removing Posix header inclusion as networking APIs and code should be self contained now. Signed-off-by: Jukka Rissanen <[email protected]>
Do not include Posix header files inside network stack as we should not depend on Posix symbols in net stack. Signed-off-by: Jukka Rissanen <[email protected]>
Use namespaced network symbols in order to avoid circular dependency between Posix and network subsystems. Signed-off-by: Jukka Rissanen <[email protected]>
|
Initial work on namespacing the network APIs, this is WIP. Creating a draft in order to get some faster CI runs than what I can do locally. |
|
Instructions for the reviewers. The main changes are in
The majority of the other changes are just to there to support the API naming changes. |
|
cfriedt
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good work so far. I just left a couple of comments for non-sockaddr things. Although I'm sure you are already aware and planning for those too.
| static int send_temperature(struct coap_resource *resource, | ||
| const struct sockaddr *addr, socklen_t addr_len, | ||
| const struct net_sockaddr *addr, socklen_t addr_len, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
socklen_t should probably also be changed to uint32_t within the network subsystem.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did not go that route, but just changed socklen_t to net_socklen_t. Not sure if we should do that uint32_t change as now it is easy to map the socklen variable between the network and Posix apis (because the naming is similar).
What other reviewers think about this, I am open to replace net_socklen_t if there are good arguments for it.
| addr.hci_channel = HCI_CHANNEL_USER; | ||
|
|
||
| if (bind(fd, (struct sockaddr *)&addr, sizeof(addr)) < 0) { | ||
| if (bind(fd, (struct net_sockaddr *)&addr, sizeof(addr)) < 0) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Calls to bind() should also be replaced with zsock_bind() (unless being used via POSIX through a sample).
Similarly, connect(), close(), should be switched to zsock_connect(), zsock_close(), etc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That file is for native_sim driver so I think I need to remove all the changes I did there.
| } | ||
|
|
||
| if (connect(fd, (struct sockaddr *)&addr, sizeof(addr)) < 0) { | ||
| if (connect(fd, (struct net_sockaddr *)&addr, sizeof(addr)) < 0) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should use zsock_connect()
| { | ||
| struct net_eth_hdr *eth_hdr = (struct net_eth_hdr *)rx_buf; | ||
|
|
||
| switch (ntohs(eth_hdr->type)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we should have a non-POSIX version of ntohs() and others too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I will change these. I started to change the source all over the code base but there was just so much of it, so I decided to concentrate my efforts first to subsys/net, tests/net and include/zephyr/net directories. Some of the drivers contain already changes and I will continue this work.
| void usbip_start(void) | ||
| { | ||
| struct sockaddr_in srv; | ||
| struct net_sockaddr_in srv; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is also a native_sim bottom.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @jukkar , overall it is looking good to me :)
| #ifndef ZEPHYR_INCLUDE_NET_NET_COMPAT_H_ | ||
| #define ZEPHYR_INCLUDE_NET_NET_COMPAT_H_ | ||
|
|
||
| #if defined(CONFIG_NET_NAMESPACE_COMPAT_MODE) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder should this ifdef be in include/zephyr/net/net_ip.h instead, and
should CONFIG_NET_NAMESPACE_COMPAT_MODE start being default y always?
Then The POSIX_API can include this header on its own from wherever it needs, and CONFIG_NET_NAMESPACE_COMPAT_MODE be switched to default n and deprecated(?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is certainly one option. The various Posix related symbols are now set in the compat header file, so we can keep it like that and just include this compat header in the Posix header files too. On the other hand Posix mandates that certain symbols are found in certain Posix header files, is it ok if we do not honor that but let network symbols be found in all the header files (by including the compat header)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On the other hand Posix mandates that certain symbols are found in certain Posix header files, is it ok if we do not honor that but let network symbols be found in all the header files (by including the compat header)?
I guess ensuring that could be done with some future PR? meaning the posix api could first include this header to keep backwards compatibility, and later in a future PR it could split its content around in the proper headers.
Anyhow, no strong opinion from me, it just seemed like a simple/practical way to proceed retaining backwards compatibility.
| } | ||
|
|
||
| char *inet_ntoa(struct in_addr in) | ||
| char *inet_ntoa(struct net_in_addr in) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess these (the signature of the shim functions the POSX API exposes) should remain the same?




Rename network symbols i.e., add net_, NET_ or ZSOCK_ prefixes to those network symbols that can be found in Posix or libc. This way we can avoid circular dependency issues.
Fixes #97050