Re: [PATCH v2 3/3] Consolidate posix_fadvise implementations

[Adhemerval Zanella <adhemerval dot zanella at linaro dot org>]

Thanks for the review.

On 05/10/2016 14:14, Siddhesh Poyarekar wrote:
> On Wednesday 28 September 2016 05:45 AM, Adhemerval Zanella wrote:
>> From: Adhemerval Zanella <adhemerval.zanella@linaro.org>
>>
>> This patch consolidates mostly of the Linux posix_fallocate{64} implementations
>> on sysdeps/unix/sysv/linux/posix_fallocate{64}.c.  It still keeps arch-specific
>> files for:
> 
> I guess you mean posix_fadvise :)

Oops...


>> +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)
>> +    { 
>> +      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)
> 
> Use the new FAIL definition in test-skeleton.c that you'll add from 1/3.

Ack.

> 
>> +
>> +/* 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).
> 
> "so the kernel is free to use this information in any way it deems fit,
> including ignoring it."

Ack.

> 
>> +
>> +   This test check for some invalid returned operation to check argument
>> +   passing and if implementation follows POSIX error definition.  */
>> +static int
>> +do_test_common (void)
>> +{
>> +  /* Add some data to file and ensure it is written down on disk.  */
> 
> "written to disk".

Ack.

> 
>> +  char buffer[2048] = { 0xcd };
>> +
>> +  if (write (temp_fd, buffer, 2048) != 2048)
>> +    FAIL ("write returned a value different than expected 2048");
>> +
>> +  if (fsync (temp_fd) != 0)
>> +    FAIL ("fsync failed");
>> +
>> +  /* Test passing an invalid fd.  */
>> +  if (posix_fadvise (-1, 0, 0, POSIX_FADV_NORMAL) != EBADF)
>> +    FAIL ("posix_fadvise with invalid fd did not return EBADF");
>> +
>> +  /* Test passing an invalid operation.  */
>> +  if (posix_fadvise (temp_fd, 0, 0, -1) != EINVAL)
>> +    FAIL ("posix_fadvise with invalid advise did not return EINVAL");
>> +
>> +  /* Test passing a FIFO fd.  */
>> +  if (posix_fadvise (fifofd, 0, 0, POSIX_FADV_NORMAL) != ESPIPE)
>> +    FAIL ("posix_advise with PIPE fd did not return ESPIPE");
>> +
>> +  /* Default fadvise on all file starting at initial position.  */
>> +  if (posix_fadvise (temp_fd, 0, 0, POSIX_FADV_NORMAL) != 0)
>> +    FAIL ("default posix_fadvise failed");
>> +
>> +  if (posix_fadvise (temp_fd, 0, 4096, POSIX_FADV_NORMAL) != 0)
>> +    FAIL ("posix_fadvise failed (offset = 0, len = 4096) failed");
>> +
>> +  if (posix_fadvise (temp_fd, 4096, 0, POSIX_FADV_NORMAL) != 0)
>> +    FAIL ("posix_fadvise failed (offset = 4096, len = 0) failed");
>> +
>> +  return 0;
>> +}
>> diff --git a/posix/tst-posix_fadvise.c b/posix/tst-posix_fadvise.c
>> new file mode 100644
>> index 0000000..6ee0936
>> --- /dev/null
>> +++ b/posix/tst-posix_fadvise.c
>> @@ -0,0 +1,25 @@
>> +/* Basic posix_fadvise tests.
>> +   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 "tst-posix_fadvise-common.c"
>> +
>> +static int
>> +do_test (void)
>> +{
>> +  return do_test_common ();
>> +}
>> diff --git a/posix/tst-posix_fadvise64.c b/posix/tst-posix_fadvise64.c
>> new file mode 100644
>> index 0000000..91d1860
>> --- /dev/null
>> +++ b/posix/tst-posix_fadvise64.c
>> @@ -0,0 +1,44 @@
>> +/* Basic posix_fadvise64 tests.
>> +   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/>.  */
>> +
>> +#define _FILE_OFFSET_BITS 64
>> +#include "tst-posix_fadvise-common.c"
>> +
>> +static int
>> +do_test (void)
>> +{
>> +  int ret = do_test_common ();
>> +  if (ret == -1)
>> +    return -1;
> 
> do_test_common returns 1, not -1.

Ack.

> 
>> +
>> +  /* Test passing a negative length.  The compat fadvise64 might use
>> +     off64_t for size argument passing, so using -1 for len without
>> +     _FILE_OFFSET_BITS might not trigger the length issue.  */
>> +  if (posix_fadvise (temp_fd, 0, -1, POSIX_FADV_NORMAL) != EINVAL)
>> +    FAIL ("posix_fadvise with negative length did not return EINVAL");
>> +
>> +  /* Check with some offset values larger than 32-bits.  */
>> +  off_t offset = UINT32_MAX + 2048LL;
>> +  if (posix_fadvise (temp_fd, 0, offset, POSIX_FADV_NORMAL) != 0)
>> +    FAIL ("posix_fadvise failed (offset = 0, len = 4096) failed");
>> +
>> +  if (posix_fadvise (temp_fd, offset, 0, POSIX_FADV_NORMAL) != 0)
>> +    FAIL ("posix_fadvise failed (offset = 4096, len = 0) failed");
>> +
>> +  return 0;
>> +}
>> diff --git a/sysdeps/unix/sysv/linux/arm/kernel-features.h b/sysdeps/unix/sysv/linux/arm/kernel-features.h
>> index 6ca607e..628d27f 100644
>> --- a/sysdeps/unix/sysv/linux/arm/kernel-features.h
>> +++ b/sysdeps/unix/sysv/linux/arm/kernel-features.h
>> @@ -27,6 +27,13 @@
>>  # undef __ASSUME_SET_ROBUST_LIST
>>  #endif
>>  
>> +/* ARM fadvise64_64 reorganize the syscall arguments.  */
>> +#define __ASSUME_FADVISE64_64_6ARG	1
>> +
>>  /* Define this if your 32-bit syscall API requires 64-bit register
>>     pairs to start with an even-number register.  */
>>  #define __ASSUME_ALIGNED_REGISTER_PAIRS	1
>> +
>> +/* ARM only has a syscall for fadvise64{_64} and it defined with a
>> +   non-standard name.  */
> 
> "it is defined"

Ack.