-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Apple: Fix AF_SYSTEM and AF_SYS_CONTROL #3047
base: main
Are you sure you want to change the base?
Conversation
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @JohnTitor (or someone else) soon. Please see the contribution instructions for more information. |
27e5f21
to
6761b10
Compare
src/unix/bsd/apple/mod.rs
Outdated
@@ -3606,7 +3606,7 @@ pub const PF_ISDN: ::c_int = AF_ISDN; | |||
pub const PF_KEY: ::c_int = pseudo_AF_KEY; | |||
pub const PF_INET6: ::c_int = AF_INET6; | |||
pub const PF_NATM: ::c_int = AF_NATM; | |||
pub const PF_SYSTEM: ::c_int = AF_SYSTEM; | |||
pub const PF_SYSTEM: ::c_int = 32; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I changed the type of PF_SYSTEM
to c_int
regarding the man page of SOCKET(2):
NAME
socket – create an endpoint for communication
SYNOPSIS
#include <sys/socket.h>
int
socket(int domain, int type, int protocol);
DESCRIPTION
socket() creates an endpoint for communication and returns a descriptor.
The domain parameter specifies a communications domain within which communication will take place; this selects the protocol family which should be used. These
families are defined in the include file ⟨sys/socket.h⟩. The currently understood formats are
PF_LOCAL Host-internal protocols, formerly called PF_UNIX,
PF_UNIX Host-internal protocols, deprecated, use PF_LOCAL,
PF_INET Internet version 4 protocols,
PF_ROUTE Internal Routing protocol,
PF_KEY Internal key-management function,
PF_INET6 Internet version 6 protocols,
PF_SYSTEM System domain,
PF_NDRV Raw access to network device,
PF_VSOCK VM Sockets protocols
The socket has the indicated type, which specifies the semantics of communication. Currently defined types are:
SOCK_STREAM
SOCK_DGRAM
SOCK_RAW
A SOCK_STREAM type provides sequenced, reliable, two-way connection based byte streams. An out-of-band data transmission mechanism may be supported. A SOCK_DGRAM
socket supports datagrams (connectionless, unreliable messages of a fixed (typically small) maximum length). SOCK_RAW sockets provide access to internal network
protocols and interfaces. The type SOCK_RAW, which is available only to the super-user.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just change to AF_SYSTEM as c_int
rather than duplicating the literal
src/unix/bsd/apple/mod.rs
Outdated
pub const AF_NETBIOS: ::c_int = 33; | ||
pub const AF_PPP: ::c_int = 34; | ||
pub const pseudo_AF_HDRCMPLT: ::c_int = 35; | ||
pub const AF_SYS_CONTROL: ::c_int = 2; | ||
pub const AF_SYS_CONTROL: ::c_ushort = 2; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The old examples uses the value of AF_SYS_CONTROL
for AF_SYS_KERNCONTROL
. I can also add AF_SYS_KERNCONTROL
and leave AF_SYS_CONTROL
untouched.
Is there anything else needed from me? |
Changing types is a breaking change and we don't ship it on minor releases. I'll revisit once we decide to do a major release. |
We have an upcoming major release where we can do breaking changes. It seems unusual to only change one |
@rustbot author for the above question |
6761b10
to
624a424
Compare
Updated. Sorry for the delay. Please review. |
Just to clarify; is |
src/unix/bsd/apple/mod.rs
Outdated
pub const AF_NETBIOS: c_int = 33; | ||
pub const AF_PPP: c_int = 34; | ||
pub const pseudo_AF_HDRCMPLT: c_int = 35; | ||
pub const AF_IEEE80211: c_int = 37; | ||
pub const AF_UTUN: c_int = 38; | ||
pub const AF_VSOCK: c_int = 40; | ||
pub const AF_SYS_CONTROL: c_int = 2; | ||
pub const AF_SYS_CONTROL: c_ushort = 2; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pub const AF_SYS_CONTROL: c_ushort = 2; | |
pub const AF_SYS_CONTROL: c_ushort = 2; |
It looks like the field is u_int16_t
, so this can just be u16
https://developer.apple.com/documentation/kernel/sockaddr_ctl/1809252-ss_sysaddr.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated. Waiting on CI.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The CI test failed after I change AF_SYS_CONTROL
to u16
. Any idea why? @tgross35
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It doesn't look related, the android CI has been flaky.
I think so regarding to the document. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, please squash. I think the CI failure is unrelated.
Since this is breaking:
@rustbot label +stable-declined
6a26836
to
a6ff0dd
Compare
Squashed. |
a6ff0dd
to
92bb592
Compare
Trying to fix the issue #3044
Here is the definition I found:
Shall I alias
AF_SYS_CONTROL
toAF_SYS_KERNCONTROL
?