[LTP] [PATCH 0/2] syscalls/sendfile: Convert sendfile{08, 09} to the new API

[LTP] [PATCH 0/2] syscalls/sendfile: Convert sendfile{08, 09} to the new API

[Xie Ziyao]

Xie Ziyao (2):
  syscalls/sendfile: Convert sendfile08 to the new API
  syscalls/sendfile: Convert sendfile09 to the new API

 .../kernel/syscalls/sendfile/sendfile08.c     | 138 ++++-------
 .../kernel/syscalls/sendfile/sendfile09.c     | 234 ++++++------------
 2 files changed, 118 insertions(+), 254 deletions(-)

--
2.17.1

[LTP] [PATCH 1/2] syscalls/sendfile: Convert sendfile08 to the new API

[Xie Ziyao]

Convert sendfile08 to the new API.

Signed-off-by: Xie Ziyao <xieziyao@huawei.com>
---
 .../kernel/syscalls/sendfile/sendfile08.c     | 138 ++++++------------
 1 file changed, 46 insertions(+), 92 deletions(-)

diff --git a/testcases/kernel/syscalls/sendfile/sendfile08.c b/testcases/kernel/syscalls/sendfile/sendfile08.c
index a29632712..868a610e4 100644
--- a/testcases/kernel/syscalls/sendfile/sendfile08.c
+++ b/testcases/kernel/syscalls/sendfile/sendfile08.c
@@ -1,123 +1,77 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
 /*
  * Copyright (C) 2012 Red Hat, Inc.
- *
- * This program is free software;  you can redistribute it and/or modify
- * it under the terms of the GNU General Public License as published by
- * the Free Software Foundation; either version 2 of the License, or
- * (at your option) any later version.
- *
- * This program 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 General Public License for more details.
- *
- * You should have received a copy of the GNU General Public License
- * along with this program;  if not, write to the Free Software
- * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
  */

-/*
+/*\
+ * [Description]
+ *
  * Bug in the splice code has caused the file position on the write side
  * of the sendfile system call to be incorrectly set to the read side file
  * position. This can result in the data being written to an incorrect offset.
  *
- * This is a regression test for kernel commit
- * 2cb4b05e7647891b46b91c07c9a60304803d1688
+ * This is a regression test for kernel commit 2cb4b05e7647.
  */

-#include <sys/sendfile.h>
-#include <sys/stat.h>
-#include <sys/types.h>
-#include <errno.h>
-#include <fcntl.h>
 #include <stdio.h>
+#include <fcntl.h>
 #include <string.h>
 #include <unistd.h>
-#include "test.h"
-#include "safe_macros.h"
+#include <sys/stat.h>
+#include <sys/types.h>
+#include <sys/sendfile.h>
+
+#include "tst_test.h"

-#define TEST_MSG_IN "world"
-#define TEST_MSG_OUT "hello"
-#define TEST_MSG_ALL (TEST_MSG_OUT TEST_MSG_IN)
+#define IN_FILE		"in_file"
+#define OUT_FILE	"out_file"

-TCID_DEFINE(sendfile08);
-int TST_TOTAL = 1;
+#define TEST_MSG_IN	"world"
+#define TEST_MSG_OUT	"hello"
+#define TEST_MSG_ALL	(TEST_MSG_OUT TEST_MSG_IN)

 static int in_fd;
 static int out_fd;
-static char *in_file = "sendfile08.in";
-static char *out_file = "sendfile08.out";

-static void cleanup(void);
-static void setup(void);
-
-int main(int argc, char *argv[])
+static void run(void)
 {
-	int lc;
-	int ret;
-	char buf[BUFSIZ];
+	TEST(sendfile(out_fd, in_fd, NULL, strlen(TEST_MSG_IN)));
+	if (TST_RET == -1)
+		tst_brk(TBROK | TTERRNO, "sendfile() failed");

-	tst_parse_opts(argc, argv, NULL, NULL);
-
-	setup();
-
-	for (lc = 0; TEST_LOOPING(lc); lc++) {
-		TEST(sendfile(out_fd, in_fd, NULL, strlen(TEST_MSG_IN)));
-
-		if (TEST_RETURN == -1)
-			tst_brkm(TBROK | TTERRNO, cleanup, "sendfile() failed");
-
-		ret = SAFE_LSEEK(cleanup, out_fd, 0, SEEK_SET);
-		ret = read(out_fd, buf, BUFSIZ);
-		if (ret == -1)
-			tst_brkm(TBROK | TERRNO, cleanup, "read %s failed",
-				 out_file);
-
-		if (!strncmp(buf, TEST_MSG_ALL, strlen(TEST_MSG_ALL)))
-			tst_resm(TPASS, "sendfile(2) copies data correctly");
-		else
-			tst_resm(TFAIL, "sendfile(2) copies data incorrectly."
-				 " Expect \"%s%s\", got \"%s\"", TEST_MSG_OUT,
-				 TEST_MSG_IN, buf);
-	}
-
-	cleanup();
-	tst_exit();
+	char buf[BUFSIZ];
+	SAFE_LSEEK(out_fd, 0, SEEK_SET);
+	SAFE_READ(0, out_fd, buf, BUFSIZ);
+
+	if (!strncmp(buf, TEST_MSG_ALL, strlen(TEST_MSG_ALL)))
+		tst_res(TPASS, "sendfile(2) copies data correctly");
+	else
+		tst_res(TFAIL, "sendfile(2) copies data incorrectly. "
+			       "Expect \"%s%s\", got \"%s\"",
+			TEST_MSG_OUT, TEST_MSG_IN, buf);
 }

 static void setup(void)
 {
-	int ret;
-
-	/* Disable test if the version of the kernel is less than 2.6.33 */
-	if ((tst_kvercmp(2, 6, 33)) < 0) {
-		tst_resm(TCONF, "The out_fd must be socket before kernel");
-		tst_brkm(TCONF, NULL, "2.6.33, see kernel commit cc56f7d");
-	}
+	in_fd = SAFE_CREAT(IN_FILE, 0700);
+	SAFE_WRITE(1, in_fd, TEST_MSG_IN, strlen(TEST_MSG_IN));
+	SAFE_CLOSE(in_fd);
+	in_fd = SAFE_OPEN(IN_FILE, O_RDONLY);

-	TEST_PAUSE;
-
-	tst_tmpdir();
-
-	in_fd = SAFE_CREAT(cleanup, in_file, 0700);
-
-	ret = write(in_fd, TEST_MSG_IN, strlen(TEST_MSG_IN));
-	if (ret == -1)
-		tst_brkm(TBROK | TERRNO, cleanup, "Write %s failed", in_file);
-	close(in_fd);
-
-	in_fd = SAFE_OPEN(cleanup, in_file, O_RDONLY);
-	out_fd = SAFE_OPEN(cleanup, out_file, O_TRUNC | O_CREAT | O_RDWR, 0777);
-
-	ret = write(out_fd, TEST_MSG_OUT, strlen(TEST_MSG_OUT));
-	if (ret == -1)
-		tst_brkm(TBROK | TERRNO, cleanup, "Write %s failed", out_file);
+	out_fd = SAFE_OPEN(OUT_FILE, O_TRUNC | O_CREAT | O_RDWR, 0777);
+	SAFE_WRITE(1, out_fd, TEST_MSG_OUT, strlen(TEST_MSG_OUT));
 }

 static void cleanup(void)
 {
-	close(out_fd);
-	close(in_fd);
-
-	tst_rmdir();
+	SAFE_CLOSE(in_fd);
+	SAFE_CLOSE(out_fd);
 }
