splice02: Generate input in C

instead relying on shell piping data into it.

Found on SLES JeOS (https://progress.opensuse.org/issues/77260).

Also add metadata docs.

Reported-by: Martin Loviska <mloviska@suse.com>
Suggested-by: Cyril Hrubis <chrubis@suse.cz>
Signed-off-by: Petr Vorel <pvorel@suse.cz>
---
Hi,

Hope using TST_CHECKPOINT_*() correctly.
Not using tst_tag for such old kernel.

Kind regards,
Petr

 runtest/smoketest                           |  2 +-
 runtest/syscalls                            |  2 +-
 testcases/kernel/syscalls/splice/splice02.c | 67 ++++++++++++++++++++-
 3 files changed, 66 insertions(+), 5 deletions(-)

Hi!
> diff --git a/runtest/smoketest b/runtest/smoketest
> index 0c24fc1fa..ec0c088cb 100644
> --- a/runtest/smoketest
> +++ b/runtest/smoketest
> @@ -11,5 +11,5 @@ symlink01 symlink01
>  stat04 symlink01 -T stat04
>  utime01A symlink01 -T utime01
>  rename01A symlink01 -T rename01
> -splice02 seq 1 20 | splice02
> +splice02 splice02 -n 20

I do wonder if this should be replaced with something that includes a
shell pipe instead. It has been selected here to make sure we pass the
command line correctly to a shell interpreter.

Maybe something as:

shell_test01 echo "SUCCESS" | cat

>  route4-change-dst route-change-dst.sh
> diff --git a/runtest/syscalls b/runtest/syscalls
> index fe22ae5b6..355e71144 100644
> --- a/runtest/syscalls
> +++ b/runtest/syscalls
> @@ -1446,7 +1446,7 @@ socketpair02 socketpair02
>  sockioctl01 sockioctl01
>  
>  splice01 splice01
> -splice02 seq 1 20000 | splice02
> +splice02 splice02 -n 20000

Don't we default to 20000 in the test anyway? What is the point of
passing the default value here?

>  splice03 splice03
>  splice04 splice04
>  splice05 splice05
> diff --git a/testcases/kernel/syscalls/splice/splice02.c b/testcases/kernel/syscalls/splice/splice02.c
> index b579667b9..6e183ac7e 100644
> --- a/testcases/kernel/syscalls/splice/splice02.c
> +++ b/testcases/kernel/syscalls/splice/splice02.c
> @@ -1,9 +1,24 @@
>  // SPDX-License-Identifier: GPL-2.0-or-later
>  /*
>   * Copyright (c) Jens Axboe <axboe@kernel.dk>, 2009
> - * http://lkml.org/lkml/2009/4/2/55
> + * Copyright (c) 2021 Petr Vorel <pvorel@suse.cz>
>   */
>  
> +/*\
> + * [DESCRIPTION]
> + * Original reproducer for kernel fix
> + * bf40d3435caf NFS: add support for splice writes
> + * from v2.6.31-rc1.
> + *
> + * http://lkml.org/lkml/2009/4/2/55
> + *
> + * [ALGORITHM]
> + * - create pipe
> + * - fork(), child replace stdin with pipe
> + * - parent write to pipe
> + * - child slice from pipe
> +\*/
> +
>  #define _GNU_SOURCE
>  
>  #include <stdio.h>
> @@ -15,11 +30,27 @@
>  #include "lapi/splice.h"
>  
>  #define SPLICE_SIZE (64*1024)
> +#define DEFAULT_NARG 20000
>  
> -static void splice_test(void)
> +static char *narg;
> +static int num = DEFAULT_NARG;
> +static int pipe_fd[2];
> +
> +static void setup(void)
> +{
> +	if (tst_parse_int(narg, &num, 1, INT_MAX))
> +		tst_brk(TBROK, "invalid number of input '%s'", narg);
                                        ^
					That does not sound english


Maybe "Invalid number of writes" or "Invalid size" something along these lines.

> +}
> +
> +static void do_child(void)
>  {
>  	int fd;
>  
> +	SAFE_CLOSE(pipe_fd[1]);
> +	close(STDIN_FILENO);
> +	SAFE_DUP2(pipe_fd[0], STDIN_FILENO);

dup2() closed the newfd, no need to close STDIN_FILENO here.

> +	TST_CHECKPOINT_WAIT(0);
>  	fd = SAFE_OPEN("splice02-temp", O_WRONLY | O_CREAT | O_TRUNC, 0644);
>  
>  	TEST(splice(STDIN_FILENO, NULL, fd, NULL, SPLICE_SIZE, 0));
> @@ -31,10 +62,40 @@ static void splice_test(void)
>  	}
>  
>  	SAFE_CLOSE(fd);
> +
> +	exit(0);
> +}
> +
> +static void run(void)
> +{
> +	int i;
> +
> +	SAFE_PIPE(pipe_fd);
> +
> +	if (SAFE_FORK())
> +		do_child();
> +
> +	tst_res(TINFO, "writting %d times", num);
> +
> +	for (i = 0; i < num; i++)
> +		SAFE_WRITE(1, pipe_fd[1], "x", 1);
> +
> +	TST_CHECKPOINT_WAKE(0);

I guess that the test will timeout if the -n parameter is greater than
maximal pipe capacity, since the write would end up blocking.

Note that in the original test excess data was simply ignored.

If we wanted to increase the test coverage we could change the child to
splice in a loop with a proper offset until all data are written. After
that no synchronization would be required. Then we could check if we
ended up with a right file size and if the content is correct as well.

> +	tst_reap_children();
> +
> +	SAFE_CLOSE(pipe_fd[0]);
> +	SAFE_CLOSE(pipe_fd[1]);
>  }
>  
>  static struct tst_test test = {
> -	.test_all = splice_test,
> +	.test_all = run,
> +	.setup = setup,
> +	.needs_checkpoints = 1,
>  	.needs_tmpdir = 1,
> +	.forks_child = 1,
>  	.min_kver = "2.6.17",
> +	.options = (struct tst_option[]) {
> +		{"n:", &narg, "-n x     Number of input"},

Here as well.

> +		{}
> +	},
>  };
> -- 
> 2.30.1
>

Hi Cyril,

thanks for your review!

> I do wonder if this should be replaced with something that includes a
> shell pipe instead. It has been selected here to make sure we pass the
> command line correctly to a shell interpreter.

> Maybe something as:

> shell_test01 echo "SUCCESS" | cat

I guess you mean to add another test to cover shell pipe.
Makes sense to me, but I'd wrap it to a test file, e.g. something like:

cat shell01.sh
#!/bin/sh
TST_TESTFUNC=do_test
TST_NEEDS_CMDS="cat"
. tst_test.sh

do_test()
{
	EXPECT_PASS [ "$(echo 'SUCCESS' | cat)" = "SUCCESS" ]
}

tst_run

> > +++ b/runtest/syscalls
> >  splice01 splice01
> > -splice02 seq 1 20000 | splice02
> > +splice02 splice02 -n 20000

> Don't we default to 20000 in the test anyway? What is the point of
> passing the default value here?
+1

...
> > +static void setup(void)
> > +{
> > +	if (tst_parse_int(narg, &num, 1, INT_MAX))
> > +		tst_brk(TBROK, "invalid number of input '%s'", narg);
>                                         ^
> 					That does not sound english


> Maybe "Invalid number of writes" or "Invalid size" something along these lines.
+1 (before it was invalid number of input lines, but then I removed \n).

...
> > +	SAFE_CLOSE(pipe_fd[1]);
> > +	close(STDIN_FILENO);
> > +	SAFE_DUP2(pipe_fd[0], STDIN_FILENO);

> dup2() closed the newfd, no need to close STDIN_FILENO here.
+1.

...
> > +static void run(void)
> > +{
> > +	int i;
> > +
> > +	SAFE_PIPE(pipe_fd);
> > +
> > +	if (SAFE_FORK())
> > +		do_child();
> > +
> > +	tst_res(TINFO, "writting %d times", num);
> > +
> > +	for (i = 0; i < num; i++)
> > +		SAFE_WRITE(1, pipe_fd[1], "x", 1);
> > +
> > +	TST_CHECKPOINT_WAKE(0);

> I guess that the test will timeout if the -n parameter is greater than
> maximal pipe capacity, since the write would end up blocking.
Yes. I could use fcntl(pipe_fd[1], F_GETPIPE_SZ)
(16 pages, i.e. 65536 on my system).
But with changes you suggest below we don't have to bother about F_GETPIPE_SZ.
And I guess having more for regular test (to block) does not give more test
coverage, right?

> Note that in the original test excess data was simply ignored.
I noticed that as well, considered it as a bug. But probably it was enough
for the original reproducer.

> If we wanted to increase the test coverage we could change the child to
> splice in a loop with a proper offset until all data are written. After
> that no synchronization would be required. Then we could check if we
> ended up with a right file size and if the content is correct as well.
Sounds good, I'll try.

> > +	.options = (struct tst_option[]) {
> > +		{"n:", &narg, "-n x     Number of input"},

> Here as well.
+1

Kind regards,
Petr

Hi!
> > I do wonder if this should be replaced with something that includes a
> > shell pipe instead. It has been selected here to make sure we pass the
> > command line correctly to a shell interpreter.
> 
> > Maybe something as:
> 
> > shell_test01 echo "SUCCESS" | cat
> 
> I guess you mean to add another test to cover shell pipe.
> Makes sense to me, but I'd wrap it to a test file, e.g. something like:
> 
> cat shell01.sh
> #!/bin/sh
> TST_TESTFUNC=do_test
> TST_NEEDS_CMDS="cat"
> . tst_test.sh
> 
> do_test()
> {
> 	EXPECT_PASS [ "$(echo 'SUCCESS' | cat)" = "SUCCESS" ]
> }
> 
> tst_run

That would not work, the pipe is supposed to be in the runtest file.

Hi Cyril,

> Hi!
> > > I do wonder if this should be replaced with something that includes a
> > > shell pipe instead. It has been selected here to make sure we pass the
> > > command line correctly to a shell interpreter.

> > > Maybe something as:

> > > shell_test01 echo "SUCCESS" | cat

> > I guess you mean to add another test to cover shell pipe.
> > Makes sense to me, but I'd wrap it to a test file, e.g. something like:

> > cat shell01.sh
> > #!/bin/sh
> > TST_TESTFUNC=do_test
> > TST_NEEDS_CMDS="cat"
> > . tst_test.sh

> > do_test()
> > {
> > 	EXPECT_PASS [ "$(echo 'SUCCESS' | cat)" = "SUCCESS" ]
> > }

> > tst_run

> That would not work, the pipe is supposed to be in the runtest file.
Do you want to test that runtest is working with pipe?
I considered anything but shell script with getopt parameters a bit strange and
thought it'd be removed in new shell runner. But obviously you want to keep it.

But in case of failure script don't detect it. e.g.:

echo "SUCCESS" | cat /asdf
cat: /asdf: No such file or directory

=> there is no TFAIL/TBROK/TCONF. Not sure if all users check exit status (which
they should now, because that is the only common thing so far).

Kind regards,
Petr

Hi!
> > That would not work, the pipe is supposed to be in the runtest file.
> Do you want to test that runtest is working with pipe?

Yes, that was the original idea behind adding the splice test there.

> I considered anything but shell script with getopt parameters a bit strange and
> thought it'd be removed in new shell runner. But obviously you want to keep it.

We will have to keep the functionality for the time being.

> But in case of failure script don't detect it. e.g.:
> 
> echo "SUCCESS" | cat /asdf
> cat: /asdf: No such file or directory
> 
> => there is no TFAIL/TBROK/TCONF. Not sure if all users check exit status (which
> they should now, because that is the only common thing so far).

Well yes, I guess that we can do something as:

echo "SUCCESS" | grep -q "SUCCESS"

Which at least returns 1 if the grep fails.

> Hi!
> > > That would not work, the pipe is supposed to be in the runtest file.
> > Do you want to test that runtest is working with pipe?

> Yes, that was the original idea behind adding the splice test there.

> > I considered anything but shell script with getopt parameters a bit strange and
> > thought it'd be removed in new shell runner. But obviously you want to keep it.

> We will have to keep the functionality for the time being.

> > But in case of failure script don't detect it. e.g.:

> > echo "SUCCESS" | cat /asdf
> > cat: /asdf: No such file or directory

> > => there is no TFAIL/TBROK/TCONF. Not sure if all users check exit status (which
> > they should now, because that is the only common thing so far).

> Well yes, I guess that we can do something as:

> echo "SUCCESS" | grep -q "SUCCESS"

> Which at least returns 1 if the grep fails.
Yep, that's better than using cat.

Although, IMHO it should be possible to do something like:

shell_test01 echo "SUCCESS" | shell_pipe01.sh

cat shell_pipe01.sh
...
do_test()
{
	tst_res TINFO "expecting SUCCESS string passed from stdin"

	read line
	EXPECT_PASS [ "$line" = "SUCCESS" ]
}

We'd use standard LTP interface with tst_test.sh. WDYT?

Kind regards,
Petr

Hi!
> > Well yes, I guess that we can do something as:
> 
> > echo "SUCCESS" | grep -q "SUCCESS"
> 
> > Which at least returns 1 if the grep fails.
> Yep, that's better than using cat.
> 
> Although, IMHO it should be possible to do something like:
> 
> shell_test01 echo "SUCCESS" | shell_pipe01.sh
> 
> cat shell_pipe01.sh
> ...
> do_test()
> {
> 	tst_res TINFO "expecting SUCCESS string passed from stdin"
> 
> 	read line
> 	EXPECT_PASS [ "$line" = "SUCCESS" ]
> }
> 
> We'd use standard LTP interface with tst_test.sh. WDYT?

Well we have to figure out where to put the dummy test, but other than
that no complaints. Also please put that change in a separate commit.

> > We'd use standard LTP interface with tst_test.sh. WDYT?

> Well we have to figure out where to put the dummy test, but other than
> that no complaints. Also please put that change in a separate commit.
Sure! It will be in v2.
Place: it could be in new shell directory: either in testcases/misc/shell/ or testcases/shell/.

Kind regards,
Petr