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

fix: send ETH_P_ALL in htons format #1925

Merged
merged 1 commit into from
Mar 21, 2023

Conversation

tzneal
Copy link
Contributor

@tzneal tzneal commented Dec 8, 2022

No description provided.

@tzneal
Copy link
Contributor Author

tzneal commented Dec 8, 2022

This was required in my testing on Linux at least. I think the other values need the same treatment, but didn't want to change them as I didn't test on the other platforms.

@@ -217,7 +224,7 @@ pub enum SockProtocol {
// The protocol number is fed into the socket syscall in network byte order.
#[cfg(any(target_os = "android", target_os = "linux"))]
#[cfg_attr(docsrs, doc(cfg(all())))]
EthAll = libc::ETH_P_ALL.to_be(),
EthAll = htons_i32(libc::ETH_P_ALL),
Copy link
Member

Choose a reason for hiding this comment

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

Would this work instead? It's a little bit simpler to inline it.

Suggested change
EthAll = htons_i32(libc::ETH_P_ALL),
EthAll = (libc::ETH_P_ALL as u16).to_be() as i32,

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it works the same, but it's a const fn so it's evaluated at compile time and won't require inlining, or did you just mean it's simpler to not have the function?

Happy to do it either way, but I think the other values need the same treatment on Linux, I just hadn't tested that far.

Copy link
Member

Choose a reason for hiding this comment

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

Oh! It would really help if you could test them too. I suggested inlining that function just for readability's sake.

@collinoc
Copy link

collinoc commented Feb 24, 2023

Any status updates on this? Attaching this relevant Rust forums post that looks into some of the issues occurring from this.

I can confirm this fixes the mentioned issue.

@tzneal
Copy link
Contributor Author

tzneal commented Mar 21, 2023

From what I can tell, just the other ETH_ constants need this handling. The only one in nix at the moment is ETH_P_ALL, so I think it's good.

@asomers
Copy link
Member

asomers commented Mar 21, 2023

@tzneal are you going to inline that function like I suggested? And this needs a CHANGELOG, too.

@tzneal
Copy link
Contributor Author

tzneal commented Mar 21, 2023

Yeah I can, I was going to leave it for the next ETH_ case, but inlining is fine with me.

Copy link
Member

@asomers asomers left a comment

Choose a reason for hiding this comment

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

bors r+

@bors bors bot merged commit f38b721 into nix-rust:master Mar 21, 2023
@tzneal tzneal deleted the send-ethall-as-htons branch March 21, 2023 02:31
asomers pushed a commit to asomers/nix that referenced this pull request Aug 27, 2023
1925: fix: send ETH_P_ALL in htons format r=asomers a=tzneal

Co-authored-by: Todd Neal <toddneal@protonmail.com>
asomers pushed a commit to asomers/nix that referenced this pull request Aug 27, 2023
1925: fix: send ETH_P_ALL in htons format r=asomers a=tzneal

Co-authored-by: Todd Neal <toddneal@protonmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants