bug-coreutils
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Bernhard Voelker
Subject: bug#19681: [PATCH] sync: use syncfs(2) if any argument is specified
Date: Wed, 28 Jan 2015 09:17:55 +0100
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.4.0

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.

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?


> 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?

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)

Please note that syncfs() for the /tmp argument succeeded correctly.
For completeness, here is the correct case with a good error diagnostic
for an inaccessible file:

  $ src/sync --file /root/.bashrc
  src/sync: error opening ‘/root/.bashrc’: Permission denied

Thanks & have a nice day,
Berny





reply via email to

[Prev in Thread] Current Thread [Next in Thread]