[PATCH liburing] Fix incorrect close in test for multishot accept

[PATCH liburing] Fix incorrect close in test for multishot accept

[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 <donald.hunter@gmail.com>
---
 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

Re: [PATCH liburing] Fix incorrect close in test for multishot accept

[Hao Xu]

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 <donald.hunter@gmail.com>
> ---
>   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;

Re: [PATCH liburing] Fix incorrect close in test for multishot accept

[Donald Hunter]

Hao Xu <hao.xu@linux.dev> 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.