diff mbox

[v2] Fix p{readv,writev}{64} consolidation implementation

Message ID 1466025604-17044-1-git-send-email-adhemerval.zanella@linaro.org
State New
Headers show

Commit Message

Adhemerval Zanella Netto June 15, 2016, 9:20 p.m. UTC
Changes from previous version:

 - Remove SYSCALL_LL{64} usage and replace by another macro based on
   previous default implementation (LO_HI_LONG)
 - Fix some issue with testcase

--

This patch fixes the p{readv,writev}{64} consolidation implementation
from commits 4e77815 and af5fdf5.  Different from pread/pwrite
implementation, preadv/pwritev implementation does not require
__ALIGNMENT_ARG because kernel syscall prototypes define
the high and low part of the off_t, if it is the case, directly
(different from pread/pwrite where the architecture ABI for passing
64-bit values must be in consideration for passsing the arguments).

It also adds some basic tests for preadv/pwritev.

Tested on x86_64, i686, and armhf.

	* misc/Makefile (tests): Add tst-preadvwritev and tst-preadvwritev64.
	* misc/tst-preadvwritev.c: New file.
	* misc/tst-preadvwritev64.c: Likewise.
	* sysdeps/unix/sysv/linux/preadv.c (preadv): Remove SYSCALL_LL{64}
	usage.
	* sysdeps/unix/sysv/linux/preadv64.c (preadv64): Likewise.
	* sysdeps/unix/sysv/linux/pwritev.c (pwritev): Likewise.
	* sysdeps/unix/sysv/linux/pwritev64.c (pwritev64): Likewise.
	* sysdeps/unix/sysv/linux/sysdep.h (LO_HI_LONG): New macro.
---
 ChangeLog                           |  12 ++++
 misc/Makefile                       |   3 +-
 misc/tst-preadvwritev.c             | 114 ++++++++++++++++++++++++++++++++++++
 misc/tst-preadvwritev64.c           |  22 +++++++
 sysdeps/unix/sysv/linux/preadv.c    |   5 +-
 sysdeps/unix/sysv/linux/preadv64.c  |   5 +-
 sysdeps/unix/sysv/linux/pwritev.c   |   5 +-
 sysdeps/unix/sysv/linux/pwritev64.c |   7 +--
 sysdeps/unix/sysv/linux/sysdep.h    |   9 +++
 9 files changed, 168 insertions(+), 14 deletions(-)
 create mode 100644 misc/tst-preadvwritev.c
 create mode 100644 misc/tst-preadvwritev64.c

Comments

Mike Frysinger June 15, 2016, 10:38 p.m. UTC | #1
On 15 Jun 2016 18:20, Adhemerval Zanella wrote:
> --- a/sysdeps/unix/sysv/linux/sysdep.h
> +++ b/sysdeps/unix/sysv/linux/sysdep.h
> @@ -47,3 +47,12 @@
>  #define SYSCALL_LL64(val) \
>    __LONG_LONG_PAIR ((long) ((val) >> 32), (long) ((val) & 0xffffffff))
>  #endif
> +
> +/* Provide a macro to pass the off{64}_t argument on p{readv,writev}{64}.  */
> +#if __WORDSIZE == 64 || defined __ASSUME_WORDSIZE64_ILP32
> +# define LO_HI_LONG(val) (val)
> +#else
> +# define LO_HI_LONG(val) \
> +  (off_t) (val),                                                          \
> +  (off_t) ((((uint64_t) (val)) >> (sizeof (long) * 4)) >> (sizeof (long) * 4))
> +#endif

does this really need to be this complicated ?  we're already making an
assumption about the size for 64-bit & 64-bit/ilp32 ports, and you're
using an uint64_t cast to start with, and SYSCALL_LL64 assumes 32.
  (long) (val),
  (long) (((uint64_t) (val)) >> 32)
