Skip to content

eval: Make writefile() able to disable fsync() #6427

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 12 commits into from
Apr 3, 2017
Merged

eval: Make writefile() able to disable fsync() #6427

merged 12 commits into from
Apr 3, 2017

Conversation

ZyX-I
Copy link
Contributor

@ZyX-I ZyX-I commented Apr 2, 2017

Ref #6420

@ZyX-I ZyX-I mentioned this pull request Apr 2, 2017
Closed
@ZyX-I
Copy link
Contributor Author

ZyX-I commented Apr 2, 2017

As fsync() is a safer option, it is on by default. As other flags enable something and are lowercase, flag that disables fsync() is uppercase.

@justinmk
Copy link
Member

justinmk commented Apr 2, 2017

Shouldn't writefile() default behavior be decided by 'fsync' option? And then overridden with s/S flag pair.

@blueyed
Copy link
Contributor

blueyed commented Apr 2, 2017

@justinmk
Good idea, but the documentation for 'fsync' could be interpreted as not applying here (but only to :w etc).
I'm assuming that Vim does not fsync with writefile(), does it?

@blueyed
Copy link
Contributor

blueyed commented Apr 2, 2017

Just for reference: any flag for writefile will trigger the binary flag in old Vim versions, where a trueish value enables this, and no other flags were known yet. So using S would always trigger it, but that should be handled in plugins by checking for a patch level probably.

@justinmk
Copy link
Member

justinmk commented Apr 2, 2017

Good idea, but the documentation for 'fsync' could be interpreted as not applying here (but only to :w etc).

Right, but I'm suggesting that we apply it anyway.

@ZyX-I
Copy link
Contributor Author

ZyX-I commented Apr 2, 2017

@blueyed Vim does not fsync() neither with writefile() nor when writing viminfo (second user of buffered writing is ShaDa). Basically you can’t use FILE*, fsync and keep up with the standard: even if fileno() is available (it is POSIX, not C99), it is required that you should not mix unistd API with stdio API, this may yield unexpected results.

Though fsync() with the whole fd-related API is also BSD/POSIX/SUS and not C99, yet available on Windows (but specifically fsync() is absent, libuv uses FlushFileBuffers and we are using libuv).

@ZyX-I
Copy link
Contributor Author

ZyX-I commented Apr 2, 2017

ag finds exactly three occurrences of p_fs (fsync option): fileio.c with #ifdef UNIX guard (BTW, I think I should remove it, libuv has windows implementation for that), globals.h and options.lua.

Note that our help does not warn about “systems without fsync implementation” like Vim documentation. Yet there is a guard around the only place where p_fs is checked.

@justinmk
Copy link
Member

justinmk commented Apr 2, 2017

ag finds exactly three occurrences of p_fs (fsync option): fileio.c with #ifdef UNIX guard (BTW, I think I should remove it, libuv has windows implementation for that),

Yes, probably should remove it. Most likely we overlooked it because Windows was "todo" for a long time. But the documentation was updated in anticipation of convergence.

ZyX-I added 9 commits April 3, 2017 00:35
@ZyX-I
fileio: Refactor errmsg handling
364709b 
Adds os_strerror() result to a number of places. Also since I could not track 
where err\* variables are NULL and where they are not, using macros to make sure 
that all three variables are set at once.

Removes #ifdef UNIX around the use of os_fsync, makes it use os_close in place 
of close in some places.
@ZyX-I
message: Do not use IObuff in emsgf
8dd9c6e
@ZyX-I
fixup!
8eb598c
@ZyX-I
fileio: Clean up IObuff-manipulation mess
1c41b9c
@ZyX-I
fileio: Refactor msg_add_fname to something which needs no comments
5dcf280
@ZyX-I
eval: Add s flag, use p_fs by default, error out on unknown flag
97a7f47
@ZyX-I
fileio: Save details about E212 error
2dbd49f
@ZyX-I
functests: Test some :write errors
9912043
@ZyX-I
tests: Fix testlint errors
dc75766
@ZyX-I
Copy link
Contributor Author

ZyX-I commented Apr 3, 2017

My watchdog is working (which restarts single includes test), but this is not very useful: “hang up” was replaced with “CMakeFiles/test-includes-api-window-h.dir/auto/api/window.h.test-include.c.o: file not recognized: File truncated” and “/usr/bin/ld: cannot find CMakeFiles/test-includes-mark_defs-h.dir/auto/mark_defs.h.test-include.c.o: No such file or directory”.

ZyX-I
ZyX-I commented Apr 3, 2017
View reviewed changes
test/functional/ui/cursor_spec.lua
local insert, execute = helpers.insert, helpers.execute
local eq, funcs = helpers.eq, helpers.funcs
local clear, meths = helpers.clear, helpers.meths
local eq = helpers.eq
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BTW, how that appeared in master? I did not touch the file, yet tests fail.

Copy link
Member

@justinmk justinmk Apr 3, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My fault. In the 'guicursor' PR, there were some changes which intentionally fail linter. But I forgot to check testlint on the last round.

@ZyX-I
Copy link
Contributor Author

ZyX-I commented Apr 3, 2017

Will have to merge despite linter error: I am not really up to splitting buf_write.

@blueyed
Copy link
Contributor

blueyed commented Apr 3, 2017

I wonder if it should really use fsync by default?!
Typically you call this manually, or implicit when closing a file.
(A file cannot be closed though in VimL, can it?)

@ZyX-I
Copy link
Contributor Author

ZyX-I commented Apr 3, 2017

@blueyed Typically software does not bother to call fsync(), it is never done implicitly and “manually” you can only fsync all files in the whole system. What does call fsync() is something old-school like Vim: software whose developers assume that crashing system is relatively common. E.g. in Python you will get fsync() only if you use posix module, neither file.flush() nor file.close() do that.

@ZyX-I
Copy link
Contributor Author

ZyX-I commented Apr 3, 2017

@blueyed And writefile() does close file. As well as e.g. :write first opens it and then closes back (after writing).

@ZyX-I
Copy link
Contributor Author

ZyX-I commented Apr 3, 2017

When file is opened in a buffer it is not opened from the system POV: file descriptor is created and associated with the file only temporary, when you read or write a file, disappearing after finishing reading or writing.

@justinmk
Copy link
Member

justinmk commented Apr 3, 2017 •
edited
Loading

I wonder if it should really use fsync by default?!

You can control the default, e.g. :set nofsync. Doesn't it make sense for writefile() to respect that?

@justinmk justinmk merged commit 6afa7d6 into neovim:master Apr 3, 2017
@ZyX-I ZyX-I deleted the writefile-allow-omitting-fsync branch April 3, 2017 11:10
justinmk added a commit to justinmk/neovim that referenced this pull request May 1, 2017
@justinmk
NVIM v0.2.0
f878110 
FEATURES:
    bc4a2e1 help, man.vim: "outline" (TOC) feature neovim#5169
    58422f1 'guicursor' works in the TUI (and sends info to UIs) neovim#6423
    129f107 api: nvim_get_mode() neovim#6247
    0b59f98 api/ui: externalize tabline neovim#6583
    bc6d868 'listchars': `Whitespace` highlight group neovim#6367
    6afa7d6 writefile() obeys 'fsync' option neovim#6427
    c60e409 eval.c refactor (also improves some error messages) neovim#5119
    9d200cd getcompletion("cmdline") neovim#6376
    2ea7bfc terminal: Support extra arguments in 'shell'. neovim#4504
    bf51102 DirChanged autocmd neovim#5928 neovim#6262
    1743df8 'cpoptions': "_" flag to toggle `cw` behaviour neovim#6235
    22337b1 CTRL-R omits trailing ^M when pasting to cmdline neovim#6137
    0e44916 :edit allows unescaped spaces in filename neovim#6119
    abdbfd2 eval: Add id() function and make printf("%p") useful neovim#6095
    bdfa147 findfile(), :find, gf work in :terminal. neovim#6009
    2f38ed1 providers: Disable if `g:loaded_*` exists.
    b5560a6 setpos() can set lowercase marks in other buffers neovim#5753
    7c513d6 Throttle :! output, pulse "..." message. neovim#5396
    d2e8c76 v:exiting neovim#5651

    :terminal improvements neovim#6185 neovim#6142
      - cursor keeps position after leaving insert-mode.
      - 4ceec30 Follows output only if cursor is at end of buffer.
      - e7bbd35 new option: 'scrollback'
      - fedb844 quasi-support for undo and 'modifiable'
      - b45ddf7 disables 'list' by default
      - disables 'relativenumber' by default

    :help now contains full API documentation at `:help api`.

    man.vim saw numerous improvements.

    Windows support:
      - Windows is no longer "experimental", it is fully supported.
      - Windows package includes a GUI, curl.exe and other utilities.

    "Vim 8" features: partials, lambdas, packages.

FIXES:
    12fc1de ops: fix i<c-r> with multi-byte text neovim#6524
    dd391bf Windows: system() and friends neovim#6497
    13352c0 Windows: os_get_hostname() neovim#6413
    16babc6 tui: Less-noisy mouse seqs neovim#6411
    3a9dd13 (vim bug) folding edge-cases  neovim#6207
    f6946c6 job-control: set CLOEXEC on pty processes. neovim#5986
    d1afd43 rplugin: Call s:LoadRemotePlugins() on startup.
    1215084 backtick-expansion works with `shell=fish` neovim#6224
    e32ec03 tui: Improved behavior after resize. neovim#6202
    86c2adc edit.c: CTRL-SPC: Insert previously-inserted text. neovim#6090
    c318d8e b:changedtick now follows VimL rules neovim#6112
    34e24cb terminal: Initialize colors in reverse order neovim#6160
    e889917 undo: Don't set b_u_curhead in ex_undojoin() neovim#5869
    d25649f undo: :earlier, g-: Set b_u_seq_cur correctly. (neovim#6016)
    043d8ba 'Visual-mode put from @. register' neovim#5782
    42c922b open_buffer(): Do `BufEnter` for directories.
    50d0d89 inccommand: Preview :sub commands only after delimiter neovim#5932
    1420e10 CheckHealth improvements neovim#5519
    c8d5e92 jobstart(): Return -1 if cmd is not executable. neovim#5671

CHANGES:
    NVIM_TUI_ENABLE_CURSOR_SHAPE was removed. Use 'guicursor' instead.
        See https://github.com/neovim/neovim/wiki/Following-HEAD#20170402

    81525dc 'mouse=a' is no longer the default. (This will probably
                 change again after it is improved.) neovim#6022

    0c1f783 defaults: 'showcmd', 'belloff', 'ruler' neovim#6087
    eb0e94f api: {get,set}_option update local options as appropriate neovim#6405
    bdcb2a3 "Reading from stdin..." message was removed. neovim#6298
@justinmk justinmk mentioned this pull request May 1, 2017
Merged
justinmk added a commit to justinmk/neovim that referenced this pull request May 1, 2017
@justinmk
NVIM v0.2.0
2d1cb8a 
FEATURES:
    bc4a2e1 help, man.vim: "outline" (TOC) feature neovim#5169
    58422f1 'guicursor' works in the TUI (and sends info to UIs) neovim#6423
    129f107 api: nvim_get_mode() neovim#6247
    0b59f98 api/ui: externalize tabline neovim#6583
    bc6d868 'listchars': `Whitespace` highlight group neovim#6367
    6afa7d6 writefile() obeys 'fsync' option neovim#6427
    c60e409 eval.c refactor (also improves some error messages) neovim#5119
    9d200cd getcompletion("cmdline") neovim#6376
    2ea7bfc terminal: Support extra arguments in 'shell'. neovim#4504
    bf51102 DirChanged autocmd neovim#5928 neovim#6262
    1743df8 'cpoptions': "_" flag to toggle `cw` behaviour neovim#6235
    22337b1 CTRL-R omits trailing ^M when pasting to cmdline neovim#6137
    0e44916 :edit allows unescaped spaces in filename neovim#6119
    abdbfd2 eval: Add id() function and make printf("%p") useful neovim#6095
    bdfa147 findfile(), :find, gf work in :terminal. neovim#6009
    2f38ed1 providers: Disable if `g:loaded_*` exists.
    b5560a6 setpos() can set lowercase marks in other buffers neovim#5753
    7c513d6 Throttle :! output, pulse "..." message. neovim#5396
    d2e8c76 v:exiting neovim#5651

    :terminal improvements neovim#6185 neovim#6142
      - cursor keeps position after leaving insert-mode.
      - 4ceec30 Follows output only if cursor is at end of buffer.
      - e7bbd35 new option: 'scrollback'
      - fedb844 quasi-support for undo and 'modifiable'
      - b45ddf7 disables 'list' by default
      - disables 'relativenumber' by default

    :help now contains full API documentation at `:help api`.

    man.vim saw numerous improvements.

    Windows support:
      - Windows is no longer "experimental", it is fully supported.
      - Windows package includes a GUI, curl.exe and other utilities.

    "Vim 8" features: partials, lambdas.

SECURITY FIXES:
    CVE-2017-5953 CVE-2017-6349 CVE-2017-6350 neovim#6485

CHANGES:
    NVIM_TUI_ENABLE_CURSOR_SHAPE was removed. Use 'guicursor' instead.
        See https://github.com/neovim/neovim/wiki/Following-HEAD#20170402

    81525dc 'mouse=a' is no longer the default. (This will probably
                 change again after it is improved.) neovim#6022

    0c1f783 defaults: 'showcmd', 'belloff', 'ruler' neovim#6087
    eb0e94f api: {get,set}_option update local options as appropriate neovim#6405
    bdcb2a3 "Reading from stdin..." message was removed. neovim#6298

FIXES:
    12fc1de ops: fix i<c-r> with multi-byte text neovim#6524
    dd391bf Windows: system() and friends neovim#6497
    13352c0 Windows: os_get_hostname() neovim#6413
    16babc6 tui: Less-noisy mouse seqs neovim#6411
    3a9dd13 (vim bug) folding edge-cases  neovim#6207
    f6946c6 job-control: set CLOEXEC on pty processes. neovim#5986
    d1afd43 rplugin: Call s:LoadRemotePlugins() on startup.
    1215084 backtick-expansion works with `shell=fish` neovim#6224
    e32ec03 tui: Improved behavior after resize. neovim#6202
    86c2adc edit.c: CTRL-SPC: Insert previously-inserted text. neovim#6090
    c318d8e b:changedtick now follows VimL rules neovim#6112
    34e24cb terminal: Initialize colors in reverse order neovim#6160
    e889917 undo: Don't set b_u_curhead in ex_undojoin() neovim#5869
    d25649f undo: :earlier, g-: Set b_u_seq_cur correctly. (neovim#6016)
    043d8ba 'Visual-mode put from @. register' neovim#5782
    42c922b open_buffer(): Do `BufEnter` for directories.
    50d0d89 inccommand: Preview :sub commands only after delimiter neovim#5932
    1420e10 CheckHealth improvements neovim#5519
    c8d5e92 jobstart(): Return -1 if cmd is not executable. neovim#5671
justinmk added a commit to justinmk/neovim that referenced this pull request May 1, 2017
@justinmk
NVIM v0.2.0
52727d9 
FEATURES:
    bc4a2e1 help, man.vim: "outline" (TOC) feature neovim#5169
    58422f1 'guicursor' works in the TUI (and sends info to UIs) neovim#6423
    129f107 api: nvim_get_mode() neovim#6247
    0b59f98 api/ui: externalize tabline neovim#6583
    bc6d868 'listchars': `Whitespace` highlight group neovim#6367
    6afa7d6 writefile() obeys 'fsync' option neovim#6427
    c60e409 eval.c refactor (also improves some error messages) neovim#5119
    9d200cd getcompletion("cmdline") neovim#6376
    2ea7bfc terminal: Support extra arguments in 'shell'. neovim#4504
    bf51102 DirChanged autocmd neovim#5928 neovim#6262
    1743df8 'cpoptions': "_" flag to toggle `cw` behaviour neovim#6235
    22337b1 CTRL-R omits trailing ^M when pasting to cmdline neovim#6137
    0e44916 :edit allows unescaped spaces in filename neovim#6119
    abdbfd2 eval: Add id() function and make printf("%p") useful neovim#6095
    bdfa147 findfile(), :find, gf work in :terminal. neovim#6009
    2f38ed1 providers: Disable if `g:loaded_*` exists.
    b5560a6 setpos() can set lowercase marks in other buffers neovim#5753
    7c513d6 Throttle :! output, pulse "..." message. neovim#5396
    d2e8c76 v:exiting neovim#5651

    :terminal improvements neovim#6185 neovim#6142
      - cursor keeps position after leaving insert-mode.
      - 4ceec30 Follows output only if cursor is at end of buffer.
      - e7bbd35 new option: 'scrollback'
      - fedb844 quasi-support for undo and 'modifiable'
      - b45ddf7 disables 'list' by default
      - disables 'relativenumber' by default

    :help now contains full API documentation at `:help api`.

    man.vim saw numerous improvements.

    Windows support:
      - Windows is no longer "experimental", it is fully supported.
      - Windows package includes a GUI, curl.exe and other utilities.

    "Vim 8" features: partials, lambdas.

SECURITY FIXES:
    CVE-2017-5953 CVE-2017-6349 CVE-2017-6350 neovim#6485

CHANGES:
    NVIM_TUI_ENABLE_CURSOR_SHAPE was removed. Use 'guicursor' instead.
        See https://github.com/neovim/neovim/wiki/Following-HEAD#20170402

    81525dc 'mouse=a' is no longer the default. (This will probably
                 change again after it is improved.) neovim#6022

    0c1f783 defaults: 'showcmd', 'belloff', 'ruler' neovim#6087
    eb0e94f api: {get,set}_option update local options as appropriate neovim#6405
    bdcb2a3 "Reading from stdin..." message was removed. neovim#6298

FIXES:
    12fc1de ops: fix i<c-r> with multi-byte text neovim#6524
    dd391bf Windows: system() and friends neovim#6497
    13352c0 Windows: os_get_hostname() neovim#6413
    16babc6 tui: Less-noisy mouse seqs neovim#6411
    3a9dd13 (vim bug) folding edge-cases  neovim#6207
    f6946c6 job-control: set CLOEXEC on pty processes. neovim#5986
    d1afd43 rplugin: Call s:LoadRemotePlugins() on startup.
    1215084 backtick-expansion works with `shell=fish` neovim#6224
    e32ec03 tui: Improved behavior after resize. neovim#6202
    86c2adc edit.c: CTRL-SPC: Insert previously-inserted text. neovim#6090
    c318d8e b:changedtick now follows VimL rules neovim#6112
    34e24cb terminal: Initialize colors in reverse order neovim#6160
    e889917 undo: Don't set b_u_curhead in ex_undojoin() neovim#5869
    d25649f undo: :earlier, g-: Set b_u_seq_cur correctly. (neovim#6016)
    043d8ba 'Visual-mode put from @. register' neovim#5782
    42c922b open_buffer(): Do `BufEnter` for directories.
    50d0d89 inccommand: Preview :sub commands only after delimiter neovim#5932
    1420e10 CheckHealth improvements neovim#5519
    c8d5e92 jobstart(): Return -1 if cmd is not executable. neovim#5671
@zeertzjq zeertzjq mentioned this pull request Dec 24, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@justinmk justinmk justinmk approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@ZyX-I @justinmk @blueyed