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 #ifdef O_DSYNC #15425

Closed
jorangreef opened this issue Sep 15, 2017 · 6 comments
Closed

Add #ifdef O_DSYNC #15425

jorangreef opened this issue Sep 15, 2017 · 6 comments
Labels
feature request Issues that request new features to be added to Node.js. fs Issues and PRs related to the fs subsystem / file system. good first issue Issues that are suitable for first-time contributors.

Comments

@jorangreef
Copy link
Contributor

node_constants.cc exports O_SYNC on Linux, but O_DSYNC should probably be added.

@addaleax addaleax added feature request Issues that request new features to be added to Node.js. fs Issues and PRs related to the fs subsystem / file system. good first issue Issues that are suitable for first-time contributors. labels Sep 15, 2017
@jrasanen
Copy link
Contributor

I could try to do this

@addaleax
Copy link
Member

@jrasanen awesome! let us know if you need any pointers :)

@jrasanen
Copy link
Contributor

jrasanen commented Sep 16, 2017

How would one write a test case for testing if O_DSYNC is being used?

I made the changes and verified the code using strace,

$ sudo strace -e trace=open -f ./node write_cats_to_tmp.js
[pid  9226] open("/vagrant/node/write_cats_to_tmp.js", O_RDONLY|O_CLOEXEC) = 12
Wrote to /tmp/cats.tmp
[pid  9232] open("/tmp/cats.tmp", O_WRONLY|O_CREAT|O_TRUNC|O_DSYNC|O_CLOEXEC, 0666) = 12
Writing to file
Wrote 9 bytes

@bnoordhuis
Copy link
Member

Since the flag has no directly observable effect, I think the best you can do is check that it's there when common.isLinux === true and that it's accepted by fs.open() without error.

@jrasanen
Copy link
Contributor

@bnoordhuis ok thanks! Which place you would recommend I add the test to? test-fs-open-flags.js?

@bnoordhuis
Copy link
Member

Yes, that's a good place for it.

@jrasanen jrasanen mentioned this issue Sep 18, 2017
4 tasks
addaleax pushed a commit to addaleax/ayo that referenced this issue Sep 23, 2017
PR-URL: nodejs/node#15451
Fixes: nodejs/node#15425
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
tniessen pushed a commit to tniessen/node that referenced this issue Oct 3, 2017
PR-URL: nodejs#15451
Fixes: nodejs#15425
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
MylesBorins pushed a commit that referenced this issue Oct 7, 2017
Backport-PR-URL: #15653
PR-URL: #15451
Fixes: #15425
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
MylesBorins pushed a commit that referenced this issue Oct 11, 2017
Backport-PR-URL: #15653
PR-URL: #15451
Fixes: #15425
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request Issues that request new features to be added to Node.js. fs Issues and PRs related to the fs subsystem / file system. good first issue Issues that are suitable for first-time contributors.
Projects
None yet
Development

No branches or pull requests

4 participants