This is the mail archive of the
libc-alpha@sourceware.org
mailing list for the glibc project.
Re: [PATCH v2 3/3] Consolidate posix_fadvise implementations
- From: Adhemerval Zanella <adhemerval dot zanella at linaro dot org>
- To: Siddhesh Poyarekar <siddhesh at gotplt dot org>, libc-alpha at sourceware dot org
- Date: Thu, 6 Oct 2016 17:50:13 -0300
- Subject: Re: [PATCH v2 3/3] Consolidate posix_fadvise implementations
- Authentication-results: sourceware.org; auth=none
- References: <1475021701-22246-1-git-send-email-adhemerval.zanella@linaro.com> <1475021701-22246-3-git-send-email-adhemerval.zanella@linaro.com> <724b49ae-ab7a-d678-cac6-ba286bf8b069@gotplt.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.