public inbox for [email protected]
 help / color / mirror / Atom feed
* [PATCH liburing] Fix incorrect close in test for multishot accept
@ 2022-06-16 16:22 Donald Hunter
  2022-06-17  8:28 ` Hao Xu
  0 siblings, 1 reply; 3+ messages in thread
From: Donald Hunter @ 2022-06-16 16:22 UTC (permalink / raw)
  To: axboe; +Cc: io-uring, Donald Hunter

This fixes a bug in accept_conn handling in the accept tests that caused it
to incorrectly skip the multishot tests and also lose the warning message
to a closed stdout. This can be seen in the strace output below.

close(1)                                = 0
io_uring_setup(32, { ...
...
write(1, "Fixed Multishot Accept not suppo"..., 47) = -1 EINVAL

Unfortunately this exposes a a bug with gcc -O2 where multishot_mask logic
gets optimized incorrectly and "Fixed Multishot Accept misses events" is
wrongly reported. I am investigating this separately.

Signed-off-by: Donald Hunter <[email protected]>
---
 test/accept.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/test/accept.c b/test/accept.c
index 7bc6226..fb87a1d 100644
--- a/test/accept.c
+++ b/test/accept.c
@@ -103,7 +103,7 @@ static void queue_accept_conn(struct io_uring *ring, int fd,
 	}
 }
 
-static int accept_conn(struct io_uring *ring, int fixed_idx)
+static int accept_conn(struct io_uring *ring, int fixed_idx, bool multishot)
 {
 	struct io_uring_cqe *cqe;
 	int ret;
@@ -115,8 +115,10 @@ static int accept_conn(struct io_uring *ring, int fixed_idx)
 
 	if (fixed_idx >= 0) {
 		if (ret > 0) {
-			close(ret);
-			return -EINVAL;
+			if (!multishot) {
+				close(ret);
+				return -EINVAL;
+			}
 		} else if (!ret) {
 			ret = fixed_idx;
 		}
@@ -208,7 +210,7 @@ static int test_loop(struct io_uring *ring,
 		queue_accept_conn(ring, recv_s0, args);
 
 	for (i = 0; i < MAX_FDS; i++) {
-		s_fd[i] = accept_conn(ring, args.fixed ? 0 : -1);
+		s_fd[i] = accept_conn(ring, args.fixed ? 0 : -1, multishot);
 		if (s_fd[i] == -EINVAL) {
 			if (args.accept_should_error)
 				goto out;
-- 
2.36.1


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

* Re: [PATCH liburing] Fix incorrect close in test for multishot accept
  2022-06-16 16:22 [PATCH liburing] Fix incorrect close in test for multishot accept Donald Hunter
@ 2022-06-17  8:28 ` Hao Xu
  2022-06-17 10:15   ` Donald Hunter
  0 siblings, 1 reply; 3+ messages in thread
From: Hao Xu @ 2022-06-17  8:28 UTC (permalink / raw)
  To: Donald Hunter, axboe; +Cc: io-uring

On 6/17/22 00:22, Donald Hunter wrote:
> This fixes a bug in accept_conn handling in the accept tests that caused it
> to incorrectly skip the multishot tests and also lose the warning message
> to a closed stdout. This can be seen in the strace output below.
> 
> close(1)                                = 0
> io_uring_setup(32, { ...
> ...
> write(1, "Fixed Multishot Accept not suppo"..., 47) = -1 EINVAL
> 
> Unfortunately this exposes a a bug with gcc -O2 where multishot_mask logic
> gets optimized incorrectly and "Fixed Multishot Accept misses events" is
> wrongly reported. I am investigating this separately.
> 

Super thanks, Donald. you are right, we skipped the fixed multishot test
by mistake, the exposed issue after your fix is caused by
multishot_mask |= (1 << (s_fd[i] - 1))
which should be
multishot_mask |= (1U << s_fd[i])

Would you mind me to take this one to my patch series which is to fix
this and do some cleaning?

> Signed-off-by: Donald Hunter <[email protected]>
> ---
>   test/accept.c | 10 ++++++----
>   1 file changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/test/accept.c b/test/accept.c
> index 7bc6226..fb87a1d 100644
> --- a/test/accept.c
> +++ b/test/accept.c
> @@ -103,7 +103,7 @@ static void queue_accept_conn(struct io_uring *ring, int fd,
>   	}
>   }
>   
> -static int accept_conn(struct io_uring *ring, int fixed_idx)
> +static int accept_conn(struct io_uring *ring, int fixed_idx, bool multishot)
>   {
>   	struct io_uring_cqe *cqe;
>   	int ret;
> @@ -115,8 +115,10 @@ static int accept_conn(struct io_uring *ring, int fixed_idx)
>   
>   	if (fixed_idx >= 0) {
>   		if (ret > 0) {
> -			close(ret);
> -			return -EINVAL;
> +			if (!multishot) {
> +				close(ret);
> +				return -EINVAL;
> +			}
>   		} else if (!ret) {
>   			ret = fixed_idx;
>   		}
> @@ -208,7 +210,7 @@ static int test_loop(struct io_uring *ring,
>   		queue_accept_conn(ring, recv_s0, args);
>   
>   	for (i = 0; i < MAX_FDS; i++) {
> -		s_fd[i] = accept_conn(ring, args.fixed ? 0 : -1);
> +		s_fd[i] = accept_conn(ring, args.fixed ? 0 : -1, multishot);
>   		if (s_fd[i] == -EINVAL) {
>   			if (args.accept_should_error)
>   				goto out;


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

* Re: [PATCH liburing] Fix incorrect close in test for multishot accept
  2022-06-17  8:28 ` Hao Xu
@ 2022-06-17 10:15   ` Donald Hunter
  0 siblings, 0 replies; 3+ messages in thread
From: Donald Hunter @ 2022-06-17 10:15 UTC (permalink / raw)
  To: Hao Xu; +Cc: axboe, io-uring

Hao Xu <[email protected]> writes:

> Super thanks, Donald. you are right, we skipped the fixed multishot test
> by mistake, the exposed issue after your fix is caused by
> multishot_mask |= (1 << (s_fd[i] - 1))
> which should be
> multishot_mask |= (1U << s_fd[i])

I can confirm this fixes the exposed issue.

> Would you mind me to take this one to my patch series which is to fix
> this and do some cleaning?

Please do.

Thanks,
Donald.

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

end of thread, other threads:[~2022-06-17 10:15 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2022-06-16 16:22 [PATCH liburing] Fix incorrect close in test for multishot accept Donald Hunter
2022-06-17  8:28 ` Hao Xu
2022-06-17 10:15   ` Donald Hunter

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