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

Previous Next

Full log

View this message in rfc822 format

From: Bernhard Voelker <mail <at> bernhard-voelker.de>
To: Pádraig Brady <P <at> draigBrady.com>,  Giuseppe Scrivano <gscrivan <at> redhat.com>
Cc: 19681 <at> debbugs.gnu.org
Subject: bug#19681: [PATCH] sync: use syncfs(2) if any argument is specified
Date: Wed, 28 Jan 2015 09:17:55 +0100

On 01/27/2015 03:58 PM, Pádraig Brady wrote:
> From 12c6f0fd7f44133a2af8950c69b2bfa46ea5d3a4 Mon Sep 17 00:00:00 2001
> From: Giuseppe Scrivano <gscrivano <at> gnu.org>
> 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},
> +@code{syncfs}, @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

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.