library: motor: bump moto-rt ABI ver to 17 - #160392
Conversation
|
These commits modify the If this was unintentional then you should revert the changes before this PR is merged. |
rustbot
added
S-waiting-on-review
|
r? @LawnGnome rustbot has assigned @LawnGnome. Use Why was this reviewer chosen?The reviewer was selected based on:
|
|
This PR fixes #160366 |
44a90a0 to
026855b
Compare
This comment has been minimized.
This comment has been minimized.
026855b to
b375908
Compare
This comment has been minimized.
This comment has been minimized.
b375908 to
6074301
Compare
6074301 to
d566f06
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
d566f06 to
be8769c
Compare
|
This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed. Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers. |
|
@tgross35 Trevor, maybe you can look at this PR? It's been gathering dust for 2+ weeks... |
There was a problem hiding this comment.
I'm not really doing r-l/r reviews as of late so might not be the best one to ping - you can always @rustbot reroll if 2-4 weeks go by without hearing back (though pinging the reviewer after a little bit never hurts).
That said, this was pretty tiny so I was able to do a pretty quick review, few minor requests. Could you also add a summary of what changed to the PR description? Looks like vectored reads/writes is the main thing.
| #[cfg(target_os = "android")] | ||
| fn shell_cmd() -> Command { | ||
| Command::new("/system/bin/sh") | ||
| } | ||
|
|
||
| #[cfg(not(target_os = "android"))] | ||
| #[cfg(target_os = "motor")] | ||
| fn shell_cmd() -> Command { | ||
| Command::new("/system/bin/sh") | ||
| } | ||
|
|
||
| #[cfg(not(any(target_os = "android", target_os = "motor")))] | ||
| fn shell_cmd() -> Command { | ||
| Command::new("/bin/sh") | ||
| } |
There was a problem hiding this comment.
Mind just cleaning this up a bit while you're here?
fn shell_cmd() -> Command {
if cfg!(target_os = "android") || cfg!(target_os = "motor") {
Command::new("/system/bin/sh")
} else {
Command::new("/bin/sh")
}
}| // Motor pids are u64 in the ABI, but the kernel bounds them to the | ||
| // i32-positive range, so the cast below is exact (pid-refactoring-design.md). | ||
| // The pid lives in the kernel-provided read-only ProcessStaticPage; its | ||
| // address and layout are part of the kernel ABI (see ProcessStaticPage in | ||
| // Motor OS's moto-sys crate). | ||
| const PROCESS_STATIC_PAGE_VADDR: usize = 0x3F7F_FFC0_0000; | ||
| const PID_OFFSET: usize = 8; | ||
| unsafe { | ||
| *crate::ptr::with_exposed_provenance::<u64>(PROCESS_STATIC_PAGE_VADDR + PID_OFFSET) as u32 | ||
| } |
There was a problem hiding this comment.
Would it make sense to just have these constants in moto-rt? Also, perhaps worth a debug_assert that the cast is okay?
Please also add a // SAFETY: comment (we're starting to enforce these)
| // There is no public `From<io::Stdin>` conversion yet. | ||
| #[allow(dead_code)] | ||
| ParentStdin, |
There was a problem hiding this comment.
Change allow to expect so this shows up if it starts being used
| pub fn temp_dir() -> PathBuf { | ||
| PathBuf::from(moto_rt::fs::TEMP_DIR) | ||
| temp_dir_from(crate::env::var_os("TMPDIR")) | ||
| } | ||
|
|
||
| fn temp_dir_from(tmpdir: Option<OsString>) -> PathBuf { | ||
| tmpdir.map(PathBuf::from).unwrap_or_else(|| PathBuf::from("/user/tmp")) | ||
| } | ||
|
|
||
| #[cfg(test)] | ||
| mod tests { | ||
| use super::*; | ||
|
|
||
| #[test] | ||
| fn temp_dir_uses_motor_fallback_and_explicit_value() { | ||
| assert_eq!(temp_dir_from(None), PathBuf::from("/user/tmp")); | ||
| assert_eq!( | ||
| temp_dir_from(Some(OsString::from("/devtools/tmp"))), | ||
| PathBuf::from("/devtools/tmp") | ||
| ); | ||
| } | ||
| } |
There was a problem hiding this comment.
This test doesn't seem super useful, it's basically testing unwrap_or_else. Would a UI/integration test make more sense? Something single-threaded so you could env::set_var("TMPDIR", ...).
rustbot
removed
the
S-waiting-on-review
|
Reminder, once the PR becomes ready for a review, use |
rustbot
added
the
S-waiting-on-author
|
r? me |
5 participants
Also plumb through several missing pieces that come with the new ABI