Skip to content

std: Make unlink() more consistent #15227

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

Merged
merged 1 commit into from
Jul 15, 2014
Merged

Conversation

alexcrichton
Copy link
Member

Currently when a read-only file has unlink() invoked on it on windows, the call
will fail. On unix, however, the call will succeed. In order to have a more
consistent behavior across platforms, this error is recognized on windows and
the file is changed to read-write before removal is attempted.

@alexcrichton alexcrichton changed the title std: Make unlink()'s more consistent std: Make unlink() more consistent Jun 27, 2014
@huonw
Copy link
Member

huonw commented Jun 28, 2014

Just for completeness: did you consider doing the reverse (rejecting unlink on a read-only file on unix)? If so, what was the reason for rejecting that?

@bill-myers
Copy link
Contributor

@huonw That would be completely wrong in Unix, since unlink removes the name from the directory, which has nothing to do with whether the file is writable or not, but depends of course on whether the directory is writable or not.

@alexcrichton However, the code seems to suffer from race conditions, since it will fail if someone unsets the read-only attribute just after the unlink fails, but it looks like this race condition is unfixable if ERROR_ACCESS_DENIED is returned for other errors too (however, retrying N times might be better).

It appears, from web searches (https://vtopan.wordpress.com/2009/05/26/using-ntdeletefile-from-delphi/), that the NT API NtDeleteFile will delete read-only files too (and also delete the file immediately, like Unix): maybe that could be the best solution if it's indeed the case.

@alexcrichton
Copy link
Member Author

Just for completeness: did you consider doing the reverse (rejecting unlink on a read-only file on unix)? If so, what was the reason for rejecting that?

I suppose I hadn't given it an overwhelming amount of thought, no! I suppose I conceptually would rather just delete a file when I call unlink rather than check whether it's actually writable, but I would suspect that I am mostly just biased towards unix.

@alexcrichton
Copy link
Member Author

However, the code seems to suffer from race conditions

That is correct, and this was written with that in mind. I would think that essentially any filesystem interaction is inherently racy, however, so trying to protect this one case on windows may not be buying us much.

the NT API NtDeleteFile will delete read-only files too

Interesting! I was under the impression that Nt-prefix things were internal-use-only and weren't recommended for use? This is also a problem with libgreen and would require upstreaming something to libuv as well.

Currently when a read-only file has unlink() invoked on it on windows, the call
will fail. On unix, however, the call will succeed. In order to have a more
consistent behavior across platforms, this error is recognized on windows and
the file is changed to read-write before removal is attempted.
bors added a commit that referenced this pull request Jul 14, 2014
Currently when a read-only file has unlink() invoked on it on windows, the call
will fail. On unix, however, the call will succeed. In order to have a more
consistent behavior across platforms, this error is recognized on windows and
the file is changed to read-write before removal is attempted.
@bors bors closed this Jul 15, 2014
@bors bors merged commit fe67d26 into rust-lang:master Jul 15, 2014
@alexcrichton alexcrichton deleted the windows-unlink branch July 15, 2014 02:22
bors added a commit to rust-lang-ci/rust that referenced this pull request Jul 17, 2023
…nicola

fix: Indent after pressing enter on a blank line

Regressed after rust-lang/rust-analyzer#13975 (whoops).
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

Successfully merging this pull request may close these issues.

4 participants