On 11/6/18 7:35 PM, Paul Eggert wrote: > Thanks, I installed that and am closing the bug report.
That was a real bug, i.e., not only a resource leak, wasn't it? If the calling user has -r+w permissions on the file, then sync previously exited with 1 without actually syncing: $ install -m 0200 /dev/null /tmp/file $ ls -log /tmp/file --w------- 1 0 Nov 7 00:06 /tmp/file $ strace -v /usr/bin/sync /tmp/file 2>&1 | tail -n6 openat(AT_FDCWD, "/tmp/file", O_RDONLY|O_NONBLOCK) = -1 EACCES (Permission denied) openat(AT_FDCWD, "/tmp/file", O_WRONLY|O_NONBLOCK) = 3 close(1) = 0 close(2) = 0 exit_group(1) = ? +++ exited with 1 +++ With the patch, fsync is called, and sync terminated with success: $ strace -v src/sync /tmp/file 2>&1 | tail openat(AT_FDCWD, "/tmp/file", O_RDONLY|O_NONBLOCK) = -1 EACCES (Permission denied) openat(AT_FDCWD, "/tmp/file", O_WRONLY|O_NONBLOCK) = 3 fcntl(3, F_GETFL) = 0x8801 (flags O_WRONLY|O_NONBLOCK|O_LARGEFILE) fcntl(3, F_SETFL, O_WRONLY|O_LARGEFILE) = 0 fsync(3) = 0 close(3) = 0 close(1) = 0 close(2) = 0 exit_group(0) = ? +++ exited with 0 +++ Should we add a NEWS entry and a test - see attached? Thanks & have a nice day, Berny
From 90679f499b71f2432a7faa439bc7a9155e7bee9c Mon Sep 17 00:00:00 2001 From: Bernhard Voelker <[email protected]> Date: Wed, 7 Nov 2018 00:26:01 +0100 Subject: [PATCH] sync: add NEWS and test for the fix in the previous commit * NEWS (Bug fixes): Mention the fix in commit 94d364f157f0. While at it, remove duplicate "Changes in behavior" heading. * tests/misc/sync.sh: Add a test with a write-only file for the fix. --- NEWS | 5 +++-- tests/misc/sync.sh | 7 ++++++- 2 files changed, 9 insertions(+), 3 deletions(-) diff --git a/NEWS b/NEWS index 088810fe8..8c1283549 100644 --- a/NEWS +++ b/NEWS @@ -11,6 +11,9 @@ GNU coreutils NEWS -*- outline -*- df no longer corrupts displayed multibyte characters on macOS. [bug introduced with coreutils-8.18] + sync no longer fails for write-only file arguments. + [bug introduced with argument support to sync in coreutils-8.24] + ** Changes in behavior echo now always processes backslash escapes when the POSIXLY_CORRECT @@ -22,8 +25,6 @@ GNU coreutils NEWS -*- outline -*- approach is still used in situations where hard links to directories are allowed (e.g., NetBSD when superuser). -** Changes in behavior - 'test -a FILE' is not supported anymore. Long ago, there were concerns about the high probability of humans confusing the -a primary with the -a binary operator, so POSIX changed this to 'test -e FILE'. Scripts using it were diff --git a/tests/misc/sync.sh b/tests/misc/sync.sh index f60d28c5a..3bb6e179e 100755 --- a/tests/misc/sync.sh +++ b/tests/misc/sync.sh @@ -19,7 +19,7 @@ . "${srcdir=.}/tests/init.sh"; path_prepend_ ./src print_ver_ sync -touch file +touch file || framework_failure_ # fdatasync+syncfs is nonsensical returns_ 1 sync --data --file-system || fail=1 @@ -30,6 +30,11 @@ returns_ 1 sync -d || fail=1 # Test syncing of file (fsync) (little side effects) sync file || fail=1 +# Test syncing of write-only file - which failed since adding argument +# support to sync in coreutils-8.24. +chmod 0200 file || framework_failure_ +sync file || fail=1 + # Ensure multiple args are processed and diagnosed returns_ 1 sync file nofile || fail=1 -- 2.19.1
