Re: [PATCH roland/opendir-extra-stat] BZ#18921: Fix opendir inverted o_directory_works test.

["Carlos O'Donell" <carlos at redhat dot com>]

On 09/03/2015 07:14 PM, Roland McGrath wrote:
> This fix is trivial (and I'll put it on 2.22 as well when I commit).
> I'm not committing immediately mostly because I have a vague hope that
> someone will suggest a way of writing a test.
> 
> The failure mode is not user-visible in the usual sense, as the function
> still behaves correctly.  There is a performance regression, but it is
> probably very hard to measure the relevant things.  The bug was actually
> caught by a test in the coreutils suite that uses strace to count the stat
> syscalls made by ls invocations in particular circumstances.
> 
> Can anybody think of a way that a regression test for this could be written
> that fits into the way we do things in libc?

I think it's not possible given the PLT avoidance internal symbol usage,
otherwise you could have preloaded a DSO and counted stat calls with
RTLD_NEXT as your fallback.

This is a job for the new glibc-test project :-)
 
> Thanks,
> Roland
> 
> 
> 2015-09-03  Roland McGrath  <roland@hack.frob.com>
> 
> 	[BZ #18921]
> 	* sysdeps/posix/opendir.c (need_isdir_precheck) [O_DIRECTORY]:
> 	Fix inverted sense of test of 'o_directory_works' value.
> 	Reported by Pïdraig Brady <P@draigBrady.com>, diagnosed by
> 	Bernhard Voelker <mail@bernhard-voelker.de>.
> 
> diff --git a/sysdeps/posix/opendir.c b/sysdeps/posix/opendir.c
> index 6509f5c..9edf056 100644
> --- a/sysdeps/posix/opendir.c
> +++ b/sysdeps/posix/opendir.c
> @@ -105,7 +105,7 @@ need_isdir_precheck (void)
>      tryopen_o_directory ();
>  
>    /* We can skip the expensive `stat' call if O_DIRECTORY works.  */
> -  return o_directory_works > 0;
> +  return o_directory_works < 0;

I would suggest rewriting the comment and check to be more explicit
about exactly what is being done, and to have both be written from
the same context.

e.g.

  /* Return true if O_DIRECTORY is not supported; the caller must use stat.  */
  return o_directory_works == -1;


>  #endif
>    return true;
>  }
> 

Cheers,
Carlos.