Skip to content

Big-endian fixes: networking stack - #48398

Merged
scalablecory merged 1 commit into
dotnet:mainfrom
uweigand:bigendian-net
Mar 24, 2021
Merged

Big-endian fixes: networking stack#48398
scalablecory merged 1 commit into
dotnet:mainfrom
uweigand:bigendian-net

Conversation

@uweigand

@uweigand uweigand commented Feb 17, 2021

Copy link
Copy Markdown
Contributor

  • Fix various places that assumed host byte order was little-endian,
    use appropriate host <-> network byte order conversion instead.

  • This in particular affects handling of IPv4 addresses, which
    are stored internally in network order.

  • Fix endian assumptions in socket option code (GetSockOpt).

  • Update test cases and provide /proc test files from a
    big-endian system.

@ghost ghost added the area-System.Net label Feb 17, 2021
@ghost

ghost commented Feb 17, 2021

Copy link
Copy Markdown

Tagging subscribers to this area: @dotnet/ncl
See info in area-owners.md if you want to be subscribed.

Issue Details

  • Fix various places that assumed host byte order was little-endian,
    use appropriate host <-> network byte order conversion instead.

  • This in particular affects handling of IPv4 addresses, which
    are stored internally in network order.

  • Fix endian assumptions in socket option code (GetSockOpt).

  • Update test cases and provide /proc test files from a
    big-endian system.

Author: uweigand
Assignees: -
Labels:

area-System.Net
Milestone: -

@dnfadmin

dnfadmin commented Feb 17, 2021 •
edited
Loading

Copy link
Copy Markdown

CLA assistant check
All CLA requirements met.

akoeplinger
akoeplinger reviewed Feb 17, 2021
View reviewed changes
Comment thread src/libraries/Common/tests/Tests/System/MarvinTests.cs Outdated
@directhex directhex added the arch-s390x Related to s390x architecture (unsupported) label Feb 17, 2021
stephentoub
stephentoub reviewed Feb 17, 2021
View reviewed changes
Comment thread src/libraries/Common/src/System/Marvin.cs Outdated
stephentoub
stephentoub reviewed Feb 17, 2021
View reviewed changes
Comment thread src/libraries/System.Private.CoreLib/src/System/Random.Xoshiro256StarStarImpl.cs Outdated
@uweigand

uweigand commented Feb 17, 2021

Copy link
Copy Markdown
Contributor Author

Updated Marvin and Xoshiro hashes as discussed above.

@ericstj
ericstj requested a review from a team February 26, 2021 17:38
Base automatically changed from master to main March 1, 2021 09:08
@uweigand
Big-endian fixes: networking stack
0380809 
* Fix various places that assumed host byte order was little-endian,
  use appropriate host <-> network byte order conversion instead.

* This in particular affects handling of IPv4 addresses, which
  are stored internally in network order.

* Fix endian assumptions in socket option code (GetSockOpt).

* Update test cases and provide /proc test files from a
  big-endian system.
@uweigand

uweigand commented Mar 12, 2021

Copy link
Copy Markdown
Contributor Author

Fixed merge conflict due to mainline changes (Marvin hash).

@akoeplinger

akoeplinger commented Mar 16, 2021 •
edited
Loading

Copy link
Copy Markdown
Member

@uweigand I think it'd be helpful to extract the Marvin and System.Text.Json changes into separate PRs so this one can focus on just the networking fixes :)

@uweigand

uweigand commented Mar 16, 2021

Copy link
Copy Markdown
Contributor Author

@uweigand I think it'd be helpful to extract the Marvin and System.Text.Json changes into a separate PR so this one can focus on just the networking fixes :)

Will do, thanks!

@uweigand

uweigand commented Mar 16, 2021

Copy link
Copy Markdown
Contributor Author

@uweigand I think it'd be helpful to extract the Marvin and System.Text.Json changes into separate PRs so this one can focus on just the networking fixes :)

Moved the hash algorithm parts to separate PRs:
#49707 (Marvin)
#49708 (JSON key)
#49709 (Xoshiro test case)

Only the network part remains in this PR.

@akoeplinger

akoeplinger commented Mar 16, 2021

Copy link
Copy Markdown
Member

@scalablecory would you mind taking another look? :)

scalablecory
scalablecory approved these changes Mar 18, 2021
View reviewed changes

@scalablecory scalablecory left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

lgtm

@scalablecory

scalablecory commented Mar 18, 2021

Copy link
Copy Markdown
Contributor

/azp list

@azure-pipelines

azure-pipelines Bot commented Mar 18, 2021

Copy link
Copy Markdown
CI/CD Pipelines for this repository:

@scalablecory

scalablecory commented Mar 18, 2021

Copy link
Copy Markdown
Contributor

/azp run runtime-libraries-coreclr outerloop

@azure-pipelines

azure-pipelines Bot commented Mar 18, 2021

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

@akoeplinger

akoeplinger commented Mar 19, 2021 •
edited
Loading

Copy link
Copy Markdown
Member

The failures do not look related to the changes to me:

  • runtime-libraries-coreclr outerloop (Libraries Build windows x64 Debug) actually passed tests but failed due to some Helix python infrastructure problem (raised it with core-eng). actually this is a timeout as well
  • runtime-libraries-coreclr outerloop (Libraries Build OSX x64 Debug) timed out

edit: I'm trying to bump the Helix workitem timeout in #49876

@scalablecory
scalablecory merged commit 216c569 into dotnet:main Mar 24, 2021
@scalablecory

scalablecory commented Mar 24, 2021

Copy link
Copy Markdown
Contributor

Thanks for the PR!

@uweigand
uweigand deleted the bigendian-net branch March 24, 2021 09:35
@ghost ghost locked as resolved and limited conversation to collaborators Apr 28, 2021
@karelz karelz added this to the 6.0.0 milestone May 20, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@akoeplinger akoeplinger akoeplinger left review comments
@stephentoub stephentoub stephentoub left review comments
@eiriktsarpalis eiriktsarpalis Awaiting requested review from eiriktsarpalis
@jozkee jozkee Awaiting requested review from jozkee
@layomia layomia Awaiting requested review from layomia
@steveharter steveharter Awaiting requested review from steveharter
+1 more reviewer
@scalablecory scalablecory scalablecory approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

@scalablecory scalablecory

Labels

arch-s390x Related to s390x architecture (unsupported) area-System.Net

Projects

None yet

Milestone

6.0.0

Development

Successfully merging this pull request may close these issues.

7 participants

@uweigand @dnfadmin @akoeplinger @scalablecory @stephentoub @directhex @karelz