Use IoResult in the std::os module by barosl · Pull Request #18870 · rust-lang/rust

barosl · 2014-11-11T09:03:01Z

Make old-fashioned functions in the std::os module utilize IoResult.

I'm still investigating the possibility to include more functions in this pull request. Currently, it covers getcwd(), make_absolute(), and change_dir(). The issues covered by this PR are #16946 and #16315.

A few concerns:

Should we provide OsError in distinction from IoError? I'm saying this because in Python, those two are distinguished. One advantage that we keep using IoError is that we can make the error cascade down other functions whose return type also includes IoError. An example of such functions is std::io::TempDir::new_in(), which uses os::make_absolute() as well as returns IoResult<TempDir>.
os::getcwd() uses an internal buffer whose size is 2048 bytes, which is passed to getcwd(3). There is no upper limitation of file paths in the POSIX standard, but typically it is set to 4096 bytes such as in Linux. Should we increase the buffer size? One thing that makes me nervous is that the size of 2048 bytes already seems a bit excessive, thinking that in normal cases, there would be no filenames that even exceeds 512 bytes.

Fixes#16946.
Fixes#16315.

Any ideas are welcomed. Thanks!

rust-highfive · 2014-11-11T09:03:10Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @brson (or someone else) soon.

barosl · 2014-11-11T18:04:10Z

I've also added os::setenv() and os::unsetenv() to the list. They can fail if the name is invalid, or the memory is insufficient.

However, I'm a bit worried that from now, whenever setenv() is used, the unused_must_use lint will occur if the return value is not used explicitly. This can be very annoying.

If you're not fine with this, I can remove the commit.

alexcrichton · 2014-11-14T07:09:56Z

I think that getcwd and change_dir are probably OK to return IoResult, but for setenv and unsetenv I would rather not return IoResult as they are quite common to call. The "insufficient memory" use case panicking is aligned mostly with our "OOM == abort" behavior today, and I would suspect that the number of invalid names is rare enough that it would be more ergonomic to panic instead of returning an IoResult.

I'm also thinking that there may be a better error to expose here than IoResult because it's more of an OS error than an "IO error", but we just don't have a good abstraction for that today. For now I think it's ok to use IoResult, but certainly something interesting to think about! cc @aturon

huonw · 2014-11-14T07:17:04Z

(BTW, @barosl, you can automatically close issues when a PR lands by including "Fixes #..." in the commit/description. 😄 I've taken the liberty to edit this in myself.)

barosl · 2014-11-14T07:30:59Z

@alexcrichton Oh, that's good. As the return value of setenv(3) had been ignored(It didn't even panic), I only considered using IoResult to handle it. But, as you said, we can just simply panic!(). That sounds reasonable.

@huonw Thanks for informing me. I will rebase it later.

barosl · 2014-11-14T19:22:42Z

setenv() and unsetenv() are made to panic, and the branch is rebased onto the current HEAD! Good to go.

aturon · 2014-11-14T19:28:24Z

FWIW, I agree with @alexcrichton here. Thanks for this PR!

barosl · 2014-11-14T21:20:20Z

Oops, sorry. The build has failed because I forgot to import io::OtherIoError in the Windows code. It is now fixed.

barosl · 2014-11-17T12:56:47Z

I'm so embarrassed to have broken this again. I believe it builds well now. Please, @alexcrichton!

nodakai · 2014-11-17T14:11:23Z

@barosl What a coincidence, I was also working on getcwd, too. I'll rebase mine onto yours once yours is merged to master.

