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

SockProtocol::ETH_P_* SockProtocol::Htons(u16) and SockProtocol::Integer(i32) #865

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

kristate
Copy link

@kristate kristate commented Feb 23, 2018

This PR closes #854.

  • SockProtocol::ETH_P_* included in libc here
  • SockProtocol::Htons(u16) for any ethernet frames not included directly in the headers
  • SockProtocol::Integer(i32) as an override for any protocols that may be defined on custom systems that we have limited knowledge about.
  • Maintains backwards compatibility with existing API.
  • Tests

Welcome to comments and suggestions. Thanks.

/cc @mexus @Susurrus

/// Allows applications and other KEXTs to be notified when certain kernel events occur
pub enum SockProtocol {
/// htons conversion for RAW type
Htons(u16),
Copy link
Contributor

Choose a reason for hiding this comment

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

Since we are going to use plain numbers instead of fixed enums, why not make an entry like Custom that accept just an integer instead of Htons?
I mean a procotol number (in general) is just an arbitrary number, which (again, in general) doesn't have to do anything with the byte order, at least that is my understanding.

What'd you say?

Copy link
Author

@kristate kristate Feb 23, 2018

Choose a reason for hiding this comment

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

Thanks for reviewing the code!
Htons gives a conversion from u16 to htons(u16),
but yes, perhaps it might be best to also have a Integer(i32).

Two things:

  1. man socket(2) correctly states that The protocol number to use is specific to the communication domain in which communication is to take place. meaning that, as you said, a protocol is just an arbitrary number for the sock type.
  2. I like the idea of looking at API documentation and getting a feel for what is possible. That's what makes the enum so great.

So, how about keeping the existing enums, but adding an Integer(i32) enum type?

/// ([ref](https://developer.apple.com/library/content/documentation/Darwin/Conceptual/NKEConceptual/control/control.html))
#[cfg(any(target_os = "ios", target_os = "macos"))]
KextEvent = libc::SYSPROTO_EVENT,
/// Allows applications to configure and control a KEXT
KextControl,
Copy link
Contributor

Choose a reason for hiding this comment

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

Just out of curiosity, why did you reorder the items? :)

Copy link
Author

Choose a reason for hiding this comment

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

Alphabetic ordering :)

@mexus
Copy link
Contributor

mexus commented Feb 24, 2018

Well, as I've said on gitter, I would rather go with just that Integer enum. I mean these Htons and Htonl enumerators basically duplicate the to_be method. What's the reason to have them? Why there is no enum for u64? :) And what about ntohs conversions? :)

And I would definitely keep KextEvent and others as they are.

Just for the record, I'm not the one to decide :)

@kristate
Copy link
Author

kristate commented Feb 25, 2018

@mexus

Well, as I've said on gitter, I would rather go with just that Integer enum. I mean these Htons and Htonl enumerators basically duplicate the to_be method. What's the reason to have them?

Two reasons:

  1. You have to get the defines from somewhere. Using defines from the nix create keeps things cleaner.
  2. Should you get the defines from somewhere, Htons guarantees that the type called into socket(2) will be okay. There is a chance that someone calls .to_be() on a u32 instead of a u16 which, of course, is the difference between Htons and Htonl -- this plays to rust's safety.

Why there is no enum for u64?

There is not an enum for u64 because the man socket(2) call is a c_int.

And what about ntohs conversions?

The reason for Htons is to ensure that the sequence of bytes called to socket(2) are in the correct order when linux constructs a struct sockaddr_ll later on internally to match ethernet frame protocols in big endian (network). Essentially, ntohs is not required because we are ever only setting parameters from host to network, never network to host.

From man packet(7):

The link-level header information is available
in a common format in a sockaddr_ll structure.
protocol is the IEEE 802.3 protocol number in
network byte order.

Thanks for the questions.

@kristate kristate changed the title Add SockProtocol::Htons(u16) for RAW sockets SockProtocol::ETH_P_* SockProtocol::Htons(u16) and SockProtocol::Integer(i32) Feb 25, 2018
kristate added a commit to kristate/libc that referenced this pull request Feb 25, 2018
@ctrlcctrlv ctrlcctrlv mentioned this pull request Feb 25, 2018
kristate added a commit to kristate/libc that referenced this pull request Feb 25, 2018
bors added a commit to rust-lang/libc that referenced this pull request Feb 25, 2018
Add ETH_* constants on Android

Adding ETH_* constants that were found here on Linux but not on Android.

Source Reference is [linux/if_ether.h](https://android.googlesource.com/platform/bionic/+/master/libc/kernel/uapi/linux/if_ether.h) on Google OpenSource.

Thank-you.

/cc nix-rust/nix#865
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.

SockProtocol lacks protocols; No way to use ETH_P_ALL
2 participants