-
Notifications
You must be signed in to change notification settings - Fork 12.9k
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
socket set_mark
addition.
#96334
socket set_mark
addition.
#96334
Conversation
Hey! It looks like you've submitted a new PR for the library teams! If this PR contains changes to any Examples of
|
r? @thomcc (rust-highfive has picked a reviewer for you, use r? to override) |
r? rust-lang/libs-api @rustbot label +T-libs-api -T-libs |
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.
This seems okay to me. Needs documentation and a tracking issue; I assume that's why the PR is still marked Draft.
Not only that but waited to see if the idea itself would get traction. ok will do these. |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
d9b1a9e
to
57024fb
Compare
ping from triage: FYI: when a PR is ready for review, send a message containing |
@rustbot ready |
/// Set the id of the socket for network filtering purpose | ||
/// and is only a setter. |
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's not clear to me what useful information "and is only a setter" is meant to convey here. Can this sentence be just the following?—
Set the id of the socket for network filtering purposes.
/// fn main() -> std::io::Result<()> { | ||
/// let sock = UnixDatagram::unbound()?; | ||
/// sock.set_mark(32 as u32).expect("set_mark function failed"); |
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's odd to see expect
here in a function that returns io::Result anyway. How about returning the error?
/// fn main() -> std::io::Result<()> { | |
/// let sock = UnixDatagram::unbound()?; | |
/// sock.set_mark(32 as u32).expect("set_mark function failed"); | |
/// fn main() -> std::io::Result<()> { | |
/// let sock = UnixDatagram::unbound()?; | |
/// sock.set_mark(32 as u32)?; |
/// } | ||
/// ``` | ||
#[cfg(any(doc, target_os = "linux", target_os = "freebsd", target_os = "openbsd",))] | ||
#[unstable(feature = "unix_set_mark", issue = "none")] |
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.
Thanks for opening the tracking issue. You need to put the issue number here inside the issue
attribute. Check out the other unstable
attributes in this file.
Same applies to the other new unstable
attribute below.
/// | ||
/// fn main() -> std::io::Result<()> { | ||
/// let sock = UnixStream::connect("/tmp/sock")?; | ||
/// sock.set_mark(32 as u32).expect("set_mark function failed"); |
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.
32 as u32
is redundant. This can just be sock.set_mark(32)
and the integer will be of type u32 automatically.
library/std/src/sys/unix/net.rs
Outdated
#[cfg(target_os = "linux")] | ||
pub fn set_mark(&self, mark: u32) -> io::Result<()> { | ||
setsockopt(self, libc::SOL_SOCKET, libc::SO_MARK, mark as libc::c_int) | ||
} | ||
|
||
#[cfg(target_os = "freebsd")] | ||
pub fn set_mark(&self, mark: u32) -> io::Result<()> { | ||
setsockopt(self, libc::SOL_SOCKET, libc::SO_USER_COOKIE, mark) | ||
} | ||
|
||
#[cfg(target_os = "openbsd")] | ||
pub fn set_mark(&self, mark: u32) -> io::Result<()> { | ||
setsockopt(self, libc::SOL_SOCKET, libc::SO_RTABLE, mark as libc::c_int) | ||
} |
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 signature is identical across all of these platforms, and the implementation is nearly identical. How about deduplicating it this way?—
pub fn set_mark(&self, mark: u32) -> io::Result<()> {
#[cfg(target_os = "linux")]
let option = libc::SO_MARK;
#[cfg(target_os = "linux")]
let option = libc::SO_USER_COOKIE;
#[cfg(target_os = "linux")]
let option = libc::SO_RTABLE;
setsockopt(self, libc::SOL_SOCKET, option, mark as libc::c_int)
}
or maybe this:
pub fn set_mark(&self, mark: u32) -> io::Result<()> {
setsockopt(
self,
libc::SOL_SOCKET,
#[cfg(target_os = "linux")]
libc::SO_MARK,
#[cfg(target_os = "freebsd")]
libc::SO_USER_COOKIE,
#[cfg(target_os = "openbsd")]
libc::SO_RTABLE,
mark as libc::c_int,
)
}
Triage: FYI: when a PR is ready for review, send a message containing @rustbot ready |
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.
Thanks!
@bors r+ |
socket `set_mark` addition. to be able to set a marker/id on the socket for network filtering (iptables/ipfw here) purpose.
socket `set_mark` addition. to be able to set a marker/id on the socket for network filtering (iptables/ipfw here) purpose.
socket `set_mark` addition. to be able to set a marker/id on the socket for network filtering (iptables/ipfw here) purpose.
socket `set_mark` addition. to be able to set a marker/id on the socket for network filtering (iptables/ipfw here) purpose.
socket `set_mark` addition. to be able to set a marker/id on the socket for network filtering (iptables/ipfw here) purpose.
…iaskrgr Rollup of 9 pull requests Successful merges: - rust-lang#94890 (Support parsing IP addresses from a byte string) - rust-lang#96334 (socket `set_mark` addition.) - rust-lang#99027 (Replace `Body::basic_blocks()` with field access) - rust-lang#100437 (Improve const mismatch `FulfillmentError`) - rust-lang#100843 (Migrate part of rustc_infer to session diagnostic) - rust-lang#100897 (extra sanity check against consts pointing to mutable memory) - rust-lang#100959 (translations: rename warn_ to warning) - rust-lang#101111 (Use the declaration's SourceInfo for FnEntry retags, not the outermost) - rust-lang#101116 ([rustdoc] Remove Attrs type alias) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup
to be able to set a marker/id on the socket for network filtering
(iptables/ipfw here) purpose.