Skip to content

fs: incorrect use of O_SYNC? #6730

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

Closed
saghul opened this issue May 13, 2016 · 3 comments
Closed

fs: incorrect use of O_SYNC? #6730

saghul opened this issue May 13, 2016 · 3 comments
Labels
fs Issues and PRs related to the fs subsystem / file system. question Issues that look for answers.

Comments

@saghul
Copy link
Member

saghul commented May 13, 2016

  • Version: Node >= 0.7.9
  • Platform: Unix
  • Subsystem: fs

I somehow ran into fs.open's 'rs' mode, which is basically O_RDONLY | O_SYNC. All the docs I can find point out that O_SYNC is to be used for writing, making a write equivalent to write + fsync. The documentation mentions bypassing caches, which sounds more like O_DIRECT to me.

Is there something I'm missing or does the flag basically do nothing when used with a readonly file?

R=@bnoordhuis, since you landed the commit which introduced it: dfcdd5b

If it's indeed incorrect, i can PR a removal for the next major release.

@saghul saghul added question Issues that look for answers. fs Issues and PRs related to the fs subsystem / file system. labels May 13, 2016
@bnoordhuis
Copy link
Member

I don't remember what I was thinking at the time but, in retrospect, O_RDONLY + O_SYNC isn't useful, the O_SYNC is a no-op.

Presumably it's there for feature parity with the O_RDWR variant but it makes you wonder why we didn't add O_WRONLY variants as well...

I don't think it's necessary to remove 'rs' but I'd be okay with undocumenting it.

@saghul
Copy link
Member Author

saghul commented May 13, 2016

Ack, will do.

@saghul
Copy link
Member Author

saghul commented May 13, 2016

Fixed in #6732

evanlucas pushed a commit that referenced this issue May 17, 2016
Using O_SYNC with O_RDONLY is basically a noop.

Closes: #6730
PR-URL: #6732
Reviewed-By: Ben Noorhduis <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fs Issues and PRs related to the fs subsystem / file system. question Issues that look for answers.
Projects
None yet
Development

No branches or pull requests

2 participants