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

Fix macOS fcntl/fsync usage properly #4025

Closed
wants to merge 1 commit into from

Conversation

ychin
Copy link
Contributor

@ychin ychin commented Feb 21, 2019

When fcntl fails, don't check errno when deciding whether to call fsync, as there are different possible errno results and they are not documented in the man page. Instead, always fall back to fsync. Worst case is that it's only slightly slower but that should only happen when something is wrong anyway so that's not a big deal.

Also see: #4016 for the first attempt to fix this.

When fcntl fails, don't check `errno` when deciding whether to call
fsync, as there are different possible `errno` results and they are not
documented in the man page. Instead, always fall back to fsync. Worst
case is that it's only slightly slower but that should only happen when
something is wrong anyway so that's not a big deal.
@ychin
Copy link
Contributor Author

ychin commented Feb 21, 2019

Per my response on the vim_mac mailing list (https://groups.google.com/forum/#!topic/vim_mac/LVjNiPjhnxc):

Hi Bram, you have already merged my pull request, but I looked at the corresponding commit in Neovim that got spawned from this change, and it referenced this commit in libuv that has more details: libuv/libuv#2135

From that commit I think we are currently missing a case for webdav because we are not handling the case where it sets errno to "EINVAL". I think we should probably just do what that commit does and ignore errno altogether. The worst case is it will be slightly slower but that's only if something is wrong anyway. Otherwise it will be whack-a-mole with random undocumented errno's that are probably not intended to be used this way. I would think Apple just assumes the programmer fall back to fsync if fcntl failed for whatever reason.

@codecov-io
Copy link

Codecov Report

Merging #4025 into master will decrease coverage by 0.01%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #4025      +/-   ##
==========================================
- Coverage   79.15%   79.14%   -0.02%     
==========================================
  Files         105      105              
  Lines      141126   141126              
==========================================
- Hits       111707   111689      -18     
- Misses      29419    29437      +18
Impacted Files Coverage Δ
src/fileio.c 75.44% <ø> (ø) ⬆️
src/gui_beval.c 61.57% <0%> (-0.88%) ⬇️
src/gui.c 58% <0%> (-0.57%) ⬇️
src/gui_gtk_x11.c 48.32% <0%> (-0.15%) ⬇️
src/window.c 83.49% <0%> (-0.14%) ⬇️
src/sign.c 93.38% <0%> (-0.13%) ⬇️
src/ui.c 50.14% <0%> (-0.08%) ⬇️
src/terminal.c 78.21% <0%> (-0.05%) ⬇️
src/screen.c 80.32% <0%> (+0.02%) ⬆️
src/channel.c 83.22% <0%> (+0.07%) ⬆️
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8caa43d...2c69cbc. Read the comment docs.

@brammool brammool closed this in 9166838 Feb 21, 2019
@ychin ychin deleted the mac-fsync-proper-fix branch February 22, 2019 08:38
asfgit pushed a commit to apache/apr that referenced this pull request Dec 16, 2020
Follow up to r1790200: fall back to fsync() if F_FULLFSYNC is not supported
by the file descriptor, filesystem or underlying device.

See, e.g.: vim/vim#4025


git-svn-id: https://svn.apache.org/repos/asf/apr/apr/branches/1.7.x@1883341 13f79535-47bb-0310-9956-ffa450edef68
asfgit pushed a commit to apache/apr that referenced this pull request Dec 16, 2020
…ported

by the file descriptor, filesystem or underlying device.

See, e.g.: vim/vim#4025


git-svn-id: https://svn.apache.org/repos/asf/apr/apr/trunk@1883340 13f79535-47bb-0310-9956-ffa450edef68
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants