Skip to content

drivers: nsos: Add support for AF_UNIX and AF_PACKET families #80557

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.

Sign up for GitHub

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

Jump to bottom
Merged
nashif merged 2 commits into from
Nov 20, 2024
Merged

drivers: nsos: Add support for AF_UNIX and AF_PACKET families #80557

nashif merged 2 commits into from
Nov 20, 2024

Conversation

@n-gillet
Copy link

@n-gillet n-gillet commented Oct 29, 2024

These families were not supported.
Add the support based on already supported families.

@zephyrbot zephyrbot added platform: Renesas RA Renesas Electronics Corporation, RA area: Networking labels Oct 29, 2024
@github-actions
Copy link

github-actions bot commented Oct 29, 2024

Hello @n-gillet, and thank you very much for your first pull request to the Zephyr project!
Our Continuous Integration pipeline will execute a series of checks on your Pull Request commit messages and code, and you are expected to address any failures by updating the PR. Please take a look at our commit message guidelines to find out how to format your commit messages, and at our contribution workflow to understand how to update your Pull Request. If you haven't already, please make sure to review the project's Contributor Expectations and update (by amending and force-pushing the commits) your pull request if necessary.
If you are stuck or need help please join us on Discord and ask your question there. Additionally, you can escalate the review when applicable. 😊

@rlubos
Copy link
Contributor

rlubos commented Oct 29, 2024

Commit 09741c7c8f6e1d0ba4d97688ef5bc07bc787f444 looks unexpected here?

@n-gillet
Copy link
Author

n-gillet commented Oct 29, 2024

Commit 09741c7 looks unexpected here?

Sorry. bad rebase. I will fix that.

@jukkar jukkar requested a review from mniestroj October 29, 2024 10:53
@n-gillet
Copy link
Author

n-gillet commented Oct 29, 2024

One question for you. I have the tests for test_unsupported_calls that are now failing. How do you want to handle them (since now they are supposed to be supported) ?

@rlubos
Copy link
Contributor

rlubos commented Oct 29, 2024 •
edited
Loading

One question for you. I have the tests for test_unsupported_calls that are now failing. How do you want to handle them (since now they are supposed to be supported) ?

That's not really correct, they're only supported with NSOS offloaded driver, it's not used in that tests. It fails because struct sockaddr_un is larger than struct sockaddr_storage in default configuration. Pehaps we should only define larger struct sockaddr_un only if NSOS driver is enabled.

rlubos
rlubos reviewed Oct 29, 2024
View reviewed changes
rlubos
rlubos previously approved these changes Oct 30, 2024
View reviewed changes
jukkar
jukkar previously approved these changes Oct 30, 2024
View reviewed changes
mniestroj
mniestroj previously approved these changes Oct 30, 2024
View reviewed changes
@dkalowsk dkalowsk added this to the v4.1.0 milestone Oct 30, 2024
@soburi soburi removed their request for review November 13, 2024 01:17
@n-gillet
Copy link
Author

n-gillet commented Nov 15, 2024

Hi,
I have spotted an issue when CONFIG_NET_SOCKETS_PACKET=y.
I will push the fix in a separated commit.

@n-gillet n-gillet dismissed stale reviews from jukkar and rlubos via 05a681a November 15, 2024 08:45
@n-gillet
Copy link
Author

n-gillet commented Nov 15, 2024

@rlubos : I think I should merge this last commit. Tell me what you prefer.

jukkar
jukkar reviewed Nov 17, 2024
View reviewed changes
include/zephyr/net/net_ip.h Outdated
Copy link
Member

@jukkar jukkar Nov 17, 2024

Choose a reason for hiding this comment

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

The commit message of the last commit drivers: nsos: fix support of AF_PACKET is a bit misleading as the change does not do any changes for the nsos driver, it just enables it to work.
Could you change the commit subject to net: ip: Add check for packet socket for offloaded sockets
Not sure what exactly to say in commit message. Could you describe there why this change is needed and also mention nsos driver there. Currently your commit message says it fixes things but does not describe why the fix works in this case (which is important to write here).

Copy link
Member

@jukkar jukkar Nov 17, 2024

Choose a reason for hiding this comment

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

Hmm, I missed your comment about merging the last commit. Yes, we could merge it in which case my comment is not really valid any more 😄

Copy link
Author

@n-gillet n-gillet Nov 19, 2024

Choose a reason for hiding this comment

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

sorry, I missed your comment too. It should be done now.

@n-gillet n-gillet force-pushed the extend_nsos branch 2 times, most recently from 7659840 to 7167cbc Compare November 18, 2024 08:18
mniestroj
mniestroj reviewed Nov 18, 2024
View reviewed changes
Noemie Gillet added 2 commits November 19, 2024 16:58
drivers: nsos: support for AF_UNIX
ee3ad11 
Handle AF_UNIX family sockets for NSOS offloaded driver
Note that the size of struct sockaddr_un is done conditionnaly based on
CONFIG_NET_NATIVE_OFFLOADED_SOCKETS
Also, NET_SOCKADDR_PTR_MAX_SIZE needs to be updated only if
CONFIG_NET_SOCKETS_PACKET is not set. Otherwise, for a AF_PACKET socket,
a struct net_context variable can be corrupted, as local would have be on
16 bytes instead of 20 bytes.

Signed-off-by: Noemie Gillet <[email protected]>
drivers: nsos: support for AF_PACKET
99c80f2 
Handle AF_PACKET family.

Signed-off-by: Noemie Gillet <[email protected]>
@n-gillet
Copy link
Author

n-gillet commented Nov 20, 2024

Hi,
In twister logs, for both failing tests, I have the following error:

Cloning into 'ethos_u_core_driver-src'...
error: RPC failed; curl 28 Failed to connect to review.mlplatform.org port 443 after 131594 ms: Connection timed out
fatal: error reading section header 'shallow-info'
Cloning into 'ethos_u_core_driver-src'...
error: RPC failed; curl 35 gnutls_handshake() failed: Error in the pull function.
fatal: expected flush after ref listing
Cloning into 'ethos_u_core_driver-src'...
fatal: unable to access 'https://review.mlplatform.org/ml/ethos-u/ethos-u-core-driver/': Failed to connect to review.mlplatform.org port 443 after 130853 ms: Connection timed out
-- Had to git clone more than once:
3 times.
CMake Error at ethos_u_core_driver-subbuild/ethos_u_core_driver-populate-prefix/tmp/ethos_u_core_driver-populate-gitclone.cmake:31 (message):
Failed to clone repository:
'https://review.mlplatform.org/ml/ethos-u/ethos-u-core-driver'

What should I do ?

@rlubos
Copy link
Contributor

rlubos commented Nov 20, 2024

What should I do ?

This looks like some global issue, not related to this PR, it's also been reported in other PRs on Discord. Let's try a rerun for starters.

@n-gillet
Copy link
Author

n-gillet commented Nov 20, 2024

What should I do ?

This looks like some global issue, not related to this PR, it's also been reported in other PRs on Discord. Let's try a rerun for starters.

Thanks !

@nashif nashif merged commit 2253a26 into zephyrproject-rtos:main Nov 20, 2024
24 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jukkar jukkar jukkar left review comments

@mniestroj mniestroj mniestroj approved these changes

@rlubos rlubos rlubos approved these changes

@duynguyenxa duynguyenxa Awaiting requested review from duynguyenxa

@KhiemNguyenT KhiemNguyenT Awaiting requested review from KhiemNguyenT

@pdgendt pdgendt Awaiting requested review from pdgendt

@ssharks ssharks Awaiting requested review from ssharks

@tbursztyka tbursztyka Awaiting requested review from tbursztyka

@thaoluonguw thaoluonguw Awaiting requested review from thaoluonguw

Assignees

@jukkar jukkar

@rlubos rlubos

Labels

area: Networking platform: Renesas RA Renesas Electronics Corporation, RA

Projects

None yet

Milestone

v4.1.0

Development

Successfully merging this pull request may close these issues.

7 participants

@n-gillet @rlubos @jukkar @mniestroj @nashif @dkalowsk @zephyrbot