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

Add Redox OS backend #32

Merged
merged 4 commits into from Aug 15, 2017

Conversation

Projects
None yet
3 participants
@ids1024
Copy link
Contributor

ids1024 commented Aug 12, 2017

No description provided.

@Stebalien

This comment has been minimized.

Copy link
Owner

Stebalien commented Aug 12, 2017

Nice! Thanks.

Unfortunately, that's a lot of duplicate code. Given that Redox is unix-like(ish), do you think you can merge this into the unix module (maybe introducing some form of general-purpose cvt_err function to abstract away the differences in the syscall library)?

@Stebalien

This comment has been minimized.

Copy link
Owner

Stebalien commented Aug 12, 2017

Alternatively, we could also add a nixc submodule that abstracts over the various low-level unix libc functions we need.

@ids1024

This comment has been minimized.

Copy link
Contributor Author

ids1024 commented Aug 12, 2017

In addition to errors, the Redox functions differ in that they accept u8 slices rather than pointers to c strings. I guess having functions wrapping those two things would probably make it possible to merge this with the unix implementation...

@Stebalien

This comment has been minimized.

Copy link
Owner

Stebalien commented Aug 12, 2017

My primary concern is that I can't test Redox easily so any non-trivial Redox only logic will likely experience severe bitrot.

@ids1024

This comment has been minimized.

Copy link
Contributor Author

ids1024 commented Aug 12, 2017

Okay, I've merged the Redox code into imp/unix.rs.

#[cfg(target_os = "redox")]
pub fn persist(old_path: &Path, new_path: &Path, overwrite: bool) -> io::Result<()> {
// XXX implement in better way when possible
if !overwrite && new_path.exists() {

This comment has been minimized.

@Stebalien

Stebalien Aug 14, 2017

Owner

Is there no way to do this atomically (avoid clobbering existing files)? We're currently guaranteeing that this is atomic.

This comment has been minimized.

@ids1024

ids1024 Aug 14, 2017

Author Contributor

I don't think so; this can be fixed once that is possible.

It's actually somewhat worse; rename is not atomic either currently (just copy and unlink); that really should be implemented, but it hasn't been done yet.

This comment has been minimized.

@Stebalien

Stebalien Aug 14, 2017

Owner

I thought I saw a link syscall... my primary concern is not clobbering existing files and I'd rather just always return a not supported error to the user than potentially overwrite a file.

This comment has been minimized.

@ids1024

ids1024 Aug 14, 2017

Author Contributor

There is a link function in redox_syscall, but hard links are not actually implemented yet.

This comment has been minimized.

@Stebalien

Stebalien Aug 14, 2017

Owner

Wait... by "copy and unlink" you mean it literally copies the file byte-by-byte? If that's the case, I think the only sane implementation here would be to return an error. The entire point of this method is that it allows the programmer to atomically replace an existing file with an updated file (and I've seen users rely on this guarantee).

pub fn create_named(path: &Path) -> io::Result<File> {
unsafe {
let fd = try!(cvt_err(open(path.as_os_str().as_bytes(),
O_CLOEXEC | O_EXCL | O_RDWR | O_CREAT | 0o600)));

This comment has been minimized.

@Stebalien

Stebalien Aug 14, 2017

Owner

I'm pretty sure the 0o600 is not supposed to be ORed with this. Is there any way to atomically set the permissions on this file?

This comment has been minimized.

@ids1024

This comment has been minimized.

@jackpot51

jackpot51 Aug 14, 2017

Contributor

Yes, open on Redox includes mode flags in the flags parameter.

This comment has been minimized.

@Stebalien

Stebalien Aug 14, 2017

Owner

Got it. Thanks!

Make persist() just return an error on Redox
This can't really be sanely implemented on Redox yet.
use syscall::{self, open, fstat, Stat as stat_t, O_EXCL, O_RDWR, O_CREAT, O_CLOEXEC};

#[cfg(not(target_os = "redox"))]
pub fn cvt_err(result: c_int) -> io::Result<c_int> {

This comment has been minimized.

@Stebalien

Stebalien Aug 14, 2017

Owner

Can we mark this as #[inline(always)]?

}

#[cfg(target_os = "redox")]
pub fn cvt_err(result: Result<usize, syscall::Error>) -> io::Result<usize> {

This comment has been minimized.

@Stebalien
@ids1024

This comment has been minimized.

Copy link
Contributor Author

ids1024 commented Aug 14, 2017

Fair enough; I've made persist return an error on Redox; that can be implemented in the same way as Unix once Redox supports rename and link.

I also added #[inline(always)].

@Stebalien Stebalien merged commit b1a57a0 into Stebalien:master Aug 15, 2017

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@Stebalien

This comment has been minimized.

Copy link
Owner

Stebalien commented Aug 15, 2017

Thanks!

@ids1024

This comment has been minimized.

Copy link
Contributor Author

ids1024 commented Sep 24, 2017

@Stebalien Would you mind doing a release, so Redox can use this without a Cargo.toml patch for the crate using this library?

@Stebalien

This comment has been minimized.

Copy link
Owner

Stebalien commented Sep 27, 2017

@ids1024 done. Sorry, it took so long; I'm traveling and don't have the best internet at the moment.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.