[LTP] [PATCH 1/3] epoll_wait/epoll_wait01.c: add new testcase

[Guangwen Feng <fenggw-fnst@cn.fujitsu.com>]

Hi!

Thanks for your review!
I will modify these test cases according to your suggestion.

Best Regards,
Guangwen Feng

On 01/29/2016 12:19 AM, Cyril Hrubis wrote:
> Hi!
>> diff --git a/testcases/kernel/syscalls/epoll_wait/epoll_wait01.c b/testcases/kernel/syscalls/epoll_wait/epoll_wait01.c
>> new file mode 100644
>> index 0000000..46058a2
>> --- /dev/null
>> +++ b/testcases/kernel/syscalls/epoll_wait/epoll_wait01.c
>> @@ -0,0 +1,263 @@
>> +/*
>> + * Copyright (c) 2015 Fujitsu Ltd.
>> + * Author: Guangwen Feng <fenggw-fnst@cn.fujitsu.com>
>> + *
>> + * 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.
>> + */
>> +
>> +/*
>> + * Description:
>> + *  Basic test for epoll_wait(2).
>> + *  Check that epoll_wait(2) works for EPOLLOUT and EPOLLIN events
>> + *  on a epoll instance and that struct epoll_event is set correctly.
>> + */
>> +
>> +#include <sys/epoll.h>
>> +#include <unistd.h>
>> +#include <fcntl.h>
>> +#include <poll.h>
>> +#include <errno.h>
>> +
>> +#include "test.h"
>> +#include "safe_macros.h"
>> +#include "lapi/fcntl.h"
>> +
>> +char *TCID = "epoll_wait01";
>> +int TST_TOTAL = 3;
>> +
>> +static int write_size, epfd, fds[2];
>> +static struct epoll_event epevs[2];
>> +
>> +static void setup(void);
>> +static int get_writesize(void);
>> +static void verify_epollout(void);
>> +static void verify_epollin(void);
>> +static void verify_epollio(void);
>> +static void cleanup(void);
>> +
>> +int main(int ac, char **av)
>> +{
>> +	int lc;
>> +
>> +	tst_parse_opts(ac, av, NULL, NULL);
>> +
>> +	setup();
>> +
>> +	for (lc = 0; TEST_LOOPING(lc); lc++) {
>> +		tst_count = 0;
>> +
>> +		verify_epollout();
>> +		verify_epollin();
>> +		verify_epollio();
>> +	}
>> +
>> +	cleanup();
>> +	tst_exit();
>> +}
>> +
>> +static void setup(void)
>> +{
>> +	int pipe_capacity;
>> +
>> +	tst_sig(NOFORK, DEF_HANDLER, cleanup);
>> +
>> +	TEST_PAUSE;
>> +
>> +	SAFE_PIPE(NULL, fds);
>> +
>> +	/* fcntl()'s F_GETPIPE_SZ flag availbility check */
>> +	if ((tst_kvercmp(2, 6, 35)) < 0) {
>> +		write_size = get_writesize();
>> +	} else {
>> +		pipe_capacity = fcntl(fds[1], F_GETPIPE_SZ);
>> +		if (pipe_capacity == -1) {
>> +			tst_brkm(TBROK | TERRNO, cleanup,
>> +				 "failed to get the pipe capacity");
>> +		}
>> +
>> +		write_size = pipe_capacity - getpagesize() + 1;
>> +	}
> 
> I would rather go with runtime detection even for newer kernels. Since
> otherwise we have two different codepaths to test.
> 
>> +	epfd = epoll_create(2);
>> +	if (epfd == -1) {
>> +		tst_brkm(TBROK | TERRNO, cleanup,
>> +			 "failed to create epoll instance");
>> +	}
>> +
>> +	epevs[0].events = EPOLLIN;
>> +	epevs[0].data.fd = fds[0];
>> +	epevs[1].events = EPOLLOUT;
>> +	epevs[1].data.fd = fds[1];
>> +
>> +	if (epoll_ctl(epfd, EPOLL_CTL_ADD, fds[0], &epevs[0]) ||
>> +		epoll_ctl(epfd, EPOLL_CTL_ADD, fds[1], &epevs[1])) {
>> +		tst_brkm(TBROK | TERRNO, cleanup,
>> +			 "failed to register epoll target");
>> +	}
>> +}
>> +
>> +/*
>> + * fcntl(fd, F_GETPIPE_SZ) not supported on kernels which
>> + * version lower than 2.6.35, so use this function instead
>> + */
>> +static int get_writesize(void)
>> +{
>> +	int nfd, write_size;
>> +
>> +	struct pollfd pfd[] = {
>> +		{.fd = fds[0], .events = POLLIN},
>> +		{.fd = fds[1], .events = POLLOUT},
>> +	};
>> +
>> +	write_size = 0;
>> +	do {
>> +		SAFE_WRITE(cleanup, 1, fds[1], "a", 1);
>> +		write_size++;
>> +		nfd = poll(pfd, 2, -1);
>> +		if (nfd == -1)
>> +			tst_brkm(TBROK | TERRNO, cleanup, "poll failed");
>> +	} while (nfd == 2);
> 
> Why don't we write larger chunks and add the return from SAFE_WRITE to
> the write_size? That should work if we pass 0 as len strict, the last
> write should just end up truncated.
> 
>> +	char buf[write_size];
>> +
>> +	SAFE_READ(cleanup, 1, fds[0], buf, write_size);
>> +
>> +	return write_size;
>> +}
>> +
>> +static void verify_epollout(void)
>> +{
>> +	TEST(epoll_wait(epfd, epevs, 2, -1));
>> +
>> +	if (TEST_RETURN == -1) {
>> +		tst_resm(TFAIL | TTERRNO, "epoll_wait() epollout failed");
>> +		return;
>> +	}
>> +
>> +	if (TEST_RETURN != 1) {
>> +		tst_resm(TFAIL, "epoll_wait() failed to get one fd ready");
>                                        ^
> 				       We should add the TEST_RETURN
> 				       value to the output here
> I would go for:
> 		tst_resm(TFAIL, "epoll_wait() returned %i expected 1",
> 		                TEST_RETURN);
> 
> 
>> +		return;
>> +	}
>> +
>> +	if (epevs[0].data.fd != fds[1]) {
>> +		tst_resm(TFAIL, "epoll_wait() failed to set write fd");
> 
> And maybe write the actuall fds here:
> 
> 		tst_resm(TFAIL, "epoll.data.fd %i expected %i", ...);
> 
>> +		return;
>> +	}
>> +
>> +	if (epevs[0].events != EPOLLOUT) {
>> +		tst_resm(TFAIL, "epoll_wait() failed to set EPOLLOUT");
> 
> And perhaps print the value of events as hexadecimal here.
> 
>> +		return;
>> +	}
>> +
>> +	tst_resm(TPASS, "epoll_wait() epollout");
>> +}
>> +
>> +static void verify_epollin(void)
>> +{
>> +	char write_buf[write_size];
>> +	char read_buf[sizeof(write_buf)];
>> +
>> +	memset(write_buf, 'a', sizeof(write_buf));
>> +
>> +	SAFE_WRITE(cleanup, 1, fds[1], write_buf, sizeof(write_buf));
>> +
>> +	TEST(epoll_wait(epfd, epevs, 2, -1));
>> +
>> +	if (TEST_RETURN == -1) {
>> +		tst_resm(TFAIL | TTERRNO, "epoll_wait() epollin failed");
>> +		goto end;
>> +	}
>> +
>> +	if (TEST_RETURN != 1) {
>> +		tst_resm(TFAIL, "epoll_wait() failed to get one fd ready");
>> +		goto end;
>> +	}
>> +
>> +	if (epevs[0].data.fd != fds[0]) {
>> +		tst_resm(TFAIL, "epoll_wait() failed to set read fd");
>> +		goto end;
>> +	}
>> +
>> +	if (epevs[0].events != EPOLLIN) {
>> +		tst_resm(TFAIL, "epoll_wait() failed to set EPOLLIN");
>> +		goto end;
>> +	}
> 
> And I would be more verbose about the values in these messages as well.
> 
>> +	tst_resm(TPASS, "epoll_wait() epollin");
>> +
>> +end:
>> +	SAFE_READ(cleanup, 1, fds[0], read_buf, sizeof(write_buf));
>> +}
>> +
>> +static void verify_epollio(void)
>> +{
>> +	int i, checked0, checked1;
>> +	char write_buf[] = "Testing";
>> +	char read_buf[sizeof(write_buf)];
>> +
>> +	SAFE_WRITE(cleanup, 1, fds[1], write_buf, sizeof(write_buf));
>> +
>> +	TEST(epoll_wait(epfd, epevs, 2, -1));
>                                      ^
> 			       Shouldn't we pass at least 3 here (and
> 			       declare epevs to have three elements as
> 			       well). Since if there were three events
> 			       queued the epoll_wait() would return just
> 			       2 and the test will pass.
> 
>> +
>> +	if (TEST_RETURN == -1) {
>> +		tst_resm(TFAIL | TTERRNO, "epoll_wait() epollio failed");
>> +		goto end;
>> +	}
>> +
>> +	if (TEST_RETURN != 2) {
>> +		tst_resm(TFAIL, "epoll_wait() failed to get two fds ready");
>> +		goto end;
>> +	}
>> +
>> +	checked0 = 0;
>> +	checked1 = 0;
>> +	for (i = 0; i < TEST_RETURN; i++) {
>> +		if (checked0 == 0 && epevs[i].data.fd == fds[0]) {
>> +			if (epevs[i].events != EPOLLIN) {
>> +				tst_resm(TFAIL, "epoll_wait()"
>> +					 "failed to set EPOLLIN");
>> +				goto end;
>> +			}
>> +			checked0 = 1;
>> +		} else if (checked1 == 0 && epevs[i].data.fd == fds[1]) {
>> +			if (epevs[i].events != EPOLLOUT) {
>> +				tst_resm(TFAIL, "epoll_wait()"
>> +					 "failed to set EPOLLOUT");
>> +				goto end;
>> +			}
>> +			checked1 = 1;
>> +		} else {
>> +			tst_resm(TFAIL, "epoll_wait()"
>> +				 "failed to set write or read fd");
>> +			goto end;
>> +		}
>> +	}
> 
> 
> Hmm, this is kind of messy. I would implement this as function that
> checks if the array has event with specific values and would have called
> it twice once with read fd and once with write fd.
> 
> static int has_event(struct epoll_event *events, int events_len,
>                      int fd, uint32_t events)
> {
> 	...
> }
> 
> ...
> 	if (!has_event(epevs, 2, fds[0], EPOLLIN))
> 		tst_resm(TFAIL, ...);
> 
> 	if (!has_event(epevs, 2, fds[1], EPOLLOUT))
> 		tst_resm(TFAIL, ...);
> ...
> 
> 
> And perhaps we can add a function to dump the epevs array with
> tst_resm(TINFO, "") and call it on the array whenever we find that
> something went wrong before we do tst_resm(TFAIL, ...);
> 
>> +	tst_resm(TPASS, "epoll_wait() epollio");
>> +
>> +end:
>> +	SAFE_READ(cleanup, 1, fds[0], read_buf, sizeof(write_buf));
>> +}
>> +
>> +static void cleanup(void)
>> +{
>> +	if (epfd > 0 && close(epfd))
>> +		tst_resm(TWARN | TERRNO, "failed to close epfd");
>> +
>> +	if (close(fds[0]))
>> +		tst_resm(TWARN | TERRNO, "failed to close fds[0]");
>> +
>> +	if (close(fds[1]))
>> +		tst_resm(TWARN | TERRNO, "failed to close fds[1]");
>> +}
> 
> The rest looks good.
>