-
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
process_unix: prefer i32::*_be_bytes over manually shifting bytes #74271
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,3 +1,4 @@ | ||
use crate::convert::TryInto; | ||
use crate::fmt; | ||
use crate::io::{self, Error, ErrorKind}; | ||
use crate::ptr; | ||
|
@@ -17,7 +18,7 @@ impl Command { | |
default: Stdio, | ||
needs_stdin: bool, | ||
) -> io::Result<(Process, StdioPipes)> { | ||
const CLOEXEC_MSG_FOOTER: &[u8] = b"NOEX"; | ||
const CLOEXEC_MSG_FOOTER: [u8; 4] = *b"NOEX"; | ||
|
||
let envp = self.capture_env(); | ||
|
||
|
@@ -52,11 +53,12 @@ impl Command { | |
drop(input); | ||
let Err(err) = self.do_exec(theirs, envp.as_ref()); | ||
let errno = err.raw_os_error().unwrap_or(libc::EINVAL) as u32; | ||
let errno = errno.to_be_bytes(); | ||
let bytes = [ | ||
(errno >> 24) as u8, | ||
(errno >> 16) as u8, | ||
(errno >> 8) as u8, | ||
(errno >> 0) as u8, | ||
errno[0], | ||
errno[1], | ||
errno[2], | ||
errno[3], | ||
CLOEXEC_MSG_FOOTER[0], | ||
CLOEXEC_MSG_FOOTER[1], | ||
CLOEXEC_MSG_FOOTER[2], | ||
|
@@ -81,12 +83,13 @@ impl Command { | |
match input.read(&mut bytes) { | ||
Ok(0) => return Ok((p, ours)), | ||
Ok(8) => { | ||
let (errno, footer) = bytes.split_at(4); | ||
assert!( | ||
combine(CLOEXEC_MSG_FOOTER) == combine(&bytes[4..8]), | ||
combine(CLOEXEC_MSG_FOOTER) == combine(footer.try_into().unwrap()), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Actually, why we not just compare 2 slice instead? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oops, that would be better indeed. |
||
"Validation on the CLOEXEC pipe failed: {:?}", | ||
bytes | ||
); | ||
let errno = combine(&bytes[0..4]); | ||
let errno = combine(errno.try_into().unwrap()); | ||
assert!(p.wait().is_ok(), "wait() should either return Ok or panic"); | ||
return Err(Error::from_raw_os_error(errno)); | ||
} | ||
|
@@ -103,13 +106,8 @@ impl Command { | |
} | ||
} | ||
|
||
fn combine(arr: &[u8]) -> i32 { | ||
let a = arr[0] as u32; | ||
let b = arr[1] as u32; | ||
let c = arr[2] as u32; | ||
let d = arr[3] as u32; | ||
|
||
((a << 24) | (b << 16) | (c << 8) | (d << 0)) as i32 | ||
fn combine(arr: [u8; 4]) -> i32 { | ||
i32::from_be_bytes(arr) | ||
Comment on lines
-106
to
+110
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same as above: before it always swapped, now it only swaps on little endian system. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The generated asms are the same: https://rust.godbolt.org/z/ezbn7K . |
||
} | ||
} | ||
|
||
|
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.
Uhm... with this code, the
errno
is always encoded in big endian into the array. But before, it was always swapped -- even on big endian devices -- right? And I don't see any#[cfg(target_endianess = ...)]
attributes anywhere. So your PR changes behavior, right? Which means either your change or the previous code is buggy? Is this code tested on big endian CPUs?I unfortunately don't know a lot about these unix system APIs. Could you maybe link me some resource which specifies that this has to be big endian? Or is this just used internally? Like... the data is used below. But if it's just internally, why swap any bytes at all? Why not keep it in native endianess?