-mike
Adhemerval Zanella Netto June 15, 2016, 11:45 p.m. UTC | #2
On 15/06/2016 19:38, Mike Frysinger wrote:
> On 15 Jun 2016 18:20, Adhemerval Zanella wrote:
>> --- a/sysdeps/unix/sysv/linux/sysdep.h
>> +++ b/sysdeps/unix/sysv/linux/sysdep.h
>> @@ -47,3 +47,12 @@
>>  #define SYSCALL_LL64(val) \
>>    __LONG_LONG_PAIR ((long) ((val) >> 32), (long) ((val) & 0xffffffff))
>>  #endif
>> +
>> +/* Provide a macro to pass the off{64}_t argument on p{readv,writev}{64}.  */
>> +#if __WORDSIZE == 64 || defined __ASSUME_WORDSIZE64_ILP32
>> +# define LO_HI_LONG(val) (val)
>> +#else
>> +# define LO_HI_LONG(val) \
>> +  (off_t) (val),                                                          \
>> +  (off_t) ((((uint64_t) (val)) >> (sizeof (long) * 4)) >> (sizeof (long) * 4))
>> +#endif
> 
> does this really need to be this complicated ?  we're already making an
> assumption about the size for 64-bit & 64-bit/ilp32 ports, and you're
> using an uint64_t cast to start with, and SYSCALL_LL64 assumes 32.
>   (long) (val),
>   (long) (((uint64_t) (val)) >> 32)
> -mike
> 

I think your suggestion should be ok, this snippet I used came from
the previous version in fact.  I will change to just

/* Provide a macro to pass the off{64}_t argument on p{readv,writev}{64}.  */
#if __WORDSIZE == 64 || defined __ASSUME_WORDSIZE64_ILP32
# define LO_HI_LONG(val) (val)
#else
# define LO_HI_LONG(val) \
   (long) (val), \
   (long) (((uint64_t) (val)) >> 32)
#endif
diff mbox

Patch