I think you must update src/test/run-pass/*.rs; some of them use getcwd() and change_dir(). Did you run make check?

barosl · 2014-11-17T14:51:13Z

@nodakai Oh, they do! Thanks for telling me. I've also included them. As my old laptop emits an out of memory error while compiling Rust, I cannot run the tests by myself. What a shameful reason for a failing PR...

It seems that your work also includes some improvements on Windows. I'm willing to see it!

nodakai · 2014-11-18T01:51:51Z

return false here caused an error in Win32 Buildbot. We should simply call unwrap() on p.as_str() since an invalid UTF-8 string should be caught upon constructioin of windows::Path.

You're right! It seems that the actual implementation of as_str() on Windows is:
fnas_str<'a>(&'aself) -> Option<&'astr>{Some(self.repr.as_slice())}
So there won't be a problem.
Just in case, is this related to #13338? I heard that Windows can actually save non-UTF-16-compatible filenames. In that case, should we deal with it? Or, can we just simply ignore it because the corruption is still caught when creating Path?

I've never thought of such a case! So, in essense, Windows (NTFS?) allows to create a directory whose name is an invalid UTF-16 string which doesn't have a corresponding UTF-8 string... That means we currently have no way to refer to such a directory by windows::Path in Rust.
That's awful, but I think it's better to resolve that problem elsewhere, because it's not specific to getcwd(). A workaround might be using the DOS-style "8.3" filename returned by GetShortPathName().

barosl · 2014-11-18T16:56:32Z

I changed getcwd() to use Path.as_str().unwrap() on Windows as suggested by @nodakai. Also, as #18645 broke this pull request, I resolved the conflict.

r again?

mahkoh · 2014-11-18T17:10:11Z

What does invalid mean? There are many valid paths where this will fail.

The following "possible cases" clause states that there are at least two situations. Do you have any suggestions to add?

Any path longer than the buffer.

IoResult is either Ok or Err(IoError{..}). None is for Option.

Oops! That was because I once tried to use Option for the return value. Now it is fixed. Thanks, @nodakai!

alexcrichton · 2014-11-18T17:35:06Z

Looks like this needs a rebase @barosl.

barosl · 2014-11-18T18:08:44Z

@alexcrichton Uhm? I had a rebase to master as usual. It seems OK to me, did I make a mistake?

alexcrichton · 2014-11-18T18:10:10Z

Ah I think some other PRs just landed in the interim (this doesn't merge cleanly right now)

barosl · 2014-11-18T18:28:19Z

I applied the suggestion by @mahkoh, and also rebased again just in case.

nodakai · 2014-11-18T19:12:14Z

You may find the try! macro or Result::map() convenient for error handling here, and in make_absolute().

Cool, I should have known them earlier! They fit perfectly. My huge thanks to @nodakai!

os::getcwd() panics if the current directory is not available. According to getcwd(3), there are three cases: - EACCES: Permission denied. - ENOENT: The current working directory has been removed. - ERANGE: The buffer size is less than the actual absolute path. This commit makes os::getcwd() return IoResult<Path>, not just Path, preventing it from panicking. As os::make_absolute() depends on os::getcwd(), it is also modified to return IoResult<Path>. Fixesrust-lang#16946. [breaking-change]

os::change_dir() returns bool, without a meaningful error message. Change it to return IoResult<()> to indicate what IoError caused the failure. Fixesrust-lang#16315. [breaking-change]

These functions can fail if: - EINVAL: The name is empty, or contains an '=' character - ENOMEM: Insufficient memory

barosl · 2014-11-18T20:36:34Z

Good to go again!

Make old-fashioned functions in the `std::os` module utilize `IoResult`. I'm still investigating the possibility to include more functions in this pull request. Currently, it covers `getcwd()`, `make_absolute()`, and `change_dir()`. The issues covered by this PR are #16946 and #16315. A few concerns: - Should we provide `OsError` in distinction from `IoError`? I'm saying this because in Python, those two are distinguished. One advantage that we keep using `IoError` is that we can make the error cascade down other functions whose return type also includes `IoError`. An example of such functions is `std::io::TempDir::new_in()`, which uses `os::make_absolute()` as well as returns `IoResult<TempDir>`. - `os::getcwd()` uses an internal buffer whose size is 2048 bytes, which is passed to `getcwd(3)`. There is no upper limitation of file paths in the POSIX standard, but typically it is set to 4096 bytes such as in Linux. Should we increase the buffer size? One thing that makes me nervous is that the size of 2048 bytes already seems a bit excessive, thinking that in normal cases, there would be no filenames that even exceeds 512 bytes. Fixes#16946. Fixes#16315. Any ideas are welcomed. Thanks!

Drop unnecessary tracing::warn

rust-highfive assigned brson Nov 11, 2014

barosl force-pushed the os-ioresult branch 2 times, most recently from 478d064 to 79ca2deCompare November 11, 2014 17:59

barosl force-pushed the os-ioresult branch 2 times, most recently from ab3af5c to 3de5302Compare November 11, 2014 20:24

barosl force-pushed the os-ioresult branch 2 times, most recently from 71e477e to 9d54385Compare November 14, 2014 19:21

barosl force-pushed the os-ioresult branch 2 times, most recently from 5dcf9fc to ffb51a5Compare November 14, 2014 21:20

barosl force-pushed the os-ioresult branch from ffb51a5 to 9b147f7Compare November 17, 2014 12:48

barosl force-pushed the os-ioresult branch from 9b147f7 to 2a8578eCompare November 17, 2014 14:42

barosl force-pushed the os-ioresult branch from 2a8578e to 610d19fCompare November 17, 2014 14:56

nodakai reviewed Nov 18, 2014
View reviewed changes

barosl force-pushed the os-ioresult branch from 610d19f to bc580e7Compare November 18, 2014 16:37

mahkoh reviewed Nov 18, 2014
View reviewed changes

barosl force-pushed the os-ioresult branch 2 times, most recently from a6cce68 to 40ff717Compare November 18, 2014 18:25

barosl force-pushed the os-ioresult branch from 40ff717 to fe3e9bfCompare November 18, 2014 18:57

nodakai reviewed Nov 18, 2014
View reviewed changes

barosl added 3 commits November 19, 2014 05:31

Make os::change_dir() return IoResult<()>
5de56b3
os::change_dir() returns bool, without a meaningful error message. Change it to return IoResult<()> to indicate what IoError caused the failure. Fixesrust-lang#16315. [breaking-change]

Make os::setenv() and os::unsetenv() panic if an error occurs
b5286af
These functions can fail if: - EINVAL: The name is empty, or contains an '=' character - ENOMEM: Insufficient memory

barosl force-pushed the os-ioresult branch from fe3e9bf to b5286afCompare November 18, 2014 20:32

bors closed this Nov 18, 2014

gifnksm mentioned this pull request Nov 19, 2014
Update for latest rustc rust-lang/glob#9
Merged

lnicola pushed a commit to lnicola/rust that referenced this pull request Jan 7, 2025
Merge pull requestrust-lang#18870from Veykril/push-vnpsptpolyol
8e4409c
Drop unnecessary tracing::warn

Uh oh!

Uh oh!

Conversation

barosl commented Nov 11, 2014

Uh oh!

rust-highfive commented Nov 11, 2014

Uh oh!

barosl commented Nov 11, 2014

Uh oh!

alexcrichton commented Nov 14, 2014

Uh oh!

huonw commented Nov 14, 2014

Uh oh!

barosl commented Nov 14, 2014

Uh oh!

barosl commented Nov 14, 2014

Uh oh!

aturon commented Nov 14, 2014

Uh oh!

barosl commented Nov 14, 2014

Uh oh!

barosl commented Nov 17, 2014

Uh oh!

nodakai commented Nov 17, 2014

Uh oh!

barosl commented Nov 17, 2014

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

barosl commented Nov 18, 2014

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alexcrichton commented Nov 18, 2014

Uh oh!

barosl commented Nov 18, 2014

Uh oh!

alexcrichton commented Nov 18, 2014

Uh oh!

barosl commented Nov 18, 2014

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

barosl commented Nov 18, 2014

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants