Skip to content
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

Removing unsafe by replacing libc with nix #15

Open
yoshuawuyts opened this issue Oct 29, 2018 · 1 comment
Open

Removing unsafe by replacing libc with nix #15

yoshuawuyts opened this issue Oct 29, 2018 · 1 comment

Comments

@yoshuawuyts
Copy link

yoshuawuyts commented Oct 29, 2018

Hi!

First off: thanks so much for making this project! I've learned a lot just by reading it, which is fantastic!

Observation

I was going through the source today (on the futures-0.3 branch actually ✨) and something that stood out to me is that there's quite some unsafe sprinkled throughout the code to interact with the OS through libc. nix is a package which aims to make using libc's functionality safe, removing the need for unsafe.

I feel this would have a few benefits:

  • It would emphasize that it's possible to write executors in (mostly) safe code.
  • It's probably a bit more readable for most people (e.g. no need to worry unsafe and null pointers).

Example

For example the select(2) loop in the reactor currently looks like this:

let rv = unsafe {
    select(
        nfds,
        &mut read_fds,
        &mut write_fds,
        std::ptr::null_mut(),
        &mut tv,
    )
};

// don't care for errors
if rv == -1 {
    panic!("select()");
} else if rv == 0 {
    debug!("timeout");
} else {
    debug!("data available on {} fds", rv);
}

Using the nix select function this could be rewritten to something like:

// don't care for errors
let rv = select(
    Some(nfds),
    Some(&mut read_fds),
    Some(&mut write_fds),
    None,
    Some(&mut tv),
).unwrap();

debug!("data available on {} fds", rv);

Which might feel a bit more natural for most Rustaceans.

Conclusion

We made a case for replacing the usage of libc with the nix crate, citing potential benefits, and created an example conversion using existing code. I hope this issue might come in helpful. Thanks for your time!

@polachok
Copy link
Owner

polachok commented Oct 30, 2018

Hi,

I'm happy the project was useful for you!

Now to the point, I actually considered using nix, but decided against it for two reasons:

1.minimal dependencies
I think, in a teaching project, it makes sense to minimize dependencies, as every dependency adds cognitive load. A reader may be unfamiliar with nix, so they would have to consult nix docs. On the other hand, libc::select makes it clear that this is a well known C select function.

2.nix being huge
Nix covers a lot of APIs and consists of ~20k lines + 4 dependencies. Well, even mio dropped nix because it's heavy.

So, considering this, what do you think about adding a safe wrapper around select to fahrenheit itself? It would look as safe as nix in the main file, but a curious reader could inspect it in some util.rs right here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants