diff mbox

[v2] Fix LO_HI_LONG definition

Message ID 577E6114.4090004@linaro.org
State New
Headers show

Commit Message

Adhemerval Zanella Netto July 7, 2016, 2:03 p.m. UTC 
On 07/07/2016 03:56, Florian Weimer wrote:
> On 07/06/2016 09:52 PM, Adhemerval Zanella wrote:
>> +  /* Create a sparse file larger than 4GB to check if offset is handled
>> +     correctly in p{write,read}v64. */
>> +  off_t base_offset = UINT32_MAX;
>> +  off_t fsize = base_offset + 2048;
>> +  if (ftruncate (temp_fd, fsize) != 0)
>> +    {
>> +      printf ("error: ftruncate (%jd) failed: %m", (intmax_t) fsize);
>> +      return 1;
>> +    }
> 
> I don't think this is is necessary.  You should be able to write beyond the current EOF and get a sparse file directly.
> 
> I'm not sure if the current tests catch calling convention errors because the read and write offsets could be mangled in the same way. Maybe you can add a test which performs a pwrite with a large offset and check that the file size reported by fstat is as expected?

Indeed, I have changed the testcase to check if returned file is indeed what
the test expect.

---

Comments

Florian Weimer July 8, 2016, 4:41 p.m. UTC | #1 
On 07/07/2016 04:03 PM, Adhemerval Zanella wrote:
>
>
> On 07/07/2016 03:56, Florian Weimer wrote:
>> On 07/06/2016 09:52 PM, Adhemerval Zanella wrote:
>>> +  /* Create a sparse file larger than 4GB to check if offset is handled
>>> +     correctly in p{write,read}v64. */
>>> +  off_t base_offset = UINT32_MAX;
>>> +  off_t fsize = base_offset + 2048;
>>> +  if (ftruncate (temp_fd, fsize) != 0)
>>> +    {
>>> +      printf ("error: ftruncate (%jd) failed: %m", (intmax_t) fsize);
>>> +      return 1;
>>> +    }
>>
>> I don't think this is is necessary.  You should be able to write beyond the current EOF and get a sparse file directly.
>>
>> I'm not sure if the current tests catch calling convention errors because the read and write offsets could be mangled in the same way. Maybe you can add a test which performs a pwrite with a large offset and check that the file size reported by fstat is as expected?
>
> Indeed, I have changed the testcase to check if returned file is indeed what
> the test expect.

Test case looks okay to me now.

I'm not sure about the correctness of the change, sorry.

Thanks,
Florian
Adhemerval Zanella Netto July 8, 2016, 5:08 p.m. UTC | #2 
On 08/07/2016 13:41, Florian Weimer wrote:
> On 07/07/2016 04:03 PM, Adhemerval Zanella wrote:
>>
>>
>> On 07/07/2016 03:56, Florian Weimer wrote:
>>> On 07/06/2016 09:52 PM, Adhemerval Zanella wrote:
>>>> +  /* Create a sparse file larger than 4GB to check if offset is handled
>>>> +     correctly in p{write,read}v64. */
>>>> +  off_t base_offset = UINT32_MAX;
>>>> +  off_t fsize = base_offset + 2048;
>>>> +  if (ftruncate (temp_fd, fsize) != 0)
>>>> +    {
>>>> +      printf ("error: ftruncate (%jd) failed: %m", (intmax_t) fsize);
>>>> +      return 1;
>>>> +    }
>>>
>>> I don't think this is is necessary.  You should be able to write beyond the current EOF and get a sparse file directly.
>>>
>>> I'm not sure if the current tests catch calling convention errors because the read and write offsets could be mangled in the same way. Maybe you can add a test which performs a pwrite with a large offset and check that the file size reported by fstat is as expected?
>>
>> Indeed, I have changed the testcase to check if returned file is indeed what
>> the test expect.
> 
> Test case looks okay to me now.
> 
> I'm not sure about the correctness of the change, sorry.

Thanks, I checked the patch on some different platforms (x86, powerpc64le,
s390, s390x, aarch64, and arm) and test is passing.

I also checked the generated code on the platform that triggered the issue
(MIPS64n32) and syscall wrapper now is doing what pre-consolidation 
implementation were doing.

I will push it today.
diff mbox

Patch

diff --git a/misc/tst-preadvwritev-common.c b/misc/tst-preadvwritev-common.c
new file mode 100644
index 0000000..9a66a5f
--- /dev/null
+++ b/misc/tst-preadvwritev-common.c
@@ -0,0 +1,112 @@ 
+/* Common definitions 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>
+
+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 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)
+
+static int
+do_test_with_offset (off_t offset)
+{
+  struct iovec iov[2];
+  ssize_t ret;
+
+  char buf1[32];
+  char buf2[64];
+
+  memset (buf1, 0xf0, sizeof buf1);
+  memset (buf2, 0x0f, sizeof buf2);
+
+  /* Write two buffer with 32 and 64 bytes respectively.  */
+  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, offset);
+  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 + offset);
+  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;
+
+  /* Now read two buffer with 32 and 64 bytes respectively.  */
+  ret = preadv (temp_fd, iov, 2, offset);
+  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 + offset);
+  if (ret == -1)
+    FAIL ("second preadv returned -1");
+  if (ret != (sizeof buf3 + sizeof buf4))
+    FAIL ("second preadv returned an unexpected value");
+
+  /* And compare the buffers read and written to check if there are equal.  */
+  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-preadvwritev.c b/misc/tst-preadvwritev.c
index 08deecc..50a80a7 100644
--- a/misc/tst-preadvwritev.c
+++ b/misc/tst-preadvwritev.c
@@ -16,99 +16,10 @@ 
    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)
+#include "tst-preadvwritev-common.c"
 
 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;
+  return do_test_with_offset (0);
 }
diff --git a/misc/tst-preadvwritev64.c b/misc/tst-preadvwritev64.c
index ff6e134..eafe5e9 100644
--- a/misc/tst-preadvwritev64.c
+++ b/misc/tst-preadvwritev64.c
@@ -16,7 +16,36 @@ 
    License along with the GNU C Library; if not, see
    <http://www.gnu.org/licenses/>.  */
 
-#define PREADV  preadv64
-#define PWRITEV pwritev64
+#define _FILE_OFFSET_BITS 64
+#include "tst-preadvwritev-common.c"
 
-#include "tst-preadvwritev.c"
+int
+do_test (void)
+{
+  int ret;
+
+  ret = do_test_with_offset (0);
+
+  /* Create a sparse file larger than 4GB to check if offset is handled
+     correctly in p{write,read}v64. */
+  off_t base_offset = UINT32_MAX + 2048LL;
+  ret += do_test_with_offset (base_offset);
+
+  struct stat st;
+  if (fstat (temp_fd, &st) == -1)
+    {
+      printf ("error: fstat on temporary file failed: %m");
+      return 1;
+    }
+
+  /* The total size should base_offset plus 2 * 96.  */
+  off_t expected_value = base_offset + (2 * (96LL));
+  if (st.st_size != expected_value)
+    {
+      printf ("error: file size different than expected (%jd != %jd)\n",
+	      (intmax_t) expected_value, (intmax_t) st.st_size);
+      return 1;
+    }
+
+  return ret;
+}
diff --git a/sysdeps/unix/sysv/linux/sysdep.h b/sysdeps/unix/sysv/linux/sysdep.h
index 8c9e62e..a469f57 100644
--- a/sysdeps/unix/sysv/linux/sysdep.h
+++ b/sysdeps/unix/sysv/linux/sysdep.h
@@ -49,10 +49,6 @@ 
 #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) \
-  (long) (val), \
-  (long) (((uint64_t) (val)) >> 32)
-#endif
+#define LO_HI_LONG(val) \
+ (long) (val), \
+ (long) (((uint64_t) (val)) >> 32)