diff --git a/ChangeLog b/ChangeLog
index a4a1cea..e555444 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,15 @@ 
+2016-06-14  Adhemerval Zanella  <adhemerval.zanella@linaro.org>
+
+	* misc/Makefile (tests): Add tst-preadvwritev and tst-preadvwritev64.
+	* misc/tst-preadvwritev.c: New file.
+	* misc/tst-preadvwritev64.c: Likewise.
+	* sysdeps/unix/sysv/linux/preadv.c (preadv): Remove SYSCALL_LL{64}
+	usage.
+	* sysdeps/unix/sysv/linux/preadv64.c (preadv64): Likewise.
+	* sysdeps/unix/sysv/linux/pwritev.c (pwritev): Likewise.
+	* sysdeps/unix/sysv/linux/pwritev64.c (pwritev64): Likewise.
+	* sysdeps/unix/sysv/linux/sysdep.h (LO_HI_LONG): New macro.
+
 2016-06-14  Joseph Myers  <joseph@codesourcery.com>
 
 	[BZ #20255]
diff --git a/misc/Makefile b/misc/Makefile
index 6498adc..56e20ce 100644
--- a/misc/Makefile
+++ b/misc/Makefile
@@ -77,7 +77,8 @@  gpl2lgpl := error.c error.h
 
 tests := tst-dirname tst-tsearch tst-fdset tst-efgcvt tst-mntent tst-hsearch \
 	 tst-error1 tst-pselect tst-insremque tst-mntent2 bug-hsearch1 \
-	 tst-mntent-blank-corrupt tst-mntent-blank-passno bug18240
+	 tst-mntent-blank-corrupt tst-mntent-blank-passno bug18240 \
+	 tst-preadvwritev tst-preadvwritev64
 ifeq ($(run-built-tests),yes)
 tests-special += $(objpfx)tst-error1-mem.out
 endif
diff --git a/misc/tst-preadvwritev.c b/misc/tst-preadvwritev.c
new file mode 100644
index 0000000..6836574
--- /dev/null
+++ b/misc/tst-preadvwritev.c
@@ -0,0 +1,114 @@ 
+/* Tests for preadv and pwritev.
+   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 <sys/uio.h>
+
+/* Allow testing of the 64-bit versions as well.  */
+#ifndef PREADV
+# define PREADV  preadv
+# define PWRITEV pwritev
+#endif
+
+static void do_prepare (void);
+static int do_test (void);
+#define PREPARE(argc, argv)     do_prepare ()
+#define TEST_FUNCTION           do_test ()
+#include "../test-skeleton.c"
+
+static char *temp_filename;
+static int temp_fd;
+
+void
+do_prepare (void)
+{
+  temp_fd = create_temp_file ("tst-PREADVwritev.", &temp_filename);
+  if (temp_fd == -1)
+    {
+      printf ("cannot create temporary file: %m\n");
+      exit (1);
+    }
+}
+
+#define FAIL(str) \
+  do { printf ("error: %s (line %d)\n", str, __LINE__); return 1; } while (0)
+
+int
+do_test (void)
+{
+  struct iovec iov[2];
+  ssize_t ret;
+
+  char buf1[32];
+  char buf2[64];
+
+  memset (buf1, 0xf0, sizeof buf1);
+  memset (buf2, 0x0f, sizeof buf2);
+
+  memset (iov, 0, sizeof iov);
+  iov[0].iov_base = buf1;
+  iov[0].iov_len = sizeof buf1;
+  iov[1].iov_base = buf2;
+  iov[1].iov_len = sizeof buf2;
+
+  ret = PWRITEV (temp_fd, iov, 2, 0);
+  if (ret == -1)
+    FAIL ("first PWRITEV returned -1");
+  if (ret != (sizeof buf1 + sizeof buf2))
+    FAIL ("first PWRITEV returned an unexpected value");
+
+  ret = PWRITEV (temp_fd, iov, 2, sizeof buf1 + sizeof buf2);
+  if (ret == -1)
+    FAIL ("second PWRITEV returned -1");
+  if (ret != (sizeof buf1 + sizeof buf2))
+    FAIL ("second PWRITEV returned an unexpected value");
+  
+  char buf3[32];
+  char buf4[64];
+
+  memset (buf3, 0x0f, sizeof buf3);
+  memset (buf4, 0xf0, sizeof buf4);
+
+  iov[0].iov_base = buf3;
+  iov[0].iov_len = sizeof buf3;
+  iov[1].iov_base = buf4;
+  iov[1].iov_len = sizeof buf4;
+
+  ret = PREADV (temp_fd, iov, 2, 0);
+  if (ret == -1)
+    FAIL ("first PREADV returned -1");
+  if (ret != (sizeof buf3 + sizeof buf4))
+    FAIL ("first PREADV returned an unexpected value");
+
+  if (memcmp (buf1, buf3, sizeof buf1) != 0)
+    FAIL ("first buffer from first PREADV different than expected");
+  if (memcmp (buf2, buf4, sizeof buf2) != 0)
+    FAIL ("second buffer from first PREADV different than expected");
+
+  ret = PREADV (temp_fd, iov, 2, sizeof buf3 + sizeof buf4);
+  if (ret == -1)
+    FAIL ("second PREADV returned -1");
+  if (ret != (sizeof buf3 + sizeof buf4))
+    FAIL ("second PREADV returned an unexpected value");
+
+  if (memcmp (buf1, buf3, sizeof buf1) != 0)
+    FAIL ("first buffer from second PREADV different than expected");
+  if (memcmp (buf2, buf4, sizeof buf2) != 0)
+    FAIL ("second buffer from second PREADV different than expected");
+
+  return 0;
+}
diff --git a/misc/tst-preadvwritev64.c b/misc/tst-preadvwritev64.c
new file mode 100644
index 0000000..ff6e134
--- /dev/null
+++ b/misc/tst-preadvwritev64.c
@@ -0,0 +1,22 @@ 
+/* Tests for pread64 and pwrite64.
+   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 PREADV  preadv64
+#define PWRITEV pwritev64
+
+#include "tst-preadvwritev.c"
diff --git a/sysdeps/unix/sysv/linux/preadv.c b/sysdeps/unix/sysv/linux/preadv.c
index f6958c3..be206e2 100644
--- a/sysdeps/unix/sysv/linux/preadv.c
+++ b/sysdeps/unix/sysv/linux/preadv.c
@@ -29,8 +29,7 @@ 
 ssize_t
 preadv (int fd, const struct iovec *vector, int count, off_t offset)
 {
-  return SYSCALL_CANCEL (preadv, fd, vector, count,
-			 __ALIGNMENT_ARG SYSCALL_LL (offset));
+  return SYSCALL_CANCEL (preadv, fd, vector, count, LO_HI_LONG (offset));
 }
 # else
 static ssize_t __atomic_preadv_replacement (int, const struct iovec *,
@@ -40,7 +39,7 @@  preadv (int fd, const struct iovec *vector, int count, off_t offset)
 {
 #  ifdef __NR_preadv
   ssize_t result = SYSCALL_CANCEL (preadv, fd, vector, count,
-				   __ALIGNMENT_ARG SYSCALL_LL (offset));
+				   LO_HI_LONG (offset));
   if (result >= 0 || errno != ENOSYS)
     return result;
 #  endif
diff --git a/sysdeps/unix/sysv/linux/preadv64.c b/sysdeps/unix/sysv/linux/preadv64.c
index 18f5550..64164bb 100644
--- a/sysdeps/unix/sysv/linux/preadv64.c
+++ b/sysdeps/unix/sysv/linux/preadv64.c
@@ -27,8 +27,7 @@ 
 ssize_t
 preadv64 (int fd, const struct iovec *vector, int count, off64_t offset)
 {
-  return SYSCALL_CANCEL (preadv64, fd, vector, count,
-			 __ALIGNMENT_ARG SYSCALL_LL64 (offset));
+  return SYSCALL_CANCEL (preadv64, fd, vector, count, LO_HI_LONG (offset));
 }
 #else
 static ssize_t __atomic_preadv64_replacement (int, const struct iovec *,
@@ -38,7 +37,7 @@  preadv64 (int fd, const struct iovec *vector, int count, off64_t offset)
 {
 #ifdef __NR_preadv64
   ssize_t result = SYSCALL_CANCEL (preadv64, fd, vector, count,
-				   __ALIGNMENT_ARG SYSCALL_LL64 (offset));
+				   LO_HI_LONG (offset));
   if (result >= 0 || errno != ENOSYS)
     return result;
 #endif
diff --git a/sysdeps/unix/sysv/linux/pwritev.c b/sysdeps/unix/sysv/linux/pwritev.c
index b11606c..19b4a10 100644
--- a/sysdeps/unix/sysv/linux/pwritev.c
+++ b/sysdeps/unix/sysv/linux/pwritev.c
@@ -29,8 +29,7 @@ 
 ssize_t
 pwritev (int fd, const struct iovec *vector, int count, off_t offset)
 {
-  return SYSCALL_CANCEL (pwritev, fd, vector, count,
-			 __ALIGNMENT_ARG SYSCALL_LL (offset));
+  return SYSCALL_CANCEL (pwritev, fd, vector, count, LO_HI_LONG (offset));
 }
 # else
 static ssize_t __atomic_pwritev_replacement (int, const struct iovec *,
@@ -40,7 +39,7 @@  pwritev (int fd, const struct iovec *vector, int count, off_t offset)
 {
 #  ifdef __NR_pwritev
   ssize_t result = SYSCALL_CANCEL (pwritev, fd, vector, count,
-				   __ALIGNMENT_ARG SYSCALL_LL (offset));
+				   LO_HI_LONG (offset));
   if (result >= 0 || errno != ENOSYS)
     return result;
 #  endif
diff --git a/sysdeps/unix/sysv/linux/pwritev64.c b/sysdeps/unix/sysv/linux/pwritev64.c
index bed79b7..c0cfe4b 100644
--- a/sysdeps/unix/sysv/linux/pwritev64.c
+++ b/sysdeps/unix/sysv/linux/pwritev64.c
@@ -27,8 +27,7 @@ 
 ssize_t
 pwritev64 (int fd, const struct iovec *vector, int count, off64_t offset)
 {
-  return SYSCALL_CANCEL (pwritev64, fd, vector, count,
-			 __ALIGNMENT_ARG SYSCALL_LL64 (offset));
+  return SYSCALL_CANCEL (pwritev64, fd, vector, count, LO_HI_LONG (offset));
 }
 #else
 static ssize_t __atomic_pwritev64_replacement (int, const struct iovec *,
@@ -36,9 +35,9 @@  static ssize_t __atomic_pwritev64_replacement (int, const struct iovec *,
 ssize_t
 pwritev64 (int fd, const struct iovec *vector, int count, off64_t offset)
 {
-#ifdef __NR_pwrite64v
+#ifdef __NR_pwritev64
   ssize_t result = SYSCALL_CANCEL (pwritev64, fd, vector, count,
-				   __ALIGNMENT_ARG SYSCALL_LL64 (offset));
+				   LO_HI_LONG (offset));
   if (result >= 0 || errno != ENOSYS)
     return result;
 #endif
diff --git a/sysdeps/unix/sysv/linux/sysdep.h b/sysdeps/unix/sysv/linux/sysdep.h
index f2d7e05..24deb78 100644
--- a/sysdeps/unix/sysv/linux/sysdep.h
+++ b/sysdeps/unix/sysv/linux/sysdep.h
@@ -47,3 +47,12 @@ 
 #define SYSCALL_LL64(val) \
   __LONG_LONG_PAIR ((long) ((val) >> 32), (long) ((val) & 0xffffffff))
 #endif
+
+/* Provide a macro to pass the off{64}_t argument on p{readv,writev}{64}.  */
+#if __WORDSIZE == 64 || defined __ASSUME_WORDSIZE64_ILP32
+# define LO_HI_LONG(val) (val)
+#else
+# define LO_HI_LONG(val) \
+  (off_t) (val),                                                          \
+  (off_t) ((((uint64_t) (val)) >> (sizeof (long) * 4)) >> (sizeof (long) * 4))
+#endif