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.

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

Fixed wasix network structure write in concordence to the read function #4166

Closed
wants to merge 3 commits into from

Conversation

ptitSeb
Copy link
Contributor

@ptitSeb ptitSeb commented Aug 18, 2023

Fixed wasix network structure write in concordence to the read function, and to wasix-libc.

Port is read as a "network endian" u16 but was wrote as a "big endian" u16, in practice reversing low/high byte of the u16.

@ptitSeb ptitSeb requested a review from john-sharratt August 18, 2023 13:36
@ptitSeb ptitSeb added this to the v4.1.2 milestone Aug 18, 2023
@ptitSeb ptitSeb enabled auto-merge (squash) August 18, 2023 15:18
@john-sharratt
Copy link
Contributor

Need to run regression on this change as while its quite trivial it may not be backwards compatible with some of the compiled WASM. Lets test it with...

  • rust native sockets
  • rust with socket2
  • C compiled sockets (e.g. curl )
  • static-http-server

Copy link
Contributor

@theduke theduke left a comment

Choose a reason for hiding this comment

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

Ideally we'd have a C and a Rust snapshot tests for this.

One way to do it would be a simple tcp echo server.

But that will require a bit of time to write, I know.... not sure how urgent this is.

lib/wasix/src/net/mod.rs Outdated Show resolved Hide resolved
@ptitSeb ptitSeb modified the milestones: v4.1.2, v4.2 Aug 22, 2023
@ptitSeb ptitSeb added the priority-high High priority issue label Aug 22, 2023
@ptitSeb ptitSeb modified the milestones: v4.3, v4.2 Aug 22, 2023
@ptitSeb ptitSeb mentioned this pull request Aug 31, 2023
@ptitSeb ptitSeb modified the milestones: v4.2, v4.3 Sep 12, 2023
@ptitSeb ptitSeb added priority-medium Medium priority issue and removed priority-high High priority issue labels Sep 12, 2023
@ptitSeb
Copy link
Contributor Author

ptitSeb commented Sep 12, 2023

There is a workaround in wasix-libc, so lowering the priority

@theduke theduke force-pushed the fix_wasix_port_write branch from 54936dc to 7c7fb87 Compare January 15, 2024 19:59
@syrusakbary
Copy link
Member

It seems we want to move this forward, but we need to test a bit more

@syrusakbary
Copy link
Member

At the end we decided to stick with network byte ordering for WASIX (to keep it aligned with Linux). Closing the PR

auto-merge was automatically disabled June 20, 2024 07:54

Pull request was closed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority-medium Medium priority issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants