Re: [PATCH] df: fail when the mount list cannot be read but is needed [was: new snapshot available: coreutils-8.17.70-1bacb4]

[Bernhard Voelker]

On 08/14/2012 10:51 AM, Jim Meyering wrote:
> Here is the patch that I expect to squash in.
> Most are nits, but there was an exploitable bit
> of (debugging?) code in the new test.
> 
> diff --git a/NEWS b/NEWS
> index 0f63ddf..46d0a41 100644
> --- a/NEWS
> +++ b/NEWS
> @@ -2,8 +2,8 @@ GNU coreutils NEWS                                    -*- 
> outline -*-
> 
>  * Noteworthy changes in release ?.? (????-??-??) [?]
> 
> -  df now fails in cases when the list of mounted file systems (/etc/mtab)
> -  cannot be read but the file system type information is needed to process
> +  df now fails when the list of mounted file systems (/etc/mtab) cannot
> +  be read, yet the file system type information is needed to process
>    certain options like -a, -l, -t and -x.
>    [This bug was present in "the beginning".]
> 
> diff --git a/doc/coreutils.texi b/doc/coreutils.texi
> index 2fe0dd3..744b41a 100644
> --- a/doc/coreutils.texi
> +++ b/doc/coreutils.texi
> @@ -10754,9 +10754,9 @@ df invocation
>  @var{dir}} to test whether @var{dir} is on a file system of type
>  @samp{ext3} or @samp{reiserfs}.
> 
> -As the list of file systems (@var{mtab}) is needed to determine the
> +Since the list of file systems (@var{mtab}) is needed to determine the
>  file system type, failure includes the cases when that list cannot
> -be read and either of the options @option{-a}, @option{-l}, @option{-t}
> +be read and one or more of the options @option{-a}, @option{-l}, @option{-t}
>  or @option{-x} is used together with a file name argument.

Thanks for the rewording both in NEWS and cu.texi.
Often it's still hard for me to find the right wording in English ...

> diff --git a/src/df.c b/src/df.c
> index a2e6866..835cc2e 100644
> --- a/src/df.c
> +++ b/src/df.c
> @@ -1104,12 +1104,12 @@ main (int argc, char **argv)
>           or when either of -a, -l, -t or -x is used with file name
>           arguments. Otherwise, merely give a warning and proceed.  */
>        int status = 0;
> -      if ( !(optind < argc)
> -          ||(optind < argc
> -             && (show_all_fs
> -                 || show_local_fs
> -                 || fs_select_list != NULL
> -                 || fs_exclude_list != NULL)))
> +      if ( ! (optind < argc)
> +           || (optind < argc
> +               && (show_all_fs
> +                   || show_local_fs
> +                   || fs_select_list != NULL
> +                   || fs_exclude_list != NULL)))
>          {
>            status = EXIT_FAILURE;
>          }

I wonder if there's something like "make diff-syntax-check" which enforces
CU's coding styles for changed code ... the normal syntax-check didn't
complain.

> diff --git a/tests/df/no-mtab-status b/tests/df/no-mtab-status
> index 8835345..6e07854 100755
> --- a/tests/df/no-mtab-status
> +++ b/tests/df/no-mtab-status
> @@ -27,7 +27,7 @@ cat > k.c <<'EOF' || framework_failure_
>  #include <stdio.h>
>  #include <errno.h>
> 
> -struct mntent *getmntent(FILE *fp)
> +struct mntent *getmntent (FILE *fp)
>  {
>    /* Prove that LD_PRELOAD works. */
>    static int done = 0;
> @@ -46,8 +46,6 @@ EOF
>  $CC -shared -fPIC -ldl -O2 k.c -o k.so \
>    || framework_failure_ 'failed to compile with -shared -fPIC'
> 
> -cp k.so /tmp
> -

Good catch. Yes, that was a debugging statement.
I had problems with this LD_PRELOAD-ed test, because I
wanted to intercept open("/etc/mtab", ...) in the first
place, but that never fired, although strace clearly showed
such a line.

Thank you. Now it looks good.

Have a ncie day,
Berny