+
+static struct tst_test test = {
+	.min_kver = "2.6.33",
+	.needs_tmpdir = 1,
+	.setup = setup,
+	.cleanup = cleanup,
+	.test_all = run,
+};
--
2.17.1

[LTP] [PATCH 2/2] syscalls/sendfile: Convert sendfile09 to the new API

[Xie Ziyao]

1. Convert sendfile09 to the new API;
2. Bugfix for running with option "-i":
   * OFF_T offset = tc[i].offset;
   Use offset instead of tc[i].offset when calling sendfile(2).

Signed-off-by: Xie Ziyao <xieziyao@huawei.com>
---
 .../kernel/syscalls/sendfile/sendfile09.c     | 234 ++++++------------
 1 file changed, 72 insertions(+), 162 deletions(-)

diff --git a/testcases/kernel/syscalls/sendfile/sendfile09.c b/testcases/kernel/syscalls/sendfile/sendfile09.c
index b9d9c8407..3d8a6b6e8 100644
--- a/testcases/kernel/syscalls/sendfile/sendfile09.c
+++ b/testcases/kernel/syscalls/sendfile/sendfile09.c
@@ -1,74 +1,45 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
 /*
- *
- *   Copyright (c) International Business Machines  Corp., 2014
- *
- *   This program is free software;  you can redistribute it and/or modify
- *   it under the terms of the GNU General Public License as published by
- *   the Free Software Foundation; either version 2 of the License, or
- *   (at your option) any later version.
- *
- *   This program 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 General Public License for more details.
- *
- *   You should have received a copy of the GNU General Public License
- *   along with this program;  if not, write to the Free Software
- *   Foundation, Inc.
+ * Copyright (c) International Business Machines  Corp., 2014
  */
-/*
- * NAME
- *        sendfile09.c
+
+/*\
+ * [Description]
  *
- * DESCRIPTION
- *        Testcase copied from sendfile02.c to test the basic functionality of
- *        the sendfile(2) system call on large file. There is a kernel bug which
- *        introduced by commit 8f9c0119d7ba and fixed by commit 5d73320a96fcc.
+ * Testcase copied from sendfile02.c to test the basic functionality of
+ * the sendfile(2) system call on large file. There is a kernel bug which
+ * introduced by commit 8f9c0119d7ba and fixed by commit 5d73320a96fcc.
  *
- * ALGORITHM
- *        1. call sendfile(2) with offset at 0
- *        2. call sendfile(2) with offset at 3GB
+ * [Algorithm]
  *
- * USAGE:  <for command-line>
- *  sendfile09 [-c n] [-i n] [-I x] [-P x] [-t]
- *     where,
- *             -i n : Execute test n times.
- *             -I x : Execute test for x seconds.
- *             -P x : Pause for x seconds between iterations.
- *             -t   : Turn on syscall timing.
+ * 1. Call sendfile(2) with offset at 0;
+ * 2. Call sendfile(2) with offset at 3GB.
  *
+ * [Restrictions]
  *
- * RESTRICTIONS
- *        Only supports 64bit systems and kernel 2.6.33 or above
+ * Only supports 64bit systems and kernel 2.6.33 or above.
  */
-#include <stdio.h>
-#include <errno.h>
+
 #include <fcntl.h>
 #include <sys/stat.h>
 #include <sys/sendfile.h>
 #include <sys/types.h>
 #include <unistd.h>
 #include <inttypes.h>
-#include "test.h"
-#include "safe_macros.h"
-#include "lapi/abisize.h"

-#ifndef OFF_T
-#define OFF_T off_t
-#endif /* Not def: OFF_T */
+#include "tst_test.h"

-TCID_DEFINE(sendfile09);
-
-static char *in_file = "in";
-static char *out_file = "out";
-static int fd;
-static int in_fd;
-static int out_fd;
+#ifdef TST_ABI32
+tst_brkm(TCONF, "This test is only for 64bit");
+#endif

-static void cleanup(void);
-static void setup(void);
+#ifndef OFF_T
+#define OFF_T off_t
+#endif

-#define ONE_GB (INT64_C(1) << 30)
+#define ONE_GB		(INT64_C(1) << 30)
+#define IN_FILE		"in_file"
+#define OUT_FILE	"out_file"

 static struct test_case_t {
 	char *desc;
@@ -76,124 +47,63 @@ static struct test_case_t {
 	int64_t count;
 	int64_t exp_retval;
 	int64_t exp_updated_offset;
-} testcases[] = {
-	{ "Test sendfile(2) with offset at 0",
-		0, ONE_GB, ONE_GB, ONE_GB},
-	{ "Test sendfile(2) with offset at 3GB",
-		3*ONE_GB, ONE_GB, ONE_GB, 4*ONE_GB}
+} tc[] = {
+	{ "offset at 0", 0, ONE_GB, ONE_GB, ONE_GB },
+	{ "offset@3GB", 3 * ONE_GB, ONE_GB, ONE_GB, 4 * ONE_GB }
 };

-static int TST_TOTAL = ARRAY_SIZE(testcases);
-
-void do_sendfile(struct test_case_t *t)
-{
-	off_t before_pos, after_pos;
-
-	out_fd = SAFE_OPEN(cleanup, out_file, O_WRONLY);
-	in_fd = SAFE_OPEN(cleanup, in_file, O_RDONLY);
-	before_pos = SAFE_LSEEK(cleanup, in_fd, 0, SEEK_CUR);
-
-	TEST(sendfile(out_fd, in_fd, &t->offset, t->count));
-	if (TEST_RETURN == -1)
-		tst_brkm(TBROK | TTERRNO, cleanup, "sendfile(2) failed");
-
-	after_pos = SAFE_LSEEK(cleanup, in_fd, 0, SEEK_CUR);
-
-	if (TEST_RETURN != t->exp_retval) {
-		tst_resm(TFAIL, "sendfile(2) failed to return "
-			"expected value, expected: %" PRId64 ", "
-			"got: %ld", t->exp_retval,
-			TEST_RETURN);
-	} else if (t->offset != t->exp_updated_offset) {
-		tst_resm(TFAIL, "sendfile(2) failed to update "
-			"OFFSET parameter to expected value, "
-			"expected: %" PRId64 ", got: %" PRId64,
-			t->exp_updated_offset,
-			(int64_t) t->offset);
-	} else if (before_pos != after_pos) {
-		tst_resm(TFAIL, "sendfile(2) updated the file position "
-			" of in_fd unexpectedly, expected file position: %"
-			PRId64 ", " " actual file position %" PRId64,
-			(int64_t) before_pos, (int64_t) after_pos);
-	} else {
-		tst_resm(TPASS, "%s", t->desc);
-	}
-
-	close(in_fd);
-	close(out_fd);
-}
-
-/*
- * setup() - performs all ONE TIME setup for this test.
- */
-void setup(void)
+static void setup(void)
 {
-	int i;
-
-	tst_sig(FORK, DEF_HANDLER, cleanup);
-	TEST_PAUSE;
-
-	/* make a temporary directory and cd to it */
-	tst_tmpdir();
-
-	if (!tst_fs_has_free(NULL, ".", 5, TST_GB))
-		tst_brkm(TCONF, cleanup, "sendfile(2) on large file"
-			" needs 5G free space.");
+	if (!tst_fs_has_free(".", 5, TST_GB))
+		tst_brk(TCONF, "Test on large file needs 5G free space");

-	/* create a 4G file */
-	fd = SAFE_CREAT(cleanup, in_file, 00700);
-	for (i = 1; i <= (4 * 1024); i++) {
-		SAFE_LSEEK(cleanup, fd, 1024 * 1024 - 1, SEEK_CUR);
-		SAFE_WRITE(cleanup, 1, fd, "C", 1);
+	int fd = SAFE_CREAT(IN_FILE, 00700);
+	for (int i = 1; i <= (4 * 1024); ++i) {
+		SAFE_LSEEK(fd, 1024 * 1024 - 1, SEEK_CUR);
+		SAFE_WRITE(1, fd, "C", 1);
 	}
-	close(fd);
+	SAFE_CLOSE(fd);

-	fd = SAFE_CREAT(cleanup, out_file, 00700);
-	close(fd);
+	fd = SAFE_CREAT(OUT_FILE, 00700);
+	SAFE_CLOSE(fd);
 }

-void cleanup(void)
+static void run(unsigned int i)
 {
-	if (fd > 0)
-		close(fd);
+	int in_fd = SAFE_OPEN(IN_FILE, O_RDONLY);
+	int out_fd = SAFE_OPEN(OUT_FILE, O_WRONLY);
+	OFF_T offset = tc[i].offset;

-	if (in_fd > 0)
-		close(in_fd);
-
-	if (out_fd > 0)
-		close(out_fd);
-
-	tst_rmdir();
+	off_t before_pos, after_pos;
+	before_pos = SAFE_LSEEK(in_fd, 0, SEEK_CUR);
+
+	TEST(sendfile(out_fd, in_fd, &offset, tc[i].count));
+	after_pos = SAFE_LSEEK(in_fd, 0, SEEK_CUR);
+
+	if (TST_RET != tc[i].exp_retval)
+		tst_res(TFAIL, "sendfile(2) failed to return expected value, "
+			       "expected: %" PRId64 ", got: %ld",
+			tc[i].exp_retval, TST_RET);
+	else if (offset != tc[i].exp_updated_offset)
+		tst_res(TFAIL, "sendfile(2) failed to update OFFSET parameter to "
+			       "expected value, expected: %" PRId64 ", got: %" PRId64,
+			tc[i].exp_updated_offset, (int64_t)(offset));
+	else if (before_pos != after_pos)
+		tst_res(TFAIL, "sendfile(2) updated the file position of in_fd "
+			       "unexpectedly, expected file position: %" PRId64
+			       ", actual file position %" PRId64,
+			(int64_t)(before_pos), (int64_t)(after_pos));
+	else
+		tst_res(TPASS, "sendfile(2) with %s", tc[i].desc);
+
+	SAFE_CLOSE(in_fd);
+	SAFE_CLOSE(out_fd);
 }

-int main(int ac, char **av)
-{
-	int i;
-	int lc;
-
-#ifdef TST_ABI32
-	tst_brkm(TCONF, NULL, "This test is only for 64bit");
-#endif
-
-	if (tst_kvercmp(2, 6, 33) < 0) {
-		tst_resm(TINFO, "sendfile(2) on large file "
-			"skipped for kernels < 2.6.33");
-		return 0;
-	}
-
-	tst_parse_opts(ac, av, NULL, NULL);
-
-	setup();
-
-	/*
-	 * The following loop checks looping state if -c option given
-	 */
-	for (lc = 0; TEST_LOOPING(lc); lc++) {
-		tst_count = 0;
-		for (i = 0; i < TST_TOTAL; ++i)
-			do_sendfile(&testcases[i]);
-	}
-
-	cleanup();
-	tst_exit();
-}
+static struct tst_test test = {
+	.needs_tmpdir = 1,
+	.setup = setup,
+	.test = run,
+	.tcnt = ARRAY_SIZE(tc),
+	.min_kver = "2.6.33",
+};
--
2.17.1

[LTP] [PATCH 1/2] syscalls/sendfile: Convert sendfile08 to the new API

[Cyril Hrubis]

Hi!
> -/*
> +/*\
> + * [Description]
> + *
>   * Bug in the splice code has caused the file position on the write side
>   * of the sendfile system call to be incorrectly set to the read side file
>   * position. This can result in the data being written to an incorrect offset.
>   *
> - * This is a regression test for kernel commit
> - * 2cb4b05e7647891b46b91c07c9a60304803d1688
> + * This is a regression test for kernel commit 2cb4b05e7647.
                                                    ^
						    This should be added
						    to the tst_test
						    structure as a tags
						    as well.

-- 
Cyril Hrubis
chrubis@suse.cz

[LTP] [PATCH 2/2] syscalls/sendfile: Convert sendfile09 to the new API

[Cyril Hrubis]

Hi!
> +/*\
> + * [Description]
>   *
> - * DESCRIPTION
> - *        Testcase copied from sendfile02.c to test the basic functionality of
> - *        the sendfile(2) system call on large file. There is a kernel bug which
> - *        introduced by commit 8f9c0119d7ba and fixed by commit 5d73320a96fcc.
> + * Testcase copied from sendfile02.c to test the basic functionality of
> + * the sendfile(2) system call on large file. There is a kernel bug which
> + * introduced by commit 8f9c0119d7ba and fixed by commit 5d73320a96fcc.
                            ^
	Here as well the commit that introduced the bug should go to
	.tags.
>   *
> - * ALGORITHM
> - *        1. call sendfile(2) with offset at 0
> - *        2. call sendfile(2) with offset at 3GB
> + * [Algorithm]
>   *
> - * USAGE:  <for command-line>
> - *  sendfile09 [-c n] [-i n] [-I x] [-P x] [-t]
> - *     where,
> - *             -i n : Execute test n times.
> - *             -I x : Execute test for x seconds.
> - *             -P x : Pause for x seconds between iterations.
> - *             -t   : Turn on syscall timing.
> + * 1. Call sendfile(2) with offset at 0;
> + * 2. Call sendfile(2) with offset at 3GB.
>   *
> + * [Restrictions]

So far we had only [Description] and [Algorithm] but adding
[Restrictions] sounds reasonable.

>   *
> - * RESTRICTIONS
> - *        Only supports 64bit systems and kernel 2.6.33 or above
> + * Only supports 64bit systems and kernel 2.6.33 or above.

I guess that there is no point in mentioning the kernel version here, we
have it in the tst_test structure and it's exported from that structure
into the metadata as well.

>   */
> -#include <stdio.h>
> -#include <errno.h>
> +
>  #include <fcntl.h>
>  #include <sys/stat.h>
>  #include <sys/sendfile.h>
>  #include <sys/types.h>
>  #include <unistd.h>
>  #include <inttypes.h>
> -#include "test.h"
> -#include "safe_macros.h"
> -#include "lapi/abisize.h"
> 
> -#ifndef OFF_T
> -#define OFF_T off_t
> -#endif /* Not def: OFF_T */
> +#include "tst_test.h"
> 
> -TCID_DEFINE(sendfile09);
> -
> -static char *in_file = "in";
> -static char *out_file = "out";
> -static int fd;
> -static int in_fd;
> -static int out_fd;
> +#ifdef TST_ABI32
> +tst_brkm(TCONF, "This test is only for 64bit");
> +#endif

Does this even compile on 32bit?

HINT: You can compile LTP for 32bit with ./configure CFLAGS=-m32 LDFLAGS=-m32


-- 
Cyril Hrubis
chrubis@suse.cz

[LTP] [PATCH 2/2] syscalls/sendfile: Convert sendfile09 to the new API

[Cyril Hrubis]

Hi!
> >> + * Testcase copied from sendfile02.c to test the basic functionality of
> >> + * the sendfile(2) system call on large file. There is a kernel bug which
> >> + * introduced by commit 8f9c0119d7ba and fixed by commit 5d73320a96fcc.
> >                              ^
> > 	Here as well the commit that introduced the bug should go to
> > 	.tags.

And I made a mistake here, we put the commit that fixes the bug into the
tags, so the second one should be in the metadata, but I can fix that
before the patches are applied. Sorry for the confusion.

-- 
Cyril Hrubis
chrubis@suse.cz

[LTP] [PATCH 2/2] syscalls/sendfile: Convert sendfile09 to the new API

[Xie Ziyao]

Hi,

>> + * Testcase copied from sendfile02.c to test the basic functionality of
>> + * the sendfile(2) system call on large file. There is a kernel bug which
>> + * introduced by commit 8f9c0119d7ba and fixed by commit 5d73320a96fcc.
>                              ^
> 	Here as well the commit that introduced the bug should go to
> 	.tags.
Changed for sendfile{08, 09}, thanks a lot.

>> + * [Restrictions]
> 
> So far we had only [Description] and [Algorithm] but adding
> [Restrictions] sounds reasonable.
> 
>>    *
>> - * RESTRICTIONS
>> - *        Only supports 64bit systems and kernel 2.6.33 or above
>> + * Only supports 64bit systems and kernel 2.6.33 or above.
> 
> I guess that there is no point in mentioning the kernel version here, we
> have it in the tst_test structure and it's exported from that structure
> into the metadata as well.Changed and moved [Restrictions] to [Description].

>> +#ifdef TST_ABI32
>> +tst_brkm(TCONF, "This test is only for 64bit");
>> +#endif
> 
> Does this even compile on 32bit?
> 
> HINT: You can compile LTP for 32bit with ./configure CFLAGS=-m32 LDFLAGS=-m32
Sorry about this mistake and I fixed it with modifications above in the 
v2 version.

Please see: https://patchwork.ozlabs.org/project/ltp/list/?series=244863

Thanks for your review, Cyril.

Kind Regards,
Ziyao

[LTP] [PATCH 1/2] syscalls/sendfile: Convert sendfile08 to the new API

[Petr Vorel]

Hi Xio,

> +++ b/testcases/kernel/syscalls/sendfile/sendfile08.c
I'd put your or LTP copyright (as your wish) because test was significantly
changed. (We had some copyright issues in the past thus it's better to state it.)
...
> +/*\
> + * [Description]
> + *
>   * Bug in the splice code has caused the file position on the write side
>   * of the sendfile system call to be incorrectly set to the read side file
>   * position. This can result in the data being written to an incorrect offset.
>   *
> - * This is a regression test for kernel commit
> - * 2cb4b05e7647891b46b91c07c9a60304803d1688
> + * This is a regression test for kernel commit 2cb4b05e7647.

nit: I wonder if we want to repeat what we already declare in .min_kver.
This is not specific to this patch, we keep doing it, but IMHO necessary
and we should stop that.
>   */

> -#include <sys/sendfile.h>
> -#include <sys/stat.h>
> -#include <sys/types.h>
> -#include <errno.h>
> -#include <fcntl.h>
>  #include <stdio.h>
> +#include <fcntl.h>
>  #include <string.h>
>  #include <unistd.h>
> -#include "test.h"
> -#include "safe_macros.h"
> +#include <sys/stat.h>
> +#include <sys/types.h>
> +#include <sys/sendfile.h>

nit: it looks to me that only <stdio.h> <string.h> <sys/sendfile.h> are needed.
But maybe others are needed and included in other headers.

Also only these were needed in legacy API:
#include <sys/sendfile.h>
#include <errno.h>
#include "test.h"
#include "safe_macros.h"
But <errno.h> is needed only for legacy API => use just these 3 mentioned above.

...
> +	char buf[BUFSIZ];
> +	SAFE_LSEEK(out_fd, 0, SEEK_SET);
nit: sendfile08.c:43: WARNING: Missing a blank line after declarations
It's actually more readable to have blank line after char buf[BUFSIZ];

> +	SAFE_READ(0, out_fd, buf, BUFSIZ);
> +
> +	if (!strncmp(buf, TEST_MSG_ALL, strlen(TEST_MSG_ALL)))
> +		tst_res(TPASS, "sendfile(2) copies data correctly");
> +	else
> +		tst_res(TFAIL, "sendfile(2) copies data incorrectly. "
> +			       "Expect \"%s%s\", got \"%s\"",
> +			TEST_MSG_OUT, TEST_MSG_IN, buf);

sendfile08.c:50: WARNING: quoted string split across lines

	if (!strncmp(buf, TEST_MSG_ALL, strlen(TEST_MSG_ALL))) {
		tst_res(TPASS, "sendfile() copied data correctly");
		return;
	}

	tst_res(TFAIL, "sendfile() copied data incorrectly: '%s', expected '%s%s'",
			buf, TEST_MSG_OUT, TEST_MSG_IN);

i.e. not splitting string, get some space by return instead else,
we don't mind using single quote (code is more readable),
removing also 2 in sendfile(2) (2 is man section, but that's just confusing).

Changes are minor, if we agre on that it can be done during merge.

Reviewed-by: Petr Vorel <pvorel@suse.cz>

Kind regards,
Petr

[LTP] [PATCH 2/2] syscalls/sendfile: Convert sendfile09 to the new API

[Petr Vorel]

Hi Xie,

> +++ b/testcases/kernel/syscalls/sendfile/sendfile09.c
...
> + * Copyright (c) International Business Machines  Corp., 2014
Again, missing copyright.

> +/*\
> + * [Description]
>   *
> - * DESCRIPTION
> - *        Testcase copied from sendfile02.c to test the basic functionality of
> - *        the sendfile(2) system call on large file. There is a kernel bug which
> - *        introduced by commit 8f9c0119d7ba and fixed by commit 5d73320a96fcc.
> + * Testcase copied from sendfile02.c to test the basic functionality of
> + * the sendfile(2) system call on large file. There is a kernel bug which
> + * introduced by commit 8f9c0119d7ba and fixed by commit 5d73320a96fcc.
If 8f9c0119d7ba caused a regression it shouldn't be in .tags (it contains
commits which are fixes and should be backported). Also it's a question if it's
useful info, because this commit is mentioned in 5d73320a96fcc (fixing commit).

>   *
> - * ALGORITHM
> - *        1. call sendfile(2) with offset at 0
> - *        2. call sendfile(2) with offset at 3GB
> + * [Algorithm]
>   *
> - * USAGE:  <for command-line>
> - *  sendfile09 [-c n] [-i n] [-I x] [-P x] [-t]
> - *     where,
> - *             -i n : Execute test n times.
> - *             -I x : Execute test for x seconds.
> - *             -P x : Pause for x seconds between iterations.
> - *             -t   : Turn on syscall timing.
> + * 1. Call sendfile(2) with offset at 0;
> + * 2. Call sendfile(2) with offset at 3GB.
>   *
> + * [Restrictions]
>   *
> - * RESTRICTIONS
> - *        Only supports 64bit systems and kernel 2.6.33 or above
> + * Only supports 64bit systems and kernel 2.6.33 or above.
nit: Maybe: Only 64bit systems are supported.

I'm not a native speaker, but "Only supports" sounds wrong to me.
Also although I'd keep .min_kver, mentioning it is IMHO necessary -
why to repeat info we have in .tags? We still do that, but IMHO we should
stop doing it. And this ancient version is certainly not worth duplicity
(latest tested kernel is 3.10 [1]).

sendfile09.c:58: WARNING: Missing a blank line after declarations
sendfile09.c:75: WARNING: Missing a blank line after declarations
sendfile09.c:80: WARNING: Comparisons should place the constant on the right side of the test
sendfile09.c:82: WARNING: quoted string split across lines
sendfile09.c:86: WARNING: quoted string split across lines
sendfile09.c:90: WARNING: quoted string split across lines

>   */
> -#include <stdio.h>
> -#include <errno.h>
> +
>  #include <fcntl.h>
>  #include <sys/stat.h>
>  #include <sys/sendfile.h>
>  #include <sys/types.h>
>  #include <unistd.h>
>  #include <inttypes.h>
Again, only these are needed:

#include <inttypes.h>
#include <sys/sendfile.h>

#include "tst_test.h"
#include "lapi/abisize.h"

> -static void cleanup(void);
> -static void setup(void);
> +#ifndef OFF_T
> +#define OFF_T off_t
> +#endif
I wonder where OFF_T comes from and if we can just simply use off_t.

> -#define ONE_GB (INT64_C(1) << 30)
> +#define ONE_GB		(INT64_C(1) << 30)
> +#define IN_FILE		"in_file"
> +#define OUT_FILE	"out_file"

:...
> +static void setup(void)
>  {
> -	int i;
> -
> -	tst_sig(FORK, DEF_HANDLER, cleanup);
> -	TEST_PAUSE;
> -
> -	/* make a temporary directory and cd to it */
> -	tst_tmpdir();
> -
> -	if (!tst_fs_has_free(NULL, ".", 5, TST_GB))
> -		tst_brkm(TCONF, cleanup, "sendfile(2) on large file"
> -			" needs 5G free space.");
> +	if (!tst_fs_has_free(".", 5, TST_GB))
> +		tst_brk(TCONF, "Test on large file needs 5G free space");

> -	/* create a 4G file */
> -	fd = SAFE_CREAT(cleanup, in_file, 00700);
> -	for (i = 1; i <= (4 * 1024); i++) {
> -		SAFE_LSEEK(cleanup, fd, 1024 * 1024 - 1, SEEK_CUR);
> -		SAFE_WRITE(cleanup, 1, fd, "C", 1);
> +	int fd = SAFE_CREAT(IN_FILE, 00700);
> +	for (int i = 1; i <= (4 * 1024); ++i) {
This will lead to error in old compilers:
error: 'for' loop initial declarations are only allowed in C99 mode
https://travis-ci.org/github/pevik/ltp/jobs/771837120
https://travis-ci.org/github/pevik/ltp/jobs/771837126

It must be:

int i;

...
for (i = 1; i <= (4 * 1024); ++i) {

Thus probably better to declare fd before as well.

int i, fd;

...
> +static void run(unsigned int i)
>  {
...
> +	off_t before_pos, after_pos;
> +	before_pos = SAFE_LSEEK(in_fd, 0, SEEK_CUR);
> +
> +	TEST(sendfile(out_fd, in_fd, &offset, tc[i].count));
> +	after_pos = SAFE_LSEEK(in_fd, 0, SEEK_CUR);
> +
> +	if (TST_RET != tc[i].exp_retval)
> +		tst_res(TFAIL, "sendfile(2) failed to return expected value, "
> +			       "expected: %" PRId64 ", got: %ld",
> +			tc[i].exp_retval, TST_RET);
> +	else if (offset != tc[i].exp_updated_offset)
> +		tst_res(TFAIL, "sendfile(2) failed to update OFFSET parameter to "
> +			       "expected value, expected: %" PRId64 ", got: %" PRId64,
> +			tc[i].exp_updated_offset, (int64_t)(offset));
> +	else if (before_pos != after_pos)
> +		tst_res(TFAIL, "sendfile(2) updated the file position of in_fd "
> +			       "unexpectedly, expected file position: %" PRId64
> +			       ", actual file position %" PRId64,
> +			(int64_t)(before_pos), (int64_t)(after_pos));
Yes, we probably cannot avoid splitting string here, unless TST_EXP_FAIL() can
be used here.

I'd avoid 2 in "sendfile(2).

> +	else
> +		tst_res(TPASS, "sendfile(2) with %s", tc[i].desc);

Again, minor things, I can fix them before merge.

Kind regards,
Petr

[LTP] [PATCH 1/2] syscalls/sendfile: Convert sendfile08 to the new API

[Xie Ziyao]

>> +++ b/testcases/kernel/syscalls/sendfile/sendfile08.c
> I'd put your or LTP copyright (as your wish) because test was significantly
> changed. (We had some copyright issues in the past thus it's better to state it.)
> ...
>> +/*\
>> + * [Description]
>> + *
>>    * Bug in the splice code has caused the file position on the write side
>>    * of the sendfile system call to be incorrectly set to the read side file
>>    * position. This can result in the data being written to an incorrect offset.
>>    *
>> - * This is a regression test for kernel commit
>> - * 2cb4b05e7647891b46b91c07c9a60304803d1688
>> + * This is a regression test for kernel commit 2cb4b05e7647.
> 
> nit: I wonder if we want to repeat what we already declare in .min_kver.
> This is not specific to this patch, we keep doing it, but IMHO necessary
> and we should stop that.
Agree with you.

>>    */
> 
>> -#include <sys/sendfile.h>
>> -#include <sys/stat.h>
>> -#include <sys/types.h>
>> -#include <errno.h>
>> -#include <fcntl.h>
>>   #include <stdio.h>
>> +#include <fcntl.h>
>>   #include <string.h>
>>   #include <unistd.h>
>> -#include "test.h"
>> -#include "safe_macros.h"
>> +#include <sys/stat.h>
>> +#include <sys/types.h>
>> +#include <sys/sendfile.h>
> 
> nit: it looks to me that only <stdio.h> <string.h> <sys/sendfile.h> are needed.
+1

> But maybe others are needed and included in other headers.
> 
> Also only these were needed in legacy API:
> #include <sys/sendfile.h>
> #include <errno.h>
> #include "test.h"
> #include "safe_macros.h"
> But <errno.h> is needed only for legacy API => use just these 3 mentioned above.
> 
> ...
>> +	char buf[BUFSIZ];
>> +	SAFE_LSEEK(out_fd, 0, SEEK_SET);
> nit: sendfile08.c:43: WARNING: Missing a blank line after declarations
> It's actually more readable to have blank line after char buf[BUFSIZ];
+1

> 
>> +	SAFE_READ(0, out_fd, buf, BUFSIZ);
>> +
>> +	if (!strncmp(buf, TEST_MSG_ALL, strlen(TEST_MSG_ALL)))
>> +		tst_res(TPASS, "sendfile(2) copies data correctly");
>> +	else
>> +		tst_res(TFAIL, "sendfile(2) copies data incorrectly. "
>> +			       "Expect \"%s%s\", got \"%s\"",
>> +			TEST_MSG_OUT, TEST_MSG_IN, buf);
> 
> sendfile08.c:50: WARNING: quoted string split across lines
> 
> 	if (!strncmp(buf, TEST_MSG_ALL, strlen(TEST_MSG_ALL))) {
> 		tst_res(TPASS, "sendfile() copied data correctly");
> 		return;
> 	}
> 
> 	tst_res(TFAIL, "sendfile() copied data incorrectly: '%s', expected '%s%s'",
> 			buf, TEST_MSG_OUT, TEST_MSG_IN);
> 
> i.e. not splitting string, get some space by return instead else,
> we don't mind using single quote (code is more readable),
> removing also 2 in sendfile(2) (2 is man section, but that's just confusing).
> 
> Changes are minor, if we agre on that it can be done during merge.
> 
> Reviewed-by: Petr Vorel <pvorel@suse.cz>
> 
> Kind regards,
> Petr
> .
> 

Hi, Petr,

Thanks for your review and agree with changes above.

BTW, would you mind adding your changes to the v2 version? Please see: 
https://patchwork.ozlabs.org/project/ltp/list/?series=244863

Thank you,
Ziyao

[LTP] [PATCH 1/2] syscalls/sendfile: Convert sendfile08 to the new API

[Petr Vorel]

> Hi, Petr,

> Thanks for your review and agree with changes above.

> BTW, would you mind adding your changes to the v2 version? Please see:
> https://patchwork.ozlabs.org/project/ltp/list/?series=244863
Sure! I'm sorry, I actually looked locally into v2, but then replied to v1.

Kind regards,
Petr

> Thank you,
> Ziyao

[LTP] [PATCH 2/2] syscalls/sendfile: Convert sendfile09 to the new API

[Xie Ziyao]

>> +++ b/testcases/kernel/syscalls/sendfile/sendfile09.c
> ...
>> + * Copyright (c) International Business Machines  Corp., 2014
> Again, missing copyright.
I wonder if I should add copyright without changing the code logic.

> 
>> +/*\
>> + * [Description]
>>    *
>> - * DESCRIPTION
>> - *        Testcase copied from sendfile02.c to test the basic functionality of
>> - *        the sendfile(2) system call on large file. There is a kernel bug which
>> - *        introduced by commit 8f9c0119d7ba and fixed by commit 5d73320a96fcc.
>> + * Testcase copied from sendfile02.c to test the basic functionality of
>> + * the sendfile(2) system call on large file. There is a kernel bug which
>> + * introduced by commit 8f9c0119d7ba and fixed by commit 5d73320a96fcc.
> If 8f9c0119d7ba caused a regression it shouldn't be in .tags (it contains
> commits which are fixes and should be backported). Also it's a question if it's
> useful info, because this commit is mentioned in 5d73320a96fcc (fixing commit).
Agree with you.

> 
>>    *
>> - * ALGORITHM
>> - *        1. call sendfile(2) with offset at 0
>> - *        2. call sendfile(2) with offset at 3GB
>> + * [Algorithm]
>>    *
>> - * USAGE:  <for command-line>
>> - *  sendfile09 [-c n] [-i n] [-I x] [-P x] [-t]
>> - *     where,
>> - *             -i n : Execute test n times.
>> - *             -I x : Execute test for x seconds.
>> - *             -P x : Pause for x seconds between iterations.
>> - *             -t   : Turn on syscall timing.
>> + * 1. Call sendfile(2) with offset at 0;
>> + * 2. Call sendfile(2) with offset at 3GB.
>>    *
>> + * [Restrictions]
>>    *
>> - * RESTRICTIONS
>> - *        Only supports 64bit systems and kernel 2.6.33 or above
>> + * Only supports 64bit systems and kernel 2.6.33 or above.
> nit: Maybe: Only 64bit systems are supported.
> 
> I'm not a native speaker, but "Only supports" sounds wrong to me.
> Also although I'd keep .min_kver, mentioning it is IMHO necessary -
> why to repeat info we have in .tags? We still do that, but IMHO we should
> stop doing it. And this ancient version is certainly not worth duplicity
> (latest tested kernel is 3.10 [1]).
+1

> 
> sendfile09.c:58: WARNING: Missing a blank line after declarations
> sendfile09.c:75: WARNING: Missing a blank line after declarations
> sendfile09.c:80: WARNING: Comparisons should place the constant on the right side of the test
> sendfile09.c:82: WARNING: quoted string split across lines
> sendfile09.c:86: WARNING: quoted string split across lines
> sendfile09.c:90: WARNING: quoted string split across lines
> 
>>    */
>> -#include <stdio.h>
>> -#include <errno.h>
>> +
>>   #include <fcntl.h>
>>   #include <sys/stat.h>
>>   #include <sys/sendfile.h>
>>   #include <sys/types.h>
>>   #include <unistd.h>
>>   #include <inttypes.h>
> Again, only these are needed:
> 
> #include <inttypes.h>
> #include <sys/sendfile.h>
> 
> #include "tst_test.h"
> #include "lapi/abisize.h"
+1

> 
>> -static void cleanup(void);
>> -static void setup(void);
>> +#ifndef OFF_T
>> +#define OFF_T off_t
>> +#endif
> I wonder where OFF_T comes from and if we can just simply use off_t.
Not sure about this.

> 
>> -#define ONE_GB (INT64_C(1) << 30)
>> +#define ONE_GB		(INT64_C(1) << 30)
>> +#define IN_FILE		"in_file"
>> +#define OUT_FILE	"out_file"
> 
> :...
>> +static void setup(void)
>>   {
>> -	int i;
>> -
>> -	tst_sig(FORK, DEF_HANDLER, cleanup);
>> -	TEST_PAUSE;
>> -
>> -	/* make a temporary directory and cd to it */
>> -	tst_tmpdir();
>> -
>> -	if (!tst_fs_has_free(NULL, ".", 5, TST_GB))
>> -		tst_brkm(TCONF, cleanup, "sendfile(2) on large file"
>> -			" needs 5G free space.");
>> +	if (!tst_fs_has_free(".", 5, TST_GB))
>> +		tst_brk(TCONF, "Test on large file needs 5G free space");
> 
>> -	/* create a 4G file */
>> -	fd = SAFE_CREAT(cleanup, in_file, 00700);
>> -	for (i = 1; i <= (4 * 1024); i++) {
>> -		SAFE_LSEEK(cleanup, fd, 1024 * 1024 - 1, SEEK_CUR);
>> -		SAFE_WRITE(cleanup, 1, fd, "C", 1);
>> +	int fd = SAFE_CREAT(IN_FILE, 00700);
>> +	for (int i = 1; i <= (4 * 1024); ++i) {
> This will lead to error in old compilers:
> error: 'for' loop initial declarations are only allowed in C99 mode
> https://travis-ci.org/github/pevik/ltp/jobs/771837120
> https://travis-ci.org/github/pevik/ltp/jobs/771837126
> 
> It must be:
> 
> int i;
> 
> ...
> for (i = 1; i <= (4 * 1024); ++i) {
> 
> Thus probably better to declare fd before as well.
> 
> int i, fd;
I'll pay attention next time. Thanks.

> 
> ...
>> +static void run(unsigned int i)
>>   {
> ...
>> +	off_t before_pos, after_pos;
>> +	before_pos = SAFE_LSEEK(in_fd, 0, SEEK_CUR);
>> +
>> +	TEST(sendfile(out_fd, in_fd, &offset, tc[i].count));
>> +	after_pos = SAFE_LSEEK(in_fd, 0, SEEK_CUR);
>> +
>> +	if (TST_RET != tc[i].exp_retval)
>> +		tst_res(TFAIL, "sendfile(2) failed to return expected value, "
>> +			       "expected: %" PRId64 ", got: %ld",
>> +			tc[i].exp_retval, TST_RET);
>> +	else if (offset != tc[i].exp_updated_offset)
>> +		tst_res(TFAIL, "sendfile(2) failed to update OFFSET parameter to "
>> +			       "expected value, expected: %" PRId64 ", got: %" PRId64,
>> +			tc[i].exp_updated_offset, (int64_t)(offset));
>> +	else if (before_pos != after_pos)
>> +		tst_res(TFAIL, "sendfile(2) updated the file position of in_fd "
>> +			       "unexpectedly, expected file position: %" PRId64
>> +			       ", actual file position %" PRId64,
>> +			(int64_t)(before_pos), (int64_t)(after_pos));
> Yes, we probably cannot avoid splitting string here, unless TST_EXP_FAIL() can
> be used here.
> 
> I'd avoid 2 in "sendfile(2).
+1

> 
>> +	else
>> +		tst_res(TPASS, "sendfile(2) with %s", tc[i].desc);
> 
> Again, minor things, I can fix them before merge.
> 
> Kind regards,
> Petr
> .
> 

Hi, Petr,

Not sure whether to remove OFF_T, the other modifications worked fine 
for me.

Thanks for your review.

Kind regards,
Ziyao

[LTP] [PATCH 2/2] syscalls/sendfile: Convert sendfile09 to the new API

[Petr Vorel]

Hi Ziyao,

> > > +++ b/testcases/kernel/syscalls/sendfile/sendfile09.c
> > ...
> > > + * Copyright (c) International Business Machines  Corp., 2014
> > Again, missing copyright.
> I wonder if I should add copyright without changing the code logic.
IMHO yes. You significantly changed the code. At least we do that.

...
> > > -static void cleanup(void);
> > > -static void setup(void);
> > > +#ifndef OFF_T
> > > +#define OFF_T off_t
> > > +#endif
> > I wonder where OFF_T comes from and if we can just simply use off_t.
> Not sure about this.
@metan any idea?

It looks like nothing needs it:
https://travis-ci.org/github/pevik/ltp/builds/771843061

> > > +	for (int i = 1; i <= (4 * 1024); ++i) {
> > This will lead to error in old compilers:
> > error: 'for' loop initial declarations are only allowed in C99 mode
> > https://travis-ci.org/github/pevik/ltp/jobs/771837120
> > https://travis-ci.org/github/pevik/ltp/jobs/771837126

> > It must be:

> > int i;

> > ...
> > for (i = 1; i <= (4 * 1024); ++i) {

> > Thus probably better to declare fd before as well.

> > int i, fd;
> I'll pay attention next time. Thanks.
np, you obviously not work on legacy distro. Travis CI would tell you, but we
don't expect users to run Travis CI on their LTP fork to catch the failures
before sending it to ML (but of course feel free to set it up).

> Hi, Petr,

> Not sure whether to remove OFF_T, the other modifications worked fine for
> me.
Thanks for info, we sort out OFF_T and I'll push it fixed after the release.

> Thanks for your review.
yw, thanks a lot for your work!

Kind regards,
Petr

> Kind regards,
> Ziyao

[LTP] [PATCH 2/2] syscalls/sendfile: Convert sendfile09 to the new API

[Petr Vorel]

Hi all,
> Hi Ziyao,

> > > > +++ b/testcases/kernel/syscalls/sendfile/sendfile09.c
> > > ...
> > > > + * Copyright (c) International Business Machines  Corp., 2014
> > > Again, missing copyright.
> > I wonder if I should add copyright without changing the code logic.
> IMHO yes. You significantly changed the code. At least we do that.

> ...
> > > > -static void cleanup(void);
> > > > -static void setup(void);
> > > > +#ifndef OFF_T
> > > > +#define OFF_T off_t
> > > > +#endif
> > > I wonder where OFF_T comes from and if we can just simply use off_t.
> > Not sure about this.
> @metan any idea?

> It looks like nothing needs it:
> https://travis-ci.org/github/pevik/ltp/builds/771843061

I see it must stay, it's for non- "_64" versions, which have it defined in
Makefile.

Merged with changes below. Thanks!

BTW I wonder if test could be run with less than 5GB, which is quite a lot.

Kind regards,
Petr

diff --git testcases/kernel/syscalls/sendfile/sendfile08.c testcases/kernel/syscalls/sendfile/sendfile08.c
index ddb8f1dd2..48a971bfb 100644
--- testcases/kernel/syscalls/sendfile/sendfile08.c
+++ testcases/kernel/syscalls/sendfile/sendfile08.c
@@ -14,11 +14,7 @@
  */
 
 #include <stdio.h>
-#include <fcntl.h>
 #include <string.h>
-#include <unistd.h>
-#include <sys/stat.h>
-#include <sys/types.h>
 #include <sys/sendfile.h>
 
 #include "tst_test.h"
@@ -40,15 +36,17 @@ static void run(void)
 		tst_brk(TBROK | TTERRNO, "sendfile() failed");
 
 	char buf[BUFSIZ];
+
 	SAFE_LSEEK(out_fd, 0, SEEK_SET);
 	SAFE_READ(0, out_fd, buf, BUFSIZ);
 
-	if (!strncmp(buf, TEST_MSG_ALL, strlen(TEST_MSG_ALL)))
-		tst_res(TPASS, "sendfile(2) copies data correctly");
-	else
-		tst_res(TFAIL, "sendfile(2) copies data incorrectly. "
-			       "Expect \"%s%s\", got \"%s\"",
-			TEST_MSG_OUT, TEST_MSG_IN, buf);
+	if (!strncmp(buf, TEST_MSG_ALL, strlen(TEST_MSG_ALL))) {
+		tst_res(TPASS, "sendfile() copies data correctly");
+		return;
+	}
+
+	tst_res(TFAIL, "sendfile() copies data incorrectly: '%s' expected: '%s%s'",
+			buf, TEST_MSG_OUT, TEST_MSG_IN);
 }
 
 static void setup(void)
diff --git testcases/kernel/syscalls/sendfile/sendfile09.c testcases/kernel/syscalls/sendfile/sendfile09.c
index 667e314bb..297b3e212 100644
--- testcases/kernel/syscalls/sendfile/sendfile09.c
+++ testcases/kernel/syscalls/sendfile/sendfile09.c
@@ -7,23 +7,19 @@
  * [Description]
  *
  * Testcase copied from sendfile02.c to test the basic functionality of
- * the sendfile(2) system call on large file. There is a kernel bug which
+ * the sendfile() system call on large file. There is a kernel bug which
  * introduced by commit 8f9c0119d7ba9 and fixed by commit 5d73320a96fcc.
  *
  * Only supports 64bit systems.
  *
  * [Algorithm]
  *
- * 1. Call sendfile(2) with offset at 0;
- * 2. Call sendfile(2) with offset at 3GB.
+ * 1. Call sendfile() with offset at 0.
+ * 2. Call sendfile() with offset at 3GB.
  */
 
-#include <fcntl.h>
-#include <sys/stat.h>
-#include <sys/sendfile.h>
-#include <sys/types.h>
-#include <unistd.h>
 #include <inttypes.h>
+#include <sys/sendfile.h>
 
 #include "tst_test.h"
 #include "lapi/abisize.h"
@@ -51,11 +47,13 @@ static struct test_case_t {
 
 static void setup(void)
 {
+	int i, fd;
+
 	if (!tst_fs_has_free(".", 5, TST_GB))
 		tst_brk(TCONF, "Test on large file needs 5G free space");
 
-	int fd = SAFE_CREAT(IN_FILE, 00700);
-	for (int i = 1; i <= (4 * 1024); ++i) {
+	fd = SAFE_CREAT(IN_FILE, 00700);
+	for (i = 1; i <= (4 * 1024); ++i) {
 		SAFE_LSEEK(fd, 1024 * 1024 - 1, SEEK_CUR);
 		SAFE_WRITE(1, fd, "C", 1);
 	}
@@ -71,27 +69,27 @@ static void run(unsigned int i)
 	int out_fd = SAFE_OPEN(OUT_FILE, O_WRONLY);
 	OFF_T offset = tc[i].offset;
 
-	off_t before_pos, after_pos;
+	OFF_T before_pos, after_pos;
 	before_pos = SAFE_LSEEK(in_fd, 0, SEEK_CUR);
 
 	TEST(sendfile(out_fd, in_fd, &offset, tc[i].count));
 	after_pos = SAFE_LSEEK(in_fd, 0, SEEK_CUR);
 
 	if (TST_RET != tc[i].exp_retval)
-		tst_res(TFAIL, "sendfile(2) failed to return expected value, "
+		tst_res(TFAIL, "sendfile() failed to return expected value, "
 			       "expected: %" PRId64 ", got: %ld",
 			tc[i].exp_retval, TST_RET);
 	else if (offset != tc[i].exp_updated_offset)
-		tst_res(TFAIL, "sendfile(2) failed to update OFFSET parameter to "
+		tst_res(TFAIL, "sendfile() failed to update OFFSET parameter to "
 			       "expected value, expected: %" PRId64 ", got: %" PRId64,
 			tc[i].exp_updated_offset, (int64_t)(offset));
 	else if (before_pos != after_pos)
-		tst_res(TFAIL, "sendfile(2) updated the file position of in_fd "
+		tst_res(TFAIL, "sendfile() updated the file position of in_fd "
 			       "unexpectedly, expected file position: %" PRId64
 			       ", actual file position %" PRId64,
 			(int64_t)(before_pos), (int64_t)(after_pos));
 	else
-		tst_res(TPASS, "sendfile(2) with %s", tc[i].desc);
+		tst_res(TPASS, "sendfile() with %s", tc[i].desc);
 
 	SAFE_CLOSE(in_fd);
 	SAFE_CLOSE(out_fd);
@@ -104,7 +102,6 @@ static struct tst_test test = {
 	.tcnt = ARRAY_SIZE(tc),
 	.min_kver = "2.6.33",
 	.tags = (const struct tst_tag[]) {
-		{"linux-git", "8f9c0119d7ba9"},
 		{"linux-git", "5d73320a96fcc"},
 		{}
 	}

[LTP] [PATCH 2/2] syscalls/sendfile: Convert sendfile09 to the new API

[Petr Vorel]

> Hi!
> > >> + * Testcase copied from sendfile02.c to test the basic functionality of
> > >> + * the sendfile(2) system call on large file. There is a kernel bug which
> > >> + * introduced by commit 8f9c0119d7ba and fixed by commit 5d73320a96fcc.
> > >                              ^
> > > 	Here as well the commit that introduced the bug should go to
> > > 	.tags.

> And I made a mistake here, we put the commit that fixes the bug into the
> tags, so the second one should be in the metadata, but I can fix that
> before the patches are applied. Sorry for the confusion.
Yep I noticed that and discussed that and fixed it.
But I haven't noticed you reviewed v1, thus didn't put your tags.
Anyway, I merged fixed v1 version. Now I noticed v2, which was still including
some headers which were not needed.

Kind regards,
Petr

[LTP] [PATCH 2/2] syscalls/sendfile: Convert sendfile09 to the new API

[Xie Ziyao]

Hi,

> 
> BTW I wonder if test could be run with less than 5GB, which is quite a lot.
> 
According to the kernel commit 5d73320a96fcc,testcase for the sendfile 
of lengths greater than 2G (maybe offset+count) is OK.

IMHO, maybe we can reduce the size, but still a lot.

Kind regards,
Ziyao