Re: [PATCH 4/4] Consolidate posix_fadvise implementations

[Adhemerval Zanella <adhemerval.zanella@linaro.org>]

Thanks for the reviews, comments below.

On 22/08/2016 12:08, Yury Norov wrote:
>> diff --git a/posix/tst-posix_fadvise-common.c b/posix/tst-posix_fadvise-common.c
>> new file mode 100644
>> index 0000000..6670835
>> --- /dev/null
>> +++ b/posix/tst-posix_fadvise-common.c
>> @@ -0,0 +1,113 @@
>> +/* Common posix_fadvise tests definitions.
>> +   Copyright (C) 2016 Free Software Foundation, Inc.
>> +   This file is part of the GNU C Library.
>> +
>> +   The GNU C Library is free software; you can redistribute it and/or
>> +   modify it under the terms of the GNU Lesser General Public
>> +   License as published by the Free Software Foundation; either
>> +   version 2.1 of the License, or (at your option) any later version.
>> +
>> +   The GNU C Library is distributed in the hope that it will be useful,
>> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
>> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
>> +   Lesser General Public License for more details.
>> +
>> +   You should have received a copy of the GNU Lesser General Public
>> +   License along with the GNU C Library; if not, see
>> +   <http://www.gnu.org/licenses/>.  */
>> +
>> +#include <fcntl.h>
>> +#include <sys/types.h>
>> +#include <sys/stat.h>
>> +#include <unistd.h>
>> +
>> +static void do_prepare (void);
>> +#define PREPARE(argc, argv)     do_prepare ()
>> +static int do_test (void);
>> +#define TEST_FUNCTION           do_test ()
>> +
>> +#include <test-skeleton.c>
>> +
>> +static char *temp_filename;
>> +static int temp_fd;
>> +static char fifoname[] = "/tmp/tst-posix_fadvise-fifo-XXXXXX";
>> +static int fifofd;
>> +
>> +static void
>> +do_prepare (void)
>> +{
>> +  temp_fd = create_temp_file ("tst-posix_fadvise.", &temp_filename);
>> +  if (temp_fd == -1)
>> +    {
>> +      printf ("cannot create temporary file: %m\n");
>> +      exit (1);
>> +    }
>> +
>> +  if (mktemp (fifoname) == NULL)
>> +    { 
> 
> Trailing whitespace

Ack, I will fix  it.

> 
>> +      printf ("%s: cannot generate temp file name: %m\n", __func__);
>> +      exit (1);
>> +    }
>> +  add_temp_file (fifoname);
>> +
>> +  if (mkfifo (fifoname, S_IWUSR | S_IRUSR) != 0)
>> +    {
>> +      printf ("%s: cannot create fifo: %m\n", __func__);
>> +      exit (1);
>> +    }
>> +
>> +  fifofd = open (fifoname, O_RDONLY | O_NONBLOCK);
>> +  if (fifofd == -1)
>> +    {
>> +      printf ("%s: cannot open fifo: %m\n", __func__);
>> +      exit (1);
>> +    }
>> +}
>> +
>> +#define FAIL(str) \
>> +  do { printf ("error: %s (line %d)\n", str, __LINE__); return 1; } while (0)
>> +
> 
> You don't fit into a single line anyway. Why don't you make it nicer?

Right, I think the snippet below should be more readable.  I will change it.

#define FAIL(str) \
  do { \
    printf ("error: %s (line %d)\n", str, __LINE__); \
    return 1; \
  } while (0)

>> +/* Effectivelly testing posix_fadvise is hard because side effects are not
>> +   observed without checking either performance or any kernel specific
>> +   supplied information.  Also, the syscall is meant to be an advisory,
>> +   so kernel is free to use these information in which way it seems as
>> +   fit (even ignoring it).
>> +   
> 
> Trailing whitespaces
> 

Ack.

>> diff --git a/sysdeps/unix/sysv/linux/posix_fadvise.c b/sysdeps/unix/sysv/linux/posix_fadvise.c
>> index 093d707..869a642 100644
>> --- a/sysdeps/unix/sysv/linux/posix_fadvise.c
>> +++ b/sysdeps/unix/sysv/linux/posix_fadvise.c
>> @@ -22,27 +22,46 @@
>>  /* Advice the system about the expected behaviour of the application with
>>     respect to the file associated with FD.  */
>>  
>> +#ifndef __OFF_T_MATCHES_OFF64_T
>> +
>> +/* Both arm and powerpc implements fadvise64_64 with last 'advise' argument
>> +   just after 'fd' to avoid the requirement of implementing 7-arg syscalls.
>> +   ARM also defines __NR_fadvise64_64 as __NR_arm_fadvise64_64.
>> +
>> +   tile requires __ASSUME_ALIGNED_REGISTER_PAIRS but implements the 32-bit
>> +   fadvise64_64 without the padding 0 after fd.
>> +
>> +   s390 implements fadvice64_64 using a specific struct with arguments
>> +   packed inside.  This is the only implementation handled in arch-specific
>> +   code.  */
> 
> Are you sure it will be the only implementation forever, and comment
> will be valid too?

The comment is meant to only describe supported architectures at the
time the patch was created.  Any other port that either deviates
from current approach or uses a already describe one should update
this description.

>> diff --git a/sysdeps/unix/sysv/linux/posix_fadvise64.c b/sysdeps/unix/sysv/linux/posix_fadvise64.c
>> index 6d10558..15e08b7 100644
>> --- a/sysdeps/unix/sysv/linux/posix_fadvise64.c
>> +++ b/sysdeps/unix/sysv/linux/posix_fadvise64.c
>> @@ -17,32 +17,54 @@
>>  
>>  #include <errno.h>
>>  #include <fcntl.h>
>> -#include <sysdep.h>
>> +#include <shlib-compat.h>
>>  
>>  int __posix_fadvise64_l64 (int fd, off64_t offset, off64_t len, int advise);
>> -int __posix_fadvise64_l32 (int fd, off64_t offset, size_t len, int advise);
>> +
>> +/* Both arm and powerpc implements fadvise64_64 with last 'advise' argument
>> +   just after 'fd' to avoid the requirement of implementing 7-arg syscalls.
>> +   ARM also defines __NR_fadvise64_64 as __NR_arm_fadvise64_64.
>> +
>> +   tile requires __ASSUME_ALIGNED_REGISTER_PAIRS but implements the 32-bit
>> +   fadvise64_64 without the padding 0 after fd.
>> +
>> +   s390 implements fadvice64_64 using a specific struct with arguments
>> +   packed inside.  This is the only implementation handled in arch-specific
>> +   code.  */
>> +
>> +#ifdef __ASSUME_FADVISE64_64_NO_ALIGN
>> +# undef __ALIGNMENT_ARG
>> +# define __ALIGNMENT_ARG
>> +#endif
>> +
>> +#ifndef __NR_fadvise64_64
>> +# define __NR_fadvise64_64 __NR_fadvise64
>> +#endif
>>  
>>  /* Advice the system about the expected behaviour of the application with
>>     respect to the file associated with FD.  */
>> -
> 
> Is it necessary to remove this line. You don't do so elsewhere..

Ack, I will add it.