Re: [PATCH v2 2/2] selftests/nolibc: add testcase for pipe

[Thomas Weißschuh]

On 2023-08-01 14:51:40+0800, Yuan Tan wrote:
> Hi Thomas,
> 
> On Mon, 2023-07-31 at 20:28 +0200, Thomas Weißschuh wrote:
> > On 2023-08-01 02:01:36+0800, Yuan Tan wrote:
> > > Hi Thomas,
> > > On Mon, 2023-07-31 at 17:41 +0200, Thomas Weißschuh wrote:
> > > > On 2023-07-31 20:35:28+0800, Yuan Tan wrote:
> > > > > On Mon, 2023-07-31 at 08:10 +0200, Thomas Weißschuh wrote:
> > > > > > On 2023-07-31 13:51:00+0800, Yuan Tan wrote:
> > > > > > > Add a testcase of pipe that child process sends message to
> > > > > > > parent
> > > > > > > process.
> > > > > > 
> > > > > > Thinking about it some more:
> > > > > > 
> > > > > > What's the advantage of going via a child process?
> > > > > > The pipe should work the same within the same process.
> > > > > > 
> > > > > 
> > > > > The pipe is commonly used for process communication, and I
> > > > > think as
> > > > > a
> > > > > test case it is supposed to cover the most common scenarios.
> > > > 
> > > > The testcase is supposed to cover the code of nolibc.
> > > > It should be the *minimal* amount of code to be reasonable sure
> > > > that
> > > > the
> > > > code in nolibc does the correct thing.
> > > > If pipe() returns a value that behaves like a pipe I see no
> > > > reason to
> > > > doubt it will also survive fork().
> > > > 
> > > > Validating that would mean testing the kernel and not nolibc.
> > > > For the kernel there are different testsuites.
> > > > 
> > > > Less code means less work for everyone involved, now and in the
> > > > future.
> > > > 
> > > 
> > > It's a good point and I never thought about this aspect.
> > > 
> > > I wonder whether the code below is enough?
> > > 
> > > static int test_pipe(void)
> > > {
> > >         int pipefd[2];
> > > 
> > >         if (pipe(pipefd) == -1)
> > >                 return 1;
> > > 
> > >         close(pipefd[0]);
> > >         close(pipefd[1]);
> > > 
> > >         return 0;
> > > }
> > 
> > That is very barebones.
> > 
> > If accidentally a wrong syscall number was used and the used syscall
> > would not take any arguments this test would still succeed.
> > 
> > Keeping the write-read-cycle from the previous revision would test
> > that
> > nicely. Essentially the same code as before but without the fork().
> > 
> > > 
> > > And I forgot to add this line:
> > > 
> > >         CASE_TEST(pipe); EXPECT_SYSZR(1, test_pipe()); break;
> > > 
> > > I will add it in next patch.
> > > 
> > 
> 
> In the situation you described, that is indeed the case.
> 
> Would this be fine?
> 
> static int test_pipe(void)
> {
> 	const char *const msg = "hello, nolibc";
> 	int pipefd[2];
> 	char buf[32];
> 	ssize_t len;
> 
> 	if (pipe(pipefd) == -1)
> 		return 1;
> 
> 	write(pipefd[1], msg, strlen(msg));
> 	close(pipefd[1]);
> 	len = read(pipefd[0], buf, sizeof(buf));
> 	close(pipefd[0]);
> 
> 	if (len != strlen(msg))
> 		return 1;
> 
> 	return !!memcmp(buf, msg, len);
> }

Looks good!

The return value of write() could also be validated but given we
validate the return value from read() it shouldn't make a difference.

(Also the manual manipulation of "buf" is gone that necessitated the
check in v1 of the series)