Redox OS support #844

jD91mZM2 · 2018-05-23T15:17:17Z

This was a part of my RSoC project of porting tokio and I think it's complete now :)

Tested:

Two small custom examples
11/13 tokio examples work (udp servers might not be working but I'm getting 0.0.0.0:0 straight from rust which indicates this is a problem with either my setup or rust itself)
6/7 hyper examples work (web_api is trying to connect to itself, which I think is generally broken - you can't even communicate between two netcat instances running on redox)

jD91mZM2 · 2018-05-23T15:30:44Z

Oof, seems like our net2 patch breaks windows support. Thank you AppVeyor :)

jD91mZM2 · 2018-05-23T16:17:18Z

Sometimes I really wish cargo would let you apply a patch to all dependencies

jD91mZM2 · 2018-05-25T05:22:08Z

We are currently trying to get net2 and other deps upstreamed. You might want to hold on to this PR until we're done :)

carllerche · 2018-07-03T21:26:59Z

Thanks for doing this work.

Currently, adding additional platform support requires time from me that I do not have. I have written up an issue proposing how to address this here: #853. Thoughts?

To get this going, someone would have to step up to be the maintainer of Mio + Redox. Next, the PR would need to be updated to only impact redox. I noticed that dependencies Cargo.toml were updated, etc... these should be submitted as separate PRs. Also, all tests would need to pass.

jD91mZM2 · 2018-07-04T05:42:06Z

Initially I thought the version bumps were necessary to get miow to use the latest version of dependencies and therefore support redox. But Cargo is nice enough that 0.2.5 will also match 0.2.33 so a cargo update would suffice. I will separate it from this PR shortly.

Making the platform unstable for a while does not work very well with tokio since people would then indirectly use mio and can't control the features.

I guess I'll need to step up as maintainer for this.

carllerche · 2018-07-05T17:03:31Z

We can continue the details of the process (stable vs. unstable) here: #853

jD91mZM2 · 2018-07-21T08:39:27Z

Bump?

Thomasdezeeuw

I've left a good number of small comments, mostly just small fixes. I have no idea if the code actually works, as I don't have Redox running. I think it would be nice if it could be tested on Travis.

Also I think the commits could be cleaned up.

Other then that this look good.

Thomasdezeeuw · 2018-07-22T21:41:05Z

Cargo.toml

@@ -26,18 +26,21 @@ with-deprecated = []
 default = ["with-deprecated"]

 [dependencies]
-lazycell = "1"
+iovec    = "0.1.2"


Some dependencies are downgraded, such lazycell and libc.

Thomasdezeeuw · 2018-07-22T21:42:39Z

src/deprecated/redox.rs

+ */
+
+pub fn pipe() -> io::Result<(PipeReader, PipeWriter)> {
+    let (rd, wr) = try!(sys::pipe());


Use sys::pipe()?. ? is prefered over try!.

Thomasdezeeuw · 2018-07-22T21:43:00Z

src/deprecated/redox.rs

+
+pub fn pipe() -> io::Result<(PipeReader, PipeWriter)> {
+    let (rd, wr) = try!(sys::pipe());
+    Ok((From::from(rd), From::from(wr)))


You can also use rd.into(), slightly smaller.

Thomasdezeeuw · 2018-07-22T21:44:58Z

src/deprecated/redox.rs

+
+impl PipeReader {
+    pub fn from_stdout(stdout: process::ChildStdout) -> io::Result<Self> {
+        match sys::set_nonblock(stdout.as_raw_fd()) {


You can use sys::set_nonblock(stdout.as_raw_fd())?.

Looks like most of this file was a copy paste of the unix code. Perhaps that should be cleaned up as well 😛

Thomasdezeeuw · 2018-07-22T21:45:23Z

src/deprecated/redox.rs

+            _ => {},
+        }
+        return Ok(PipeReader::from(unsafe { Io::from_raw_fd(stdout.into_raw_fd()) }));
+    }


Add new empty line.

Thomasdezeeuw · 2018-07-22T22:22:44Z

src/sys/redox/selector.rs

+        unsafe {
+            let bytes = (&self.efd).read(slice::from_raw_parts_mut(
+                evts.events.as_mut_ptr() as *mut u8,
+                evts.events.capacity() * mem::size_of::<syscall::Event>()


I think this is incorrect. You're going beyond the bounds of the slice.

???

I'm taking the full vector, including the uninitialized contents, as a byte slice.
Then depending on how much the operating system read I set_len to update the vector's length.

I'm sorry, you're right. The code is fine I missed the fact that the type of the slice is changed to bytes.

Thomasdezeeuw · 2018-07-22T22:27:19Z

src/sys/redox/selector.rs

+pub struct Selector {
+    id: usize,
+    efd: File,
+    tokens: Mutex<BTreeMap<RawFd, BTreeSet<Token>>>


Why not use HashMap and HashSet? Just curious.

No idea, I just went with what Jeremy did. I heard someone mention you for some reason couldn't use HashMaps inside the redox kernel, so perhaps he just got used to BTrees...

Ok, well then it's fine to leave it.

Thomasdezeeuw · 2018-07-22T22:29:21Z

src/sys/redox/tcp.rs

+    }
+
+    pub fn try_clone(&self) -> io::Result<TcpStream> {
+        self.inner.try_clone().map(|s| {


self.inner.try_clone().map(TcpStream::from_stream)?

Thomasdezeeuw · 2018-07-22T22:30:27Z

src/sys/redox/tcp.rs

+    }
+}
+
+impl<'a> Read for &'a TcpStream {


As with other Read/Write implementations on references; this is ok to do in Redox?

Thomasdezeeuw · 2018-07-22T22:32:49Z

src/sys/redox/udp.rs

+
+    pub fn set_only_v6(&self, _only_v6: bool) -> io::Result<()> {
+        //self.io.set_only_v6(only_v6)
+        Err(io::Error::new(io::ErrorKind::Other, "Not implemented"))


Maybe unimplemented!()? Since I would like to know this while developing, but I'm not sure about this.

The idea is that an application can fall back to something else

Ok. So I would be correct to assum it's not implemented in Redox?

jD91mZM2 · 2018-07-23T07:25:08Z

Thank you for taking a look!

~~While fixing the things I noticed unix now has a uds module??? Since when are unix sockets a thing? I'll have to port that as well I guess.~~ (Oh, it's deprecated.)

Anyway, if you want the commits squashed you're welcome to do so. I'd prefer that the pull request itself keeps the history

jD91mZM2 · 2018-07-23T11:08:11Z

Could you restart the appveyor test? I really doubt I affected that 😛

Thomasdezeeuw · 2018-07-23T11:04:58Z

src/deprecated/redox.rs

+    }
+}
+
+impl<'a> Read for &'a PipeReader {


Ok, then this looks good to me.

Thomasdezeeuw · 2018-07-23T11:05:52Z

src/deprecated/redox.rs

+
+impl From<Io> for PipeReader {
+    fn from(io: Io) -> PipeReader {
+        PipeReader { io: io }


PipeReader { io } works here.

Thomasdezeeuw · 2018-07-23T11:06:11Z

src/deprecated/redox.rs

+
+impl From<Io> for PipeWriter {
+    fn from(io: Io) -> PipeWriter {
+        PipeWriter { io: io }


PipeWriter { io }

Thomasdezeeuw · 2018-07-23T11:10:32Z

src/net/tcp.rs

@@ -7,12 +7,16 @@
 //!
 /// [portability guidelines]: ../struct.Poll.html#portability

+#[cfg(windows)]


Your right I was look at TcpStream::connect, but it uses SocketAddr::V4.

Thomasdezeeuw · 2018-07-23T11:12:29Z

src/sys/redox/selector.rs

+
+#[derive(Debug)]
+pub struct Selector {
+    id: usize,


Ok, nevermind then.

Thomasdezeeuw · 2018-07-23T11:13:41Z

src/sys/redox/selector.rs

+    pub fn select(&self, evts: &mut Events, awakener: Token, timeout: Option<Duration>) -> io::Result<bool> {
+        let mut timeout_fd = None;
+        if let Some(timeout) = timeout {
+            let mut file = File::open(format!("time:{}", CLOCK_MONOTONIC))?;


Didn't know about File::open allocating. Then lets leave this for a future pr.

Thomasdezeeuw · 2018-07-23T11:29:07Z

A lot of things are resolved, good job. But is there a way to test this in CI? That would really ensure that the code keeps working in the future as well.

jD91mZM2 · 2018-07-23T15:32:53Z

I'd love to add CI support, but the policy for adding new platforms has not yet been completely resolved, and @asomers' suggested changes would imply that redox starts off without a CI until it gets a lower tier.

carllerche · 2018-08-17T20:01:09Z

At the very least, it would be nice if there was some sort of cross compilation test to ensure that it compiled at the very least.

carllerche · 2018-08-17T20:10:02Z

@hawkw could you review this? I would not focus on any of the implementation details. The only thing of import is that no existing code is impacted short of mod declarations & use statements that are scoped with a cfg for redox.

hawkw

@carllerche I've commented on a few changes in this branch that seem unrelated to adding Redox support.

hawkw · 2018-08-17T20:42:30Z

src/sys/unix/tcp.rs

-                inner: s,
-            }
-        })
+        self.inner.try_clone().map(Self::from_stream)


This change seems unrelated from the rest of this branch?

hawkw · 2018-08-17T20:47:34Z

src/net/tcp.rs

-        if cfg!(windows) {
-            sock.bind(&inaddr_any(addr))?;
-        }
+        #[cfg(windows)]


The change from if cfg!(windows) { ... } to #[cfg(windows)] is probably better, but seems unrelated to adding Redox support.

hawkw · 2018-08-17T20:47:49Z

src/net/tcp.rs

@@ -389,6 +401,7 @@ impl TcpStream {
    }
 }

+#[cfg(windows)]


Again, this seems unrelated to Redox support.

hawkw · 2018-08-17T20:48:19Z

src/net/tcp.rs

-        if cfg!(unix) {
-            sock.reuse_address(true)?;
-        }
+        #[cfg(unix)]


Change from if cfg!(...) to #[cfg(...)] attribute seems unrelated.

hawkw · 2018-08-17T20:49:24Z

src/net/tcp.rs

@@ -7,12 +7,16 @@
 //!
 /// [portability guidelines]: ../struct.Poll.html#portability

+#[cfg(windows)]


Looks like an unrelated change?

hawkw · 2018-08-17T20:50:20Z

Cargo.toml

 [target.'cfg(unix)'.dependencies]
-libc   = "0.2.42"
+libc = "0.2.42"


Unrelated formatting change (though, this did need to be fixed, I think).

hawkw · 2018-08-17T20:50:23Z

Cargo.toml

 lazycell = "1"
 log      = "0.4"
+net2     = "0.2.33"


Were the dependency version bumps necessary for adding Redox support?

Yes, net2 only recently has redox support

Okay, thanks for clearing that up!

jD91mZM2 · 2018-08-18T08:59:23Z

@carllerche Didn't want to set up a CI until I got thumbs up, especially with @asomers' proposed platform policy (which would only allow a CI at tier 2). With your permission I shall do so now.

jD91mZM2 · 2018-08-18T15:32:03Z

Guessing the current CI issue is unrelated?

hawkw · 2018-08-18T21:24:49Z

@jD91mZM2, yeah, the iOS CI job is busted (#863). I have a PR open (#865) to fix it which should hopefully be merged soon.

BTW, thanks for backing out the unrelated changes for now. I think several of them (such as using #[cfg(...)] rather than if cfg!(...)) would probably be good to have in the long run, but those changes probably ought to land in their own branch.

carllerche

Thanks, I added some inline comments. I think we can get this to a point where it can be merged soon.

carllerche · 2018-08-20T21:20:26Z

src/lib.rs

@@ -198,12 +201,13 @@ pub use poll::Iter as EventsIter;
 #[doc(hidden)]
 pub use io::deprecated::would_block;

-#[cfg(all(unix, not(target_os = "fuchsia")))]
+#[cfg(any(all(unix, not(target_os = "fuchsia")), target_os = "redox"))]


Is it possible to move this to a separate block? So, split out the cfg(redox) and have a separate pub mod unix? It would make it easier to manage IMO.

carllerche · 2018-08-20T21:22:01Z

src/net/tcp.rs

@@ -7,12 +7,16 @@
 //!
 /// [portability guidelines]: ../struct.Poll.html#portability

+#[cfg(not(target_os = "redox"))]


Could you explain on why this file needs changes?

I would prefer to avoid adding platform cfg guards in src/net/tcp.

Back when I wrote this, I would argue that "redox doesn't support creating non-blocking sockets before an address is bound". I am extremely certain that I tried it, but... I've seen the source code of redox' libc now, and it seems to disagree.

It's still always better to use native rust where possible (redox' backend doesn't use libc), but I cannot safely say the old code won't wory. I'll try it again, later.

Just double checked, also checked the code. There are unbound sockets implemented, but they don't support fcntl. I'll ask in the community chat what they'd like to do here.

carllerche · 2018-08-20T21:23:22Z

src/net/tcp.rs

@@ -683,24 +709,24 @@ impl fmt::Debug for TcpListener {
 *
 */

-#[cfg(all(unix, not(target_os = "fuchsia")))]
+#[cfg(any(all(unix, not(target_os = "fuchsia")), target_os = "redox"))]


I'm wondering if these impls should be moved out of this file in order to maintain the goal of "no cfgs" in this file.

Is that a question or an order? Personally I think it makes sense to keep them here since they're all identical content-wise (apart from fuschia), it's just a bummer redox doesn't count as unix for some reason

jD91mZM2 · 2018-08-21T06:04:47Z

The latest changes separate stuff into their respective platforms. This obviously also affects unix/windows, but not more than strictly necessary. I might be able to revert and keep redox consistent to unix if the redox netstack team gives me thumbs up to implement fcntl for unbound sockets

This reverts commit cfd4711.

Yes this commit message sucks you should really just squash all commits :)

This reverts commit 2092161.

carllerche · 2019-05-08T20:08:50Z

Ok, here is the plan. Lets get this in the 0.7 release of Mio.

I commented with a proposed implementation strategy for adding new platforms here. This proposal is based on how this PR needs to touch so many files. I want to avoid this.

So, if my proposed strategy is accepted, the contents of sys/redox would be moved into a dedicated crate that mio would conditionally depend on (repoll?). Also, we would split out the cfg flags... I'll probably work on some of this restructuring on master.

wdyt?

jD91mZM2 · 2019-05-10T14:24:45Z

I appreciate the idea, it sounds pretty great! I'm afraid, however, I might just drop this PR in its entirety as rust-lang/rust#60139 is merging. Redox is moving its focus away from updating the world to rely directly on Redox APIs, and instead making the C API cover everything. So I am sorry if I made you figure out this policy for smaller gain than expected, but I hope you can still use it for the other platforms as it sounds like a great idea in general.

carllerche · 2019-05-10T17:10:02Z

Sorry about all that. The policy is still going to be valuable I think.

However, even w/ redox targettng unix, I would guess that some level of redox specific integration is going to be needed. There are linux, bsd, etc... specific code for implementing the selector.

jD91mZM2 · 2019-05-11T07:15:16Z

That's true, someone will probably open a small PR to add redox to the linux cfgs, soon enough. We seem to be going the epoll route in our C standard library relibc. 😄

Thomasdezeeuw · 2019-05-11T08:46:27Z

@jD91mZM2 this is likely not the place for it, but I would recommend kqueue over epoll. The API in my opinion is much easier to use and supports I/O, signals, timers and use space events all with 2 system calls.

jD91mZM2 · 2019-05-11T09:01:57Z

@Thomasdezeeuw I will look up kqueue and hear with the BDFL if he thinks implementing it instead (or alongside?) of his epoll implementation is worth it. Thanks a lot!

EDIT: This will require kernel support to be any better than a compatibility layer around epoll.

jackpot51 force-pushed the master branch from 1339f36 to 8e66da4 Compare June 14, 2018 08:18

jD91mZM2 force-pushed the master branch from d4f5587 to 94485d6 Compare July 4, 2018 11:00

jD91mZM2 mentioned this pull request Jul 4, 2018

Bump miow version #857

Closed

jD91mZM2 force-pushed the master branch from 94485d6 to 58cc190 Compare July 9, 2018 09:44

Thomasdezeeuw reviewed Jul 22, 2018

View reviewed changes

jackpot51 force-pushed the master branch from 489d180 to 58cc190 Compare July 23, 2018 07:19

Thomasdezeeuw reviewed Jul 23, 2018

View reviewed changes

Thomasdezeeuw approved these changes Jul 24, 2018

View reviewed changes

hawkw reviewed Aug 17, 2018

View reviewed changes

jD91mZM2 force-pushed the master branch 2 times, most recently from 31a1372 to 0a4b663 Compare August 18, 2018 10:00

carllerche requested changes Aug 20, 2018

View reviewed changes

jD91mZM2 force-pushed the master branch from e766b26 to 2092161 Compare August 26, 2018 10:47

jD91mZM2 and others added 20 commits December 24, 2018 13:53

Use the std::fs::File API instead of syscall

1821ed7

Update net2 fork

835d22d

Bump miow (and therefor winapi) version

1749dc6

Move miow fork to master branch

1b8f29c

Update links to gitlab

075ecce

Use more fs functions over redox_syscall

c856f4c

Don't specify rev

8e14d07

It's upstreaaaaameed baaaaaby!!!!

1296cdc

Use cargo version of net2

ab45820

Revert "Bump miow (and therefor winapi) version"

c26c7a9

This reverts commit cfd4711.

Clean up according to pull request comments

fc01d1d

Re-add from_listener

6dcc96e

Fix all redox-related redundant struct names

59b3264

Yes this commit message sucks you should really just squash all commits :)

Revert unrelated changes

857a395

Add Redox to CI

1f88c77

Fix compilation errors

32b840c

Separate connecting logic into platforms

ab19c99

Fix timing issue with 0ns timeouts

c6ee801

Revert "Separate connecting logic into platforms"

5a1a9c8

This reverts commit 2092161.

Redox now supports unbound fcntls

cc9a9f6

jackpot51 force-pushed the master branch from 8a4c08f to cc9a9f6 Compare December 24, 2018 14:19

jD91mZM2 and others added 2 commits December 24, 2018 16:57

Fix up issue after rebasing

493d2b6

Fix compilation if Redox becomes part of the unix target family

439a559

jD91mZM2 closed this May 10, 2019

Redox OS support #844

Redox OS support #844

Conversation

jD91mZM2 commented May 23, 2018

jD91mZM2 commented May 23, 2018

jD91mZM2 commented May 23, 2018

jD91mZM2 commented May 25, 2018

carllerche commented Jul 3, 2018

jD91mZM2 commented Jul 4, 2018

carllerche commented Jul 5, 2018

jD91mZM2 commented Jul 21, 2018

Thomasdezeeuw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jD91mZM2 Jul 23, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jD91mZM2 commented Jul 23, 2018 • edited Loading

jD91mZM2 commented Jul 23, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Thomasdezeeuw Jul 23, 2018 • edited Loading

Choose a reason for hiding this comment

Thomasdezeeuw commented Jul 23, 2018

jD91mZM2 commented Jul 23, 2018

carllerche commented Aug 17, 2018

carllerche commented Aug 17, 2018

hawkw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jD91mZM2 commented Aug 18, 2018

jD91mZM2 commented Aug 18, 2018

hawkw commented Aug 18, 2018

carllerche left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jD91mZM2 Aug 21, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jD91mZM2 commented Aug 21, 2018 • edited Loading

carllerche commented May 8, 2019

jD91mZM2 commented May 10, 2019

carllerche commented May 10, 2019

jD91mZM2 commented May 11, 2019 • edited Loading

Thomasdezeeuw commented May 11, 2019

jD91mZM2 commented May 11, 2019 • edited Loading

jD91mZM2 Jul 23, 2018 •

edited

Loading

jD91mZM2 commented Jul 23, 2018 •

edited

Loading

Thomasdezeeuw Jul 23, 2018 •

edited

Loading

jD91mZM2 Aug 21, 2018 •

edited

Loading

jD91mZM2 commented Aug 21, 2018 •

edited

Loading

jD91mZM2 commented May 11, 2019 •

edited

Loading

jD91mZM2 commented May 11, 2019 •

edited

Loading