Skip to content
This repository was archived by the owner on Jan 23, 2023. It is now read-only.

initial support for new network protocols#37315

Merged
wfurt merged 5 commits into
dotnet:masterfrom
wfurt:netlink_35143
May 22, 2019
Merged

initial support for new network protocols#37315
wfurt merged 5 commits into
dotnet:masterfrom
wfurt:netlink_35143

Conversation

@wfurt

@wfurt wfurt commented Apr 30, 2019 •
edited by antonfirsov
Loading

Copy link
Copy Markdown
Member

This change adds approved enums. I also added basic test for socket creation.

fixes dotnet/runtime#28636

@wfurt wfurt added this to the 3.0 milestone Apr 30, 2019
@wfurt wfurt requested review from a team, janvorli, krwq and stephentoub April 30, 2019 22:35
@wfurt wfurt self-assigned this Apr 30, 2019
janvorli
janvorli approved these changes Apr 30, 2019
View reviewed changes

@janvorli janvorli left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM

stephentoub
stephentoub reviewed Apr 30, 2019
View reviewed changes
Comment thread src/Native/Unix/System.Native/pal_networking.c Outdated
stephentoub
stephentoub reviewed Apr 30, 2019
View reviewed changes
Comment thread src/System.Net.Sockets/tests/FunctionalTests/CreateSocketTests.netcoreapp.cs Outdated
stephentoub
stephentoub reviewed Apr 30, 2019
View reviewed changes
Comment thread src/System.Net.Sockets/tests/FunctionalTests/CreateSocketTests.netcoreapp.cs Outdated
stephentoub
stephentoub reviewed Apr 30, 2019
View reviewed changes
Comment thread src/System.Net.Sockets/tests/FunctionalTests/CreateSocketTests.netcoreapp.cs Outdated
davidsh
davidsh reviewed Apr 30, 2019
View reviewed changes
Comment thread src/System.Net.Primitives/ref/System.Net.Primitives.cs
krwq
krwq approved these changes May 1, 2019
View reviewed changes

@krwq krwq left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM, thanks!

@davidsh davidsh added the * NO MERGE * The PR is not ready for merge yet (see discussion for detailed reasons) label May 2, 2019
@davidsh

davidsh commented May 2, 2019

Copy link
Copy Markdown
Contributor

@wfurt As discussed offline, we can't merge this PR as-is.

@krwq

krwq commented May 2, 2019 •
edited
Loading

Copy link
Copy Markdown
Member

@davidsh would taking https://github.com/dotnet/corefx/issues/14041 instead sound reasonable? That one is less open-ended but also allows users to consume those protocols

@davidsh

davidsh commented May 2, 2019 •
edited
Loading

Copy link
Copy Markdown
Contributor

@davidsh would taking #14041 instead sound reasonable? That one is less open-ended but also allows users to consume those protocols

Yes. We discussed this with @stephentoub, @karelz and @wfurt offline. @wfurt will send out revised proposal shortly. Please follow-up offline via him if you want as well.

@wfurt

wfurt commented May 2, 2019

Copy link
Copy Markdown
Member Author

I want to investigate little bit more before making next move. I'll update notes here when I'm ready.

@wfurt

wfurt commented May 3, 2019

Copy link
Copy Markdown
Member Author

I made some updates based on feedback and other discussion:

  • I pull out Netlink. Attempt to create ProtocolType.Netlink will throw as most of values ProtocolType enum. This will need more tests and thinking.

  • add more mapping for existing AF_INET and AF_INET6 protocols.

  • For AF_PACKET, protocol is the IEEE 802.3 protocol number in network order. That does not seems to be Platform specific and it seems to be OK to pass provided value to OS.

  • for AF_CAN we will support existing ProtocolType.Raw
    This may be subset of all existing use cases but it is Platform independent and can be supported on multiple OSes.

Long term, in Unix world the protocol is specific to AddressFamily. We did not structure our enum as such. However it seems like there is no name overlap right now. So for near future, we can keep adding values to ProtocolType

@wfurt wfurt removed the * NO MERGE * The PR is not ready for merge yet (see discussion for detailed reasons) label May 6, 2019
stephentoub
stephentoub approved these changes May 18, 2019
View reviewed changes

@stephentoub stephentoub left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Tests?

Comment thread src/Native/Unix/System.Native/pal_networking.c
case AddressFamily_AF_CAN:
*platformAddressFamily = AF_CAN;
return true;
#endif

@stephentoub stephentoub May 17, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Why isn't there a case AddressFamily_AF_NETLINK?

@wfurt wfurt May 20, 2019

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

For now, I wanted to return Error_EAFNOSUPPORT.

@stephentoub stephentoub May 21, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

For now, I wanted to return Error_EAFNOSUPPORT.

Then why are we adding the API?

@wfurt wfurt May 21, 2019

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

For Netlink I wanted to have enough so we can recognize it when we progress on #14041 e.g. we would be able to map platform to protocol enum.

Comment thread src/Native/Unix/System.Native/pal_networking.c
Comment thread src/Native/Unix/System.Native/pal_networking.c
@wfurt wfurt merged commit 6f6790f into dotnet:master May 22, 2019
@wfurt wfurt deleted the netlink_35143 branch May 22, 2019 18:31
stephentoub
stephentoub reviewed May 22, 2019
View reviewed changes
Comment thread src/System.Net.Sockets/tests/FunctionalTests/CreateSocketTests.netcoreapp.cs
}
catch (SocketException e) when (e.SocketErrorCode == SocketError.AccessDenied ||
e.SocketErrorCode == SocketError.ProtocolNotSupported ||
e.SocketErrorCode == SocketError.AddressFamilyNotSupported)

@stephentoub stephentoub May 22, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

If we're ignoring AddressFamilyNotSupported and ProtocolNotSupported, what are we actually testing?

@wfurt wfurt May 22, 2019

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I wanted to at least exercise PAL code.
For CAN, it is unlikely this will be runable on CI.
I can modify this to expect success on Linux when running as root. If you have other suggestions, please let me know. I can improve tests as follow-up.

@jozkee jozkee mentioned this pull request Jul 17, 2019
Merged
@antonfirsov antonfirsov mentioned this pull request Aug 30, 2021
Open
picenka21 pushed a commit to picenka21/runtime that referenced this pull request Feb 18, 2022
@wfurt
initial support for new network protocols (dotnet/corefx#37315)
0cbbd88 
* initial support for new network protocols

* feedback from review

* fix compilation of AF_CAN


Commit migrated from dotnet/corefx@6f6790f
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

4 more reviewers

@stephentoub stephentoub stephentoub approved these changes

@davidsh davidsh davidsh left review comments

@krwq krwq krwq approved these changes

@janvorli janvorli janvorli approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@wfurt wfurt

Labels

area-System.Net

Projects

None yet

Milestone

3.0

Development

Successfully merging this pull request may close these issues.

add Netlink, Packet and Can to AddressFamily and ProtocolFamily enum

5 participants

@wfurt @davidsh @krwq @stephentoub @janvorli