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.

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

Big-endian fixes: networking stack #48398

Merged
merged 1 commit into from
Mar 24, 2021
Merged

Big-endian fixes: networking stack #48398

merged 1 commit into from
Mar 24, 2021

Conversation

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

ghost commented Feb 17, 2021

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

dnfadmin commented Feb 17, 2021 •
edited
Loading

CLA assistant check
All CLA requirements met.

@directhex directhex added the arch-s390x Related to s390x architecture (unsupported) label Feb 17, 2021
@uweigand
Copy link
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
Copy link
Contributor Author

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

@akoeplinger
Copy link
Member

akoeplinger commented Mar 16, 2021 •
edited
Loading

@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
Copy link
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
Copy link
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
Copy link
Member

@scalablecory would you mind taking another look? :)

scalablecory
scalablecory approved these changes Mar 18, 2021
View reviewed changes
Copy link
Contributor

@scalablecory scalablecory left a comment

Choose a reason for hiding this comment

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

lgtm

@scalablecory
Copy link
Contributor

/azp list

@azure-pipelines
Copy link

CI/CD Pipelines for this repository:

@scalablecory
Copy link
Contributor

/azp run runtime-libraries-coreclr outerloop

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@akoeplinger
Copy link
Member

akoeplinger commented Mar 19, 2021 •
edited
Loading

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

@stephentoub stephentoub stephentoub left review comments

@akoeplinger akoeplinger akoeplinger left review comments

@scalablecory scalablecory scalablecory approved these changes

@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

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