| Message ID | 20220616162245.6225-1-donald.hunter@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [liburing] Fix incorrect close in test for multishot accept | expand |
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;
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.
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;
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(-)