bug#19681: [PATCH] sync: use syncfs(2) if any argument is specified

[Pádraig Brady]

On 28/01/15 08:17, Bernhard Voelker wrote:
> On 01/27/2015 03:58 PM, Pádraig Brady wrote:
>> From 12c6f0fd7f44133a2af8950c69b2bfa46ea5d3a4 Mon Sep 17 00:00:00 2001
>> From: Giuseppe Scrivano <address@hidden>
>> Date: Sun, 25 Jan 2015 01:33:45 +0100
>> Subject: [PATCH] sync: support syncing specified arguments
> 
>> --- a/doc/coreutils.texi
>> +++ b/doc/coreutils.texi
>> @@ -12043,18 +12043,40 @@ with @env{TZ}, libc, The GNU C Library Reference 
>> Manual}.
>>  @command{sync} writes any data buffered in memory out to disk.  This can
>>  include (but is not limited to) modified superblocks, modified inodes,
>>  and delayed reads and writes.  This must be implemented by the kernel;
>> -The @command{sync} program does nothing but exercise the @code{sync} system
>> -call.
>> +The @command{sync} program does nothing but exercise the @code{sync},
>> address@hidden, @code{fsync}, and @code{fdatasync} system calls.
> 
> I think sync's info page now deserves a "Synopsis" line ... as the command
> now takes more than just --help/--version.

Done.

> Maybe the first line of 'man sync'
> 
>   sync - flush file system buffers
> 
> and 'info sync'
> 
>   "synchronize memory and disk" (in the parent table), and
>   "sync data on disk with memory" (sync invocation)
> 
> should be harmonized, too?

Good point. I went with this summary everywhere:

  "Synchronize cached writes to persistent storage"

>> diff --git a/src/sync.c b/src/sync.c
>> index e9f4d7e..80d1403 100644
>> --- a/src/sync.c
>> +++ b/src/sync.c
> 
>> @@ -37,11 +61,20 @@ usage (int status)
>>      emit_try_help ();
>>    else
>>      {
>> -      printf (_("Usage: %s [OPTION]\n"), program_name);
>> +      printf (_("Usage: %s [OPTION] [FILE]...\n"), program_name);
>>        fputs (_("\
>>  Force changed blocks to disk, update the super block.\n\
>>  \n\
>> +If one or more file paths are specified, sync only them,\n\
>> +use --data and --file-system to change the default behavior\n\
>> +\n\
>>  "), stdout);
>> +
>> +      fputs (_("\
>> +  --file-system              sync the file systems that contain the files\n\
>> +  --data                     only sync data for files, no unneeded 
>> metadata\n\
>> +"), stdout);
>> +
> 
> '--d' should go before '--f'.
> 
> And shouldn't we also be more translator-friendly, and split the
> 2 options into 2 fputs calls?

Good point. also the spacing was off.
Also I added short options to to align with -f, --file-system in stat(1).
If this ever was to be standardised, or used in other systems
short options would be used, so we might as well give some precedence
here to ease compat.

> The rest of the patch including the test almost LGTM:
> when running against a non-accessible directory, then the correct error
> diagnostic ("permission denied") is eclipsed by a non-descriptive
> diagnostic:
> 
>   $ src/sync --file /tmp /root
>   src/sync: error opening ‘/root’: Is a directory
> 
> strace output of the above:
> 
>   open("/root", O_RDONLY|O_NONBLOCK)      = -1 EACCES (Permission denied)
>   open("/root", O_WRONLY|O_NONBLOCK)      = -1 EISDIR (Is a directory)

Good catch!
I've gone with always reporting the first errno.

thanks again for the excellent review.

Latest is attached.

Pádraig.

sync-args.patch
Description: Text Data