public inbox for ltp@lists.linux.it
 help / color / mirror / Atom feed
* [LTP] [PATCH 1/2] syscalls/epoll_*: allow for epoll_wait returning a subset of events
@ 2017-07-15  1:37 Steve Muckle
  2017-07-15  1:37 ` [LTP] [PATCH 2/2] syscalls/epoll_wait01: cleanup data structure usage Steve Muckle
  2017-07-27 15:20 ` [LTP] [PATCH 1/2] syscalls/epoll_*: allow for epoll_wait returning a subset of events Cyril Hrubis
  0 siblings, 2 replies; 4+ messages in thread
From: Steve Muckle @ 2017-07-15  1:37 UTC (permalink / raw)
  To: ltp

The epoll_wait syscall is not guaranteed to return all pending events -
a subset may be returned. Modify the epoll_wait01 and epoll_ctl01
testcases to allow for that.

CC: Greg KH <gregkh@linuxfoundation.org>
Signed-off-by: Steve Muckle <smuckle.linux@gmail.com>
---
 testcases/kernel/syscalls/epoll_ctl/epoll_ctl01.c  | 67 +++++++++++++------
 .../kernel/syscalls/epoll_wait/epoll_wait01.c      | 75 ++++++++++++++--------
 2 files changed, 97 insertions(+), 45 deletions(-)

diff --git a/testcases/kernel/syscalls/epoll_ctl/epoll_ctl01.c b/testcases/kernel/syscalls/epoll_ctl/epoll_ctl01.c
index 0fa1ab25f..c68b39471 100644
--- a/testcases/kernel/syscalls/epoll_ctl/epoll_ctl01.c
+++ b/testcases/kernel/syscalls/epoll_ctl/epoll_ctl01.c
@@ -85,7 +85,7 @@ static int has_event(struct epoll_event *epvs, int len,
 static void check_epoll_ctl(int opt, int exp_num)
 {
 	int res;
-
+	unsigned int events;
 	char write_buf[] = "test";
 	char read_buf[sizeof(write_buf)];
 
@@ -94,32 +94,59 @@ static void check_epoll_ctl(int opt, int exp_num)
 		{.events = 0, .data.fd = 0}
 	};
 
+	events = EPOLLIN;
+	if (exp_num == 2)
+		events |= EPOLLOUT;
+
 	SAFE_WRITE(1, fd[1], write_buf, sizeof(write_buf));
 
-	res = epoll_wait(epfd, res_evs, 2, -1);
-	if (res == -1)
-		tst_brk(TBROK | TERRNO, "epoll_wait() fails");
+	while (events) {
+		int events_returned = 0;
 
-	if (res != exp_num) {
-		tst_res(TFAIL, "epoll_wait() returns %i, expected %i",
-			res, exp_num);
-		goto end;
-	}
+		bzero(res_evs, sizeof(res_evs));
+		res = epoll_wait(epfd, res_evs, 2, -1);
+		if (res == -1)
+			tst_brk(TBROK | TERRNO, "epoll_wait() fails");
 
-	if (exp_num == 1) {
-		if (!has_event(res_evs, 2, fd[0], EPOLLIN) ||
-		    !has_event(res_evs, 2, 0, 0)) {
-			tst_res(TFAIL, "epoll_wait() fails to "
-				"get expected fd and event");
+		if (res == 0) {
+			tst_res(TFAIL, "epoll_wait() returns 0, expected %s",
+				events == (EPOLLIN|EPOLLOUT) ? "1 or 2" : "1");
 			goto end;
 		}
-	}
 
-	if (exp_num == 2) {
-		if (!has_event(res_evs, 2, fd[0], EPOLLIN) ||
-		    !has_event(res_evs, 2, fd[1], EPOLLOUT)) {
-			tst_res(TFAIL, "epoll_wait() fails to "
-				"get expected fd and event");
+		if (res == 1 && (res_evs[1].data.fd || res_evs[1].events)) {
+			tst_res(TFAIL,
+				"epoll_wait() returns 1 with 2 events");
+			goto end;
+		}
+
+		if (has_event(res_evs, 2, fd[0], EPOLLIN)) {
+			if (events & EPOLLIN) {
+				events &= ~EPOLLIN;
+				events_returned++;
+			} else {
+				tst_res(TFAIL, "epoll_wait() returned %d "
+					"and EPOLLIN %x twice", fd[0],
+					EPOLLIN);
+				goto end;
+			}
+		}
+
+		if (has_event(res_evs, 2, fd[1], EPOLLOUT)) {
+			if (events & EPOLLOUT) {
+				events &= ~EPOLLOUT;
+				events_returned++;
+			} else {
+				tst_res(TFAIL, "epoll_wait() returned %d "
+					"and EPOLLOUT %x twice", fd[1],
+					EPOLLOUT);
+				goto end;
+			}
+		}
+
+		if (res != events_returned) {
+			tst_res(TFAIL, "epoll_wait() returned %d, expected %d",
+				res, events_returned);
 			goto end;
 		}
 	}
diff --git a/testcases/kernel/syscalls/epoll_wait/epoll_wait01.c b/testcases/kernel/syscalls/epoll_wait/epoll_wait01.c
index c8c55720f..1ea1ca2ec 100644
--- a/testcases/kernel/syscalls/epoll_wait/epoll_wait01.c
+++ b/testcases/kernel/syscalls/epoll_wait/epoll_wait01.c
@@ -200,34 +200,59 @@ static void verify_epollio(void)
 {
 	char write_buf[] = "Testing";
 	char read_buf[sizeof(write_buf)];
+	uint32_t events = EPOLLIN | EPOLLOUT;
 
 	SAFE_WRITE(cleanup, 1, fds[1], write_buf, sizeof(write_buf));
 
-	TEST(epoll_wait(epfd, epevs, 3, -1));
-
-	if (TEST_RETURN == -1) {
-		tst_resm(TFAIL | TTERRNO, "epoll_wait() epollio failed");
-		goto end;
-	}
-
-	if (TEST_RETURN != 2) {
-		tst_resm(TFAIL, "epoll_wait() returned %li, expected 2",
-			 TEST_RETURN);
-		goto end;
-	}
-
-	if (!has_event(epevs, 2, fds[0], EPOLLIN)) {
-		dump_epevs(epevs, 2);
-		tst_resm(TFAIL, "epoll_wait() expected %d and EPOLLIN %x",
-			 fds[0], EPOLLIN);
-		goto end;
-	}
-
-	if (!has_event(epevs, 2, fds[1], EPOLLOUT)) {
-		dump_epevs(epevs, 2);
-		tst_resm(TFAIL, "epoll_wait() expected %d and EPOLLOUT %x",
-			 fds[1], EPOLLOUT);
-		goto end;
+	while (events) {
+		int events_returned = 0;
+
+		TEST(epoll_wait(epfd, epevs, 3, -1));
+
+		if (TEST_RETURN == -1) {
+			tst_resm(TFAIL | TTERRNO, "epoll_wait() epollio failed");
+			goto end;
+		}
+
+		if (TEST_RETURN == 0) {
+			tst_resm(TFAIL, "epoll_wait() returned 0, expected %s",
+				 events == (EPOLLIN|EPOLLOUT) ? "1 or 2" : "1");
+			goto end;
+		}
+
+		if (has_event(epevs, 2, fds[0], EPOLLIN)) {
+			if (events & EPOLLIN) {
+				events &= ~EPOLLIN;
+				events_returned++;
+			} else {
+				dump_epevs(epevs, 2);
+				tst_resm(TFAIL, "epoll_wait() returned %d "
+					 "and EPOLLIN %x twice", fds[0],
+					 EPOLLIN);
+				goto end;
+			}
+		}
+
+		if (has_event(epevs, 2, fds[1], EPOLLOUT)) {
+			if (events & EPOLLOUT) {
+				events &= ~EPOLLOUT;
+				events_returned++;
+			} else {
+				dump_epevs(epevs, 2);
+				tst_resm(TFAIL, "epoll_wait() returned %d "
+					 "and EPOLLOUT %x twice", fds[1],
+					 EPOLLOUT);
+				goto end;
+			}
+		}
+
+		if (TEST_RETURN != events_returned) {
+			dump_epevs(epevs, 2);
+			tst_resm(TFAIL,
+				 "epoll_wait() returned %li, expected %d",
+				 TEST_RETURN, events_returned);
+			goto end;
+		}
 	}
 
 	tst_resm(TPASS, "epoll_wait() epollio");
-- 
2.13.2.932.g7449e964c-goog


^ permalink raw reply related	[flat|nested] 4+ messages in thread

* [LTP] [PATCH 2/2] syscalls/epoll_wait01: cleanup data structure usage
  2017-07-15  1:37 [LTP] [PATCH 1/2] syscalls/epoll_*: allow for epoll_wait returning a subset of events Steve Muckle
@ 2017-07-15  1:37 ` Steve Muckle
  2017-07-27 15:20 ` [LTP] [PATCH 1/2] syscalls/epoll_*: allow for epoll_wait returning a subset of events Cyril Hrubis
  1 sibling, 0 replies; 4+ messages in thread
From: Steve Muckle @ 2017-07-15  1:37 UTC (permalink / raw)
  To: ltp

The fds2 data structure is not used during the test, remove it.
Ensure the event structure that epoll_wait() writes into is cleared
prior to invoking the call so that the returned data is confidently
known.

Signed-off-by: Steve Muckle <smuckle.linux@gmail.com>
---
 .../kernel/syscalls/epoll_wait/epoll_wait01.c      | 58 +++++++++++-----------
 1 file changed, 29 insertions(+), 29 deletions(-)

diff --git a/testcases/kernel/syscalls/epoll_wait/epoll_wait01.c b/testcases/kernel/syscalls/epoll_wait/epoll_wait01.c
index 1ea1ca2ec..5aa34d5df 100644
--- a/testcases/kernel/syscalls/epoll_wait/epoll_wait01.c
+++ b/testcases/kernel/syscalls/epoll_wait/epoll_wait01.c
@@ -34,11 +34,10 @@
 char *TCID = "epoll_wait01";
 int TST_TOTAL = 3;
 
-static int write_size, epfd, fds[2], fds2[2];
-static struct epoll_event epevs[3] = {
+static int write_size, epfd, fds[2];
+static struct epoll_event epevs[2] = {
 	{.events = EPOLLIN},
 	{.events = EPOLLOUT},
-	{.events = EPOLLIN}
 };
 
 static void setup(void);
@@ -77,7 +76,6 @@ static void setup(void)
 	TEST_PAUSE;
 
 	SAFE_PIPE(NULL, fds);
-	SAFE_PIPE(cleanup, fds2);
 
 	write_size = get_writesize();
 
@@ -89,11 +87,9 @@ static void setup(void)
 
 	epevs[0].data.fd = fds[0];
 	epevs[1].data.fd = fds[1];
-	epevs[2].data.fd = fds2[0];
 
 	if (epoll_ctl(epfd, EPOLL_CTL_ADD, fds[0], &epevs[0]) ||
-	    epoll_ctl(epfd, EPOLL_CTL_ADD, fds[1], &epevs[1]) ||
-	    epoll_ctl(epfd, EPOLL_CTL_ADD, fds2[0], &epevs[2])) {
+	    epoll_ctl(epfd, EPOLL_CTL_ADD, fds[1], &epevs[1])) {
 		tst_brkm(TBROK | TERRNO, cleanup,
 			 "failed to register epoll target");
 	}
@@ -128,7 +124,9 @@ static int get_writesize(void)
 
 static void verify_epollout(void)
 {
-	TEST(epoll_wait(epfd, epevs, 3, -1));
+	struct epoll_event ret_evs = {.events = 0, .data.fd = 0};
+
+	TEST(epoll_wait(epfd, &ret_evs, 1, -1));
 
 	if (TEST_RETURN == -1) {
 		tst_resm(TFAIL | TTERRNO, "epoll_wait() epollout failed");
@@ -141,15 +139,15 @@ static void verify_epollout(void)
 		return;
 	}
 
-	if (epevs[0].data.fd != fds[1]) {
+	if (ret_evs.data.fd != fds[1]) {
 		tst_resm(TFAIL, "epoll.data.fd %i, expected %i",
-			 epevs[0].data.fd, fds[1]);
+			 ret_evs.data.fd, fds[1]);
 		return;
 	}
 
-	if (epevs[0].events != EPOLLOUT) {
+	if (ret_evs.events != EPOLLOUT) {
 		tst_resm(TFAIL, "epoll.events %x, expected EPOLLOUT %x",
-			 epevs[0].events, EPOLLOUT);
+			 ret_evs.events, EPOLLOUT);
 		return;
 	}
 
@@ -160,12 +158,13 @@ static void verify_epollin(void)
 {
 	char write_buf[write_size];
 	char read_buf[sizeof(write_buf)];
+	struct epoll_event ret_evs = {.events = 0, .data.fd = 0};
 
 	memset(write_buf, 'a', sizeof(write_buf));
 
 	SAFE_WRITE(cleanup, 1, fds[1], write_buf, sizeof(write_buf));
 
-	TEST(epoll_wait(epfd, epevs, 3, -1));
+	TEST(epoll_wait(epfd, &ret_evs, 1, -1));
 
 	if (TEST_RETURN == -1) {
 		tst_resm(TFAIL | TTERRNO, "epoll_wait() epollin failed");
@@ -178,15 +177,15 @@ static void verify_epollin(void)
 		goto end;
 	}
 
-	if (epevs[0].data.fd != fds[0]) {
+	if (ret_evs.data.fd != fds[0]) {
 		tst_resm(TFAIL, "epoll.data.fd %i, expected %i",
-			 epevs[0].data.fd, fds[0]);
+			 ret_evs.data.fd, fds[0]);
 		goto end;
 	}
 
-	if (epevs[0].events != EPOLLIN) {
+	if (ret_evs.events != EPOLLIN) {
 		tst_resm(TFAIL, "epoll.events %x, expected EPOLLIN %x",
-			 epevs[0].events, EPOLLIN);
+			 ret_evs.events, EPOLLIN);
 		goto end;
 	}
 
@@ -201,13 +200,15 @@ static void verify_epollio(void)
 	char write_buf[] = "Testing";
 	char read_buf[sizeof(write_buf)];
 	uint32_t events = EPOLLIN | EPOLLOUT;
+	struct epoll_event ret_evs[2];
 
 	SAFE_WRITE(cleanup, 1, fds[1], write_buf, sizeof(write_buf));
 
 	while (events) {
 		int events_returned = 0;
 
-		TEST(epoll_wait(epfd, epevs, 3, -1));
+		bzero(ret_evs, sizeof(ret_evs));
+		TEST(epoll_wait(epfd, ret_evs, 2, -1));
 
 		if (TEST_RETURN == -1) {
 			tst_resm(TFAIL | TTERRNO, "epoll_wait() epollio failed");
@@ -220,12 +221,17 @@ static void verify_epollio(void)
 			goto end;
 		}
 
-		if (has_event(epevs, 2, fds[0], EPOLLIN)) {
+		if (TEST_RETURN == 1 && (ret_evs[1].data.fd || ret_evs[1].events)) {
+			tst_resm(TFAIL, "epoll_wait() returns 1 with 2 events");
+			goto end;
+		}
+
+		if (has_event(ret_evs, 2, fds[0], EPOLLIN)) {
 			if (events & EPOLLIN) {
 				events &= ~EPOLLIN;
 				events_returned++;
 			} else {
-				dump_epevs(epevs, 2);
+				dump_epevs(ret_evs, 2);
 				tst_resm(TFAIL, "epoll_wait() returned %d "
 					 "and EPOLLIN %x twice", fds[0],
 					 EPOLLIN);
@@ -233,12 +239,12 @@ static void verify_epollio(void)
 			}
 		}
 
-		if (has_event(epevs, 2, fds[1], EPOLLOUT)) {
+		if (has_event(ret_evs, 2, fds[1], EPOLLOUT)) {
 			if (events & EPOLLOUT) {
 				events &= ~EPOLLOUT;
 				events_returned++;
 			} else {
-				dump_epevs(epevs, 2);
+				dump_epevs(ret_evs, 2);
 				tst_resm(TFAIL, "epoll_wait() returned %d "
 					 "and EPOLLOUT %x twice", fds[1],
 					 EPOLLOUT);
@@ -247,7 +253,7 @@ static void verify_epollio(void)
 		}
 
 		if (TEST_RETURN != events_returned) {
-			dump_epevs(epevs, 2);
+			dump_epevs(ret_evs, 2);
 			tst_resm(TFAIL,
 				 "epoll_wait() returned %li, expected %d",
 				 TEST_RETURN, events_returned);
@@ -294,10 +300,4 @@ static void cleanup(void)
 
 	if (close(fds[1]))
 		tst_resm(TWARN | TERRNO, "failed to close fds[1]");
-
-	if (fds2[0] > 0 && close(fds2[0]))
-		tst_resm(TWARN | TERRNO, "failed to close fds2[0]");
-
-	if (fds2[1] > 0 && close(fds2[1]))
-		tst_resm(TWARN | TERRNO, "failed to close fds2[1]");
 }
-- 
2.13.2.932.g7449e964c-goog


^ permalink raw reply related	[flat|nested] 4+ messages in thread

* [LTP] [PATCH 1/2] syscalls/epoll_*: allow for epoll_wait returning a subset of events
  2017-07-15  1:37 [LTP] [PATCH 1/2] syscalls/epoll_*: allow for epoll_wait returning a subset of events Steve Muckle
  2017-07-15  1:37 ` [LTP] [PATCH 2/2] syscalls/epoll_wait01: cleanup data structure usage Steve Muckle
@ 2017-07-27 15:20 ` Cyril Hrubis
  2017-07-27 19:06   ` Steve Muckle
  1 sibling, 1 reply; 4+ messages in thread
From: Cyril Hrubis @ 2017-07-27 15:20 UTC (permalink / raw)
  To: ltp

Hi!
>  static void check_epoll_ctl(int opt, int exp_num)
>  {
>  	int res;
> -
> +	unsigned int events;
>  	char write_buf[] = "test";
>  	char read_buf[sizeof(write_buf)];
>  
> @@ -94,32 +94,59 @@ static void check_epoll_ctl(int opt, int exp_num)
>  		{.events = 0, .data.fd = 0}
>  	};
>  
> +	events = EPOLLIN;
> +	if (exp_num == 2)
> +		events |= EPOLLOUT;
> +
>  	SAFE_WRITE(1, fd[1], write_buf, sizeof(write_buf));
>  
> -	res = epoll_wait(epfd, res_evs, 2, -1);
> -	if (res == -1)
> -		tst_brk(TBROK | TERRNO, "epoll_wait() fails");
> +	while (events) {
> +		int events_returned = 0;
>  
> -	if (res != exp_num) {
> -		tst_res(TFAIL, "epoll_wait() returns %i, expected %i",
> -			res, exp_num);
> -		goto end;
> -	}
> +		bzero(res_evs, sizeof(res_evs));

We should remove the initialization of the res_evs array since we are
clearing it here.

> +		res = epoll_wait(epfd, res_evs, 2, -1);
> +		if (res == -1)
> +			tst_brk(TBROK | TERRNO, "epoll_wait() fails");
>  
> -	if (exp_num == 1) {
> -		if (!has_event(res_evs, 2, fd[0], EPOLLIN) ||
> -		    !has_event(res_evs, 2, 0, 0)) {
> -			tst_res(TFAIL, "epoll_wait() fails to "
> -				"get expected fd and event");
> +		if (res == 0) {
> +			tst_res(TFAIL, "epoll_wait() returns 0, expected %s",
> +				events == (EPOLLIN|EPOLLOUT) ? "1 or 2" : "1");
>  			goto end;
>  		}
> -	}
>  
> -	if (exp_num == 2) {
> -		if (!has_event(res_evs, 2, fd[0], EPOLLIN) ||
> -		    !has_event(res_evs, 2, fd[1], EPOLLOUT)) {
> -			tst_res(TFAIL, "epoll_wait() fails to "
> -				"get expected fd and event");
> +		if (res == 1 && (res_evs[1].data.fd || res_evs[1].events)) {
> +			tst_res(TFAIL,
> +				"epoll_wait() returns 1 with 2 events");
> +			goto end;
> +		}
> +
> +		if (has_event(res_evs, 2, fd[0], EPOLLIN)) {
> +			if (events & EPOLLIN) {
> +				events &= ~EPOLLIN;
> +				events_returned++;
> +			} else {
> +				tst_res(TFAIL, "epoll_wait() returned %d "
> +					"and EPOLLIN %x twice", fd[0],
> +					EPOLLIN);
> +				goto end;
> +			}
> +		}
> +
> +		if (has_event(res_evs, 2, fd[1], EPOLLOUT)) {
> +			if (events & EPOLLOUT) {
> +				events &= ~EPOLLOUT;
> +				events_returned++;
> +			} else {
> +				tst_res(TFAIL, "epoll_wait() returned %d "
> +					"and EPOLLOUT %x twice", fd[1],
> +					EPOLLOUT);
> +				goto end;
> +			}
> +		}
> +
> +		if (res != events_returned) {
> +			tst_res(TFAIL, "epoll_wait() returned %d, expected %d",
> +				res, events_returned);

I find this part a bit confusing. The res is actually number of events
returned from the epoll_wait() while the events_returned is a counter
for events that were matched correctly.

So what abou simplifying the logic a bit and doing:

	while (events) {
		int events_matched = 0;

		res = epoll_wait(...);
		if (res <= 0) {
			tst_res(TFAIL | TERRNO, "epoll_wait() returned %i", res);
			goto end;
		}

		if ((events & EPOLLIN) && has_event(res_evs, 2, fd[0], EPOLLIN)) {
			events_matched++;
			events &= ~EPOLLIN;
		}

		//the same for EPOLLOUT

		if (res != events_matched) {
			tst_res(TFAIL, "Got unexpected events");
			//And possibly dump the res_evs here
			goto end;
		}
	}

>  			goto end;
>  		}
>  	}
> diff --git a/testcases/kernel/syscalls/epoll_wait/epoll_wait01.c b/testcases/kernel/syscalls/epoll_wait/epoll_wait01.c
> index c8c55720f..1ea1ca2ec 100644
> --- a/testcases/kernel/syscalls/epoll_wait/epoll_wait01.c
> +++ b/testcases/kernel/syscalls/epoll_wait/epoll_wait01.c
> @@ -200,34 +200,59 @@ static void verify_epollio(void)
>  {
>  	char write_buf[] = "Testing";
>  	char read_buf[sizeof(write_buf)];
> +	uint32_t events = EPOLLIN | EPOLLOUT;
>  
>  	SAFE_WRITE(cleanup, 1, fds[1], write_buf, sizeof(write_buf));
>  
> -	TEST(epoll_wait(epfd, epevs, 3, -1));
> -
> -	if (TEST_RETURN == -1) {
> -		tst_resm(TFAIL | TTERRNO, "epoll_wait() epollio failed");
> -		goto end;
> -	}
> -
> -	if (TEST_RETURN != 2) {
> -		tst_resm(TFAIL, "epoll_wait() returned %li, expected 2",
> -			 TEST_RETURN);
> -		goto end;
> -	}
> -
> -	if (!has_event(epevs, 2, fds[0], EPOLLIN)) {
> -		dump_epevs(epevs, 2);
> -		tst_resm(TFAIL, "epoll_wait() expected %d and EPOLLIN %x",
> -			 fds[0], EPOLLIN);
> -		goto end;
> -	}
> -
> -	if (!has_event(epevs, 2, fds[1], EPOLLOUT)) {
> -		dump_epevs(epevs, 2);
> -		tst_resm(TFAIL, "epoll_wait() expected %d and EPOLLOUT %x",
> -			 fds[1], EPOLLOUT);
> -		goto end;
> +	while (events) {
> +		int events_returned = 0;
> +
> +		TEST(epoll_wait(epfd, epevs, 3, -1));
> +
> +		if (TEST_RETURN == -1) {
> +			tst_resm(TFAIL | TTERRNO, "epoll_wait() epollio failed");
> +			goto end;
> +		}
> +
> +		if (TEST_RETURN == 0) {
> +			tst_resm(TFAIL, "epoll_wait() returned 0, expected %s",
> +				 events == (EPOLLIN|EPOLLOUT) ? "1 or 2" : "1");
> +			goto end;
> +		}
> +
> +		if (has_event(epevs, 2, fds[0], EPOLLIN)) {
> +			if (events & EPOLLIN) {
> +				events &= ~EPOLLIN;
> +				events_returned++;
> +			} else {
> +				dump_epevs(epevs, 2);
> +				tst_resm(TFAIL, "epoll_wait() returned %d "
> +					 "and EPOLLIN %x twice", fds[0],
> +					 EPOLLIN);
> +				goto end;
> +			}
> +		}
> +
> +		if (has_event(epevs, 2, fds[1], EPOLLOUT)) {
> +			if (events & EPOLLOUT) {
> +				events &= ~EPOLLOUT;
> +				events_returned++;
> +			} else {
> +				dump_epevs(epevs, 2);
> +				tst_resm(TFAIL, "epoll_wait() returned %d "
> +					 "and EPOLLOUT %x twice", fds[1],
> +					 EPOLLOUT);
> +				goto end;
> +			}
> +		}
> +
> +		if (TEST_RETURN != events_returned) {
> +			dump_epevs(epevs, 2);
> +			tst_resm(TFAIL,
> +				 "epoll_wait() returned %li, expected %d",
> +				 TEST_RETURN, events_returned);
> +			goto end;
> +		}
>  	}

Here as well.

-- 
Cyril Hrubis
chrubis@suse.cz

^ permalink raw reply	[flat|nested] 4+ messages in thread

* [LTP] [PATCH 1/2] syscalls/epoll_*: allow for epoll_wait returning a subset of events
  2017-07-27 15:20 ` [LTP] [PATCH 1/2] syscalls/epoll_*: allow for epoll_wait returning a subset of events Cyril Hrubis
@ 2017-07-27 19:06   ` Steve Muckle
  0 siblings, 0 replies; 4+ messages in thread
From: Steve Muckle @ 2017-07-27 19:06 UTC (permalink / raw)
  To: ltp

Hi Cyril,

On 07/27/2017 08:20 AM, Cyril Hrubis wrote:
>> -	}
>> +		bzero(res_evs, sizeof(res_evs));
> 
> We should remove the initialization of the res_evs array since we are
> clearing it here.

Yep missed that... done.

> 
>> +		res = epoll_wait(epfd, res_evs, 2, -1);
>> +		if (res == -1)
>> +			tst_brk(TBROK | TERRNO, "epoll_wait() fails");
>>   
>> -	if (exp_num == 1) {
>> -		if (!has_event(res_evs, 2, fd[0], EPOLLIN) ||
>> -		    !has_event(res_evs, 2, 0, 0)) {
>> -			tst_res(TFAIL, "epoll_wait() fails to "
>> -				"get expected fd and event");
>> +		if (res == 0) {
>> +			tst_res(TFAIL, "epoll_wait() returns 0, expected %s",
>> +				events == (EPOLLIN|EPOLLOUT) ? "1 or 2" : "1");
>>   			goto end;
>>   		}
>> -	}
>>   
>> -	if (exp_num == 2) {
>> -		if (!has_event(res_evs, 2, fd[0], EPOLLIN) ||
>> -		    !has_event(res_evs, 2, fd[1], EPOLLOUT)) {
>> -			tst_res(TFAIL, "epoll_wait() fails to "
>> -				"get expected fd and event");
>> +		if (res == 1 && (res_evs[1].data.fd || res_evs[1].events)) {
>> +			tst_res(TFAIL,
>> +				"epoll_wait() returns 1 with 2 events");
>> +			goto end;
>> +		}
>> +
>> +		if (has_event(res_evs, 2, fd[0], EPOLLIN)) {
>> +			if (events & EPOLLIN) {
>> +				events &= ~EPOLLIN;
>> +				events_returned++;
>> +			} else {
>> +				tst_res(TFAIL, "epoll_wait() returned %d "
>> +					"and EPOLLIN %x twice", fd[0],
>> +					EPOLLIN);
>> +				goto end;
>> +			}
>> +		}
>> +
>> +		if (has_event(res_evs, 2, fd[1], EPOLLOUT)) {
>> +			if (events & EPOLLOUT) {
>> +				events &= ~EPOLLOUT;
>> +				events_returned++;
>> +			} else {
>> +				tst_res(TFAIL, "epoll_wait() returned %d "
>> +					"and EPOLLOUT %x twice", fd[1],
>> +					EPOLLOUT);
>> +				goto end;
>> +			}
>> +		}
>> +
>> +		if (res != events_returned) {
>> +			tst_res(TFAIL, "epoll_wait() returned %d, expected %d",
>> +				res, events_returned);
> 
> I find this part a bit confusing. The res is actually number of events
> returned from the epoll_wait() while the events_returned is a counter
> for events that were matched correctly.

Yeah I could've named things better.

> So what abou simplifying the logic a bit and doing:
> 
> 	while (events) {
> 		int events_matched = 0;
> 
> 		res = epoll_wait(...);
> 		if (res <= 0) {
> 			tst_res(TFAIL | TERRNO, "epoll_wait() returned %i", res);
> 			goto end;
> 		}
> 
> 		if ((events & EPOLLIN) && has_event(res_evs, 2, fd[0], EPOLLIN)) {
> 			events_matched++;
> 			events &= ~EPOLLIN;
> 		}
> 
> 		//the same for EPOLLOUT
> 
> 		if (res != events_matched) {
> 			tst_res(TFAIL, "Got unexpected events");
> 			//And possibly dump the res_evs here
> 			goto end;
> 		}
> 	}

Sure. Though this does reduce the verbosity/complexity of the error 
handling. I doubt it really matters though...

Thanks for the feedback, v2 on its way.

cheers,
Steve

^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2017-07-27 19:06 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-07-15  1:37 [LTP] [PATCH 1/2] syscalls/epoll_*: allow for epoll_wait returning a subset of events Steve Muckle
2017-07-15  1:37 ` [LTP] [PATCH 2/2] syscalls/epoll_wait01: cleanup data structure usage Steve Muckle
2017-07-27 15:20 ` [LTP] [PATCH 1/2] syscalls/epoll_*: allow for epoll_wait returning a subset of events Cyril Hrubis
2017-07-27 19:06   ` Steve Muckle

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox