[RFC PATCH v2 00/11] Fix sock_recv reordering issue (#52401)
(cc/ Dongwan Kim: This is the successor to the patch serie I just replied.) As discussed in https://www.winehq.org/pipermail/wine-devel/2022-January/204710.html, I converted my previous patch serie to use APCs instead of a new select op. "Talk is cheap. Show me the code." -Linus I was, well, the original proposer of the APC approach; now, this patch serie serves as my counterargument to this approach. I tried my best to minimize the complexity introduced by this patch serie; however, the APC approach resulted in net increase in the number of patches: - server: Add a helper function to trigger synchronous completion of I/O via APC_ASYNC_IO. - include: Add a helper function to test if a status indicates deferred completion. - server: Add helpers for synchronous APC delivery via ordinary server call reply. - server: Prevent I/O synchronous completion requests from firing APC interrupts if possible. - ntdll: Make server_select and server_wait accept extra "inline_apc" argument. - ntdll: Don't interrupt sock_recv with an I/O synchronous completion APC. The APC approach *did* enable code reuse of async_recv_proc, albeit with the following drawbacks: - We need to modify async_recv_proc anyway. - Deferring the synchronous I/O to the APC callback makes the synchronous I/O code flow much less obvious. (Implicit call flow: sock_recv -> async_recv_proc -> sock_recv) In the meantime, the following patches are common to both approaches: - ntdll: Don't call try_recv before server call in sock_recv. - server: Replace redundant recv_socket status fields with force_async boolean field. Further notes are placed in each patch. As always, feedbacks welcome. Changelog: - v1 -> v2 - remove autogenerated changes - server/thread.c: dequeue_synchronous_apc: check for handle allocation failure Jinoh Kang (11): server: Allow calling async_handoff() with status code STATUS_ALERTED. server: Add a helper function to trigger synchronous completion of I/O via APC_ASYNC_IO. include: Add a helper function to test if a status indicates deferred completion. server: Attempt to complete I/O request immediately in recv_socket. server: Add helpers for synchronous APC delivery via ordinary server call reply. server: Prevent I/O synchronous completion requests from firing APC interrupts if possible. ntdll: Make server_select and server_wait accept extra "inline_apc" argument. ntdll: Add a helper to process APC and wait for async in one function call. ntdll: Don't interrupt sock_recv with an I/O synchronous completion APC. ntdll: Don't call try_recv before server call in sock_recv. server: Replace redundant recv_socket status fields with force_async boolean field. dlls/ntdll/unix/server.c | 16 ++++++---- dlls/ntdll/unix/socket.c | 33 ++++++++------------- dlls/ntdll/unix/sync.c | 29 ++++++++++++++---- dlls/ntdll/unix/thread.c | 4 +-- dlls/ntdll/unix/unix_private.h | 6 ++-- dlls/ws2_32/tests/sock.c | 8 ++--- include/wine/afd.h | 17 +++++++++++ include/wine/async.h | 46 +++++++++++++++++++++++++++++ server/async.c | 32 +++++++++++++++++++- server/file.h | 1 + server/protocol.def | 11 +++++-- server/sock.c | 54 ++++++++++++++++++++++++++-------- server/thread.c | 49 ++++++++++++++++++++++++++++++ server/thread.h | 3 ++ server/trace.c | 19 ++++++++++++ tools/make_requests | 1 + 16 files changed, 276 insertions(+), 53 deletions(-) create mode 100644 include/wine/async.h -- 2.31.1
If the server detects that an I/O request could be completed immediately (e.g. the socket to read from already has incoming data), it can now return STATUS_ALERTED to allow opportunistic synchronous I/O. The Unix side will then attempt to perform I/O in nonblocking mode and report back the I/O status to the server with signal_wait_async(). If the operation returns e.g. EAGAIN or EWOULDBLOCK, the client can opt to either abandon the request (by specifying an error status) or poll for it in the server as usual (by specifying STATUS_PENDING). Without this mechanism the client cannot safely perform immediately satiable I/O operations synchronously, since it can potentially conflict with other pending I/O operations that have already been queued. Signed-off-by: Jinoh Kang <jinoh.kang.kr(a)gmail.com> --- server/async.c | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-) diff --git a/server/async.c b/server/async.c index 7aef28355f0..aa6d50cde75 100644 --- a/server/async.c +++ b/server/async.c @@ -126,7 +126,8 @@ static void async_satisfied( struct object *obj, struct wait_queue_entry *entry async->direct_result = 0; } - if (async->initial_status == STATUS_PENDING && async->blocking) + if ((async->initial_status == STATUS_PENDING && async->blocking) || + async->initial_status == STATUS_ALERTED) set_wait_status( entry, async->iosb->status ); else set_wait_status( entry, async->initial_status ); @@ -464,6 +465,17 @@ void async_set_result( struct object *obj, unsigned int status, apc_param_t tota { async->terminated = 0; async->alerted = 0; + + /* If the client attempted to complete synchronously and failed, + * then it would have called signal_wait_async() to restart the + * operation in the full asynchronous mode. In this case, we set + * the pending flag so that the completion port notification and + * APC call will be triggered appropriately. Also, the async + * object is currently in signaled state; unset the signaled flag + * if the client wants to block on this async. */ + async->pending = 1; + if (async->blocking) async->signaled = 0; + async_reselect( async ); } else -- 2.31.1
Signed-off-by: Jinoh Kang <jinoh.kang.kr(a)gmail.com> --- Notes: This patch was necessary since we have to reuse APC for synchronous I/O. The previous approach without APC would completely obviate the need for this function. server/async.c | 17 +++++++++++++++++ server/file.h | 1 + 2 files changed, 18 insertions(+) diff --git a/server/async.c b/server/async.c index aa6d50cde75..5d0857f3eec 100644 --- a/server/async.c +++ b/server/async.c @@ -383,6 +383,23 @@ obj_handle_t async_handoff( struct async *async, data_size_t *result, int force_ return async->wait_handle; } +/* Set up synchronous completion of I/O via the APC_ASYNC_IO system APC. + * This function is intended to be called immediately before async_handoff(), + * and expects the last error status to be STATUS_ALERTED. + * + * The information argument can be used to pass extra information to the client + * (e.g. whether the socket is in non-blocking mode). + */ +void async_start_sync_io_request( struct async *async, data_size_t information ) +{ + assert( get_error() == STATUS_ALERTED ); + assert( async->thread == current ); + assert( !async->pending ); + + async->direct_result = 0; /* force APC to fire off */ + async->iosb->result = information; +} + /* complete a request-based async with a pre-allocated buffer */ void async_request_complete( struct async *async, unsigned int status, data_size_t result, data_size_t out_size, void *out_data ) diff --git a/server/file.h b/server/file.h index 1d830cd3d6f..34930b87b5d 100644 --- a/server/file.h +++ b/server/file.h @@ -234,6 +234,7 @@ extern void async_set_initial_status( struct async *async, unsigned int status ) extern void async_wake_obj( struct async *async ); extern int async_waiting( struct async_queue *queue ); extern void async_terminate( struct async *async, unsigned int status ); +extern void async_start_sync_io_request( struct async *async, data_size_t information ); extern void async_request_complete( struct async *async, unsigned int status, data_size_t result, data_size_t out_size, void *out_data ); extern void async_request_complete_alloc( struct async *async, unsigned int status, data_size_t result, -- 2.31.1
On 1/22/22 08:36, Jinoh Kang wrote:
diff --git a/server/async.c b/server/async.c index aa6d50cde75..5d0857f3eec 100644 --- a/server/async.c +++ b/server/async.c @@ -383,6 +383,23 @@ obj_handle_t async_handoff( struct async *async, data_size_t *result, int force_ return async->wait_handle; }
+/* Set up synchronous completion of I/O via the APC_ASYNC_IO system APC. + * This function is intended to be called immediately before async_handoff(), + * and expects the last error status to be STATUS_ALERTED. + * + * The information argument can be used to pass extra information to the client + * (e.g. whether the socket is in non-blocking mode). + */ +void async_start_sync_io_request( struct async *async, data_size_t information ) +{ + assert( get_error() == STATUS_ALERTED ); + assert( async->thread == current ); + assert( !async->pending ); + + async->direct_result = 0; /* force APC to fire off */ + async->iosb->result = information; +} + /* complete a request-based async with a pre-allocated buffer */ void async_request_complete( struct async *async, unsigned int status, data_size_t result, data_size_t out_size, void *out_data )
Please add helpers only in the patches where they're used; it makes things very difficult to review otherwise.
Put it in a separate header file so that it won't pollute everyone's pre-processor macro definition namespace with ntstatus.h. Note that winnt.h omits STATUS_ALERTED. Signed-off-by: Jinoh Kang <jinoh.kang.kr(a)gmail.com> --- Notes: This function is used in the subsequent patch. See its notes. include/wine/async.h | 46 ++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 46 insertions(+) create mode 100644 include/wine/async.h diff --git a/include/wine/async.h b/include/wine/async.h new file mode 100644 index 00000000000..1ef4ded4319 --- /dev/null +++ b/include/wine/async.h @@ -0,0 +1,46 @@ +/* + * Common helper definitions for Wine asynchronous I/O + * + * Copyright (C) 2022 Jinoh Kang + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library 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 + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library; if not, write to the Free Software + * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301, USA + */ + +#ifndef __WINE_WINE_ASYNC_H +#define __WINE_WINE_ASYNC_H + +#include <windef.h> +#include <ntstatus.h> + +/* Tests whether the return status of an I/O request server call indicates that + * the ownership of async_fileio has been relinquished from the caller and + * transferred to the async completion routine. + */ +static inline BOOL is_completion_deferred( NTSTATUS status ) +{ + /* STATUS_ALERTED is a status code internally repurposed in wineserver + * to indicate that an APC_ASYNC_IO system APC has been queued to notify + * completion of the async. + * + * We could simply have translated STATUS_ALERTED to STATUS_PENDING in + * server side, obviating the need for this function; however, + * STATUS_ALERTED does not necessarily mean that the I/O request will be + * completed asynchronously (i.e. "async->pending" will be set). + * We explicitly differentiate these conditions to avoid confusion. + */ + return status == STATUS_PENDING || status == STATUS_ALERTED; +} + +#endif /* __WINE_WINE_ASYNC_H */ -- 2.31.1
On 1/22/22 08:36, Jinoh Kang wrote:
Put it in a separate header file so that it won't pollute everyone's pre-processor macro definition namespace with ntstatus.h. Note that winnt.h omits STATUS_ALERTED.
NTSTATUS values are already defined by all server files; there's no need for a separate header. The NTSTATUS typedef isn't, but we use "unsigned int" anyway.
On 1/26/22 03:21, Zebediah Figura (she/her) wrote:
On 1/22/22 08:36, Jinoh Kang wrote:
Put it in a separate header file so that it won't pollute everyone's pre-processor macro definition namespace with ntstatus.h. Note that winnt.h omits STATUS_ALERTED.
NTSTATUS values are already defined by all server files;
Sorry for the confusion (I'll squash up some of the patches as you've suggested anyway). The header is used by both the server and the client; hence its placement in include/.
there's no need for a separate header.
If a translation unit includes winnt.h but _not_ ntstatus.h, the compiler complains that STATUS_ALERTED is not defined. Perhaps we can just move STATUS_ALERTED into winnt.h instead?
The NTSTATUS typedef isn't, but we use "unsigned int" anyway.
Server code can already use NTSTATUS, so there's no problem with it. Nevertheless, there's nothing particularly wrong here with using "unsigned int" too. -- Sincerely, Jinoh Kang
Make recv_socket alert the async immediately if poll() call detects that there are incoming data in the socket, bypassing the wineserver's main polling loop. Signed-off-by: Jinoh Kang <jinoh.kang.kr(a)gmail.com> --- Notes: In the previous approach, we simply test for STATUS_ALERTED, do the I/O and substitute the status code appropriately. This would obviate the need for: - The hack where we pass the "nonblocking mode" flag via iosb information field. - The "is_completion_deferred" function, since we can substitute STATUS_ALERTED for STATUS_PENDING in the requestor function before doing anything else. dlls/ntdll/unix/socket.c | 10 +++++++--- include/wine/afd.h | 17 +++++++++++++++++ server/sock.c | 26 +++++++++++++++++++++++++- 3 files changed, 49 insertions(+), 4 deletions(-) diff --git a/dlls/ntdll/unix/socket.c b/dlls/ntdll/unix/socket.c index 71dfcdd1114..7d3795a953e 100644 --- a/dlls/ntdll/unix/socket.c +++ b/dlls/ntdll/unix/socket.c @@ -77,6 +77,7 @@ #include "wsipx.h" #include "af_irda.h" #include "wine/afd.h" +#include "wine/async.h" #include "unix_private.h" @@ -656,6 +657,7 @@ static BOOL async_recv_proc( void *user, ULONG_PTR *info, NTSTATUS *status ) { struct async_recv_ioctl *async = user; int fd, needs_close; + BOOL nonblocking; TRACE( "%#x\n", *status ); @@ -664,11 +666,13 @@ static BOOL async_recv_proc( void *user, ULONG_PTR *info, NTSTATUS *status ) if ((*status = server_get_unix_fd( async->io.handle, 0, &fd, &needs_close, NULL, NULL ))) return TRUE; + nonblocking = *info == AFD_WINE_IN_NONBLOCKING_MODE; + if (nonblocking) *info = 0; *status = try_recv( fd, async, info ); TRACE( "got status %#x, %#lx bytes read\n", *status, *info ); if (needs_close) close( fd ); - if (*status == STATUS_DEVICE_NOT_READY) + if (*status == STATUS_DEVICE_NOT_READY && !nonblocking) return FALSE; } release_fileio( &async->io ); @@ -756,7 +760,7 @@ static NTSTATUS sock_recv( HANDLE handle, HANDLE event, PIO_APC_ROUTINE apc, voi status = wine_server_call( req ); wait_handle = wine_server_ptr_handle( reply->wait ); options = reply->options; - if ((!NT_ERROR(status) || wait_handle) && status != STATUS_PENDING) + if ((!NT_ERROR(status) || wait_handle) && !is_completion_deferred( status )) { io->Status = status; io->Information = information; @@ -764,7 +768,7 @@ static NTSTATUS sock_recv( HANDLE handle, HANDLE event, PIO_APC_ROUTINE apc, voi } SERVER_END_REQ; - if (status != STATUS_PENDING) release_fileio( &async->io ); + if (!is_completion_deferred( status )) release_fileio( &async->io ); if (wait_handle) status = wait_async( wait_handle, options & FILE_SYNCHRONOUS_IO_ALERT ); return status; diff --git a/include/wine/afd.h b/include/wine/afd.h index efd5787e90a..1e26739229d 100644 --- a/include/wine/afd.h +++ b/include/wine/afd.h @@ -37,6 +37,23 @@ struct afd_wsabuf_32 # define WS(x) x #endif + +/* Used in the iosb.result field to indicate that the current socket I/O + * operation is in synchronous non-blocking mode. This value is normally + * transmitted via the APC_ASYNC_IO system APC call (with status STATUS_ALERTED) + * when the server gives the client a chance to complete the I/O synchronously + * before resuming the request as fully asynchronous I/O or failing it. + * If the I/O fails with EWOULDBLOCK and the iosb.result field is set to any + * other value, the client shall request the server to resume the asynchronous + * operation. + * + * The value (ULONG_PTR)-1 (the maximum value of ULONG_PTR) is chosen so that + * it will be least likely to be confused with "the number of bytes transferred + * so far." Any I/O operation that has made it to the maximum number of bytes + * shall complete immediately anyway. + */ +#define AFD_WINE_IN_NONBLOCKING_MODE ((ULONG_PTR)-1) + #define IOCTL_AFD_BIND CTL_CODE(FILE_DEVICE_BEEP, 0x800, METHOD_NEITHER, FILE_ANY_ACCESS) #define IOCTL_AFD_LISTEN CTL_CODE(FILE_DEVICE_BEEP, 0x802, METHOD_NEITHER, FILE_ANY_ACCESS) #define IOCTL_AFD_RECV CTL_CODE(FILE_DEVICE_BEEP, 0x805, METHOD_NEITHER, FILE_ANY_ACCESS) diff --git a/server/sock.c b/server/sock.c index 650e67a2e0a..03c867317b2 100644 --- a/server/sock.c +++ b/server/sock.c @@ -91,6 +91,7 @@ #include "wsipx.h" #include "af_irda.h" #include "wine/afd.h" +#include "wine/async.h" #include "process.h" #include "file.h" @@ -3397,6 +3398,7 @@ DECL_HANDLER(recv_socket) { struct sock *sock = (struct sock *)get_handle_obj( current->process, req->async.handle, 0, &sock_ops ); unsigned int status = req->status; + int pending = 0; timeout_t timeout = 0; struct async *async; struct fd *fd; @@ -3422,6 +3424,25 @@ DECL_HANDLER(recv_socket) if ((status == STATUS_PENDING || status == STATUS_DEVICE_NOT_READY) && sock->rd_shutdown) status = STATUS_PIPE_DISCONNECTED; + /* NOTE: If read_q is not empty, we cannot really tell if the already queued asyncs + * NOTE: will not consume all available data; if there's no data available, + * NOTE: the current request won't be immediately satiable. */ + if ((status == STATUS_PENDING || status == STATUS_DEVICE_NOT_READY) && !async_queued( &sock->read_q )) + { + struct pollfd pollfd; + pollfd.fd = get_unix_fd( sock->fd ); + pollfd.events = req->oob ? POLLPRI : POLLIN; + pollfd.revents = 0; + if (poll(&pollfd, 1, 0) >= 0 && pollfd.revents) + { + /* Give the client opportunity to complete synchronously. + * If it turns out that the I/O request is not actually immediately satiable, + * the client may then choose to re-queue the async (with STATUS_PENDING). */ + pending = status == STATUS_PENDING; + status = STATUS_ALERTED; + } + } + sock->pending_events &= ~(req->oob ? AFD_POLL_OOB : AFD_POLL_READ); sock->reported_events &= ~(req->oob ? AFD_POLL_OOB : AFD_POLL_READ); @@ -3438,12 +3459,15 @@ DECL_HANDLER(recv_socket) if (timeout) async_set_timeout( async, timeout, STATUS_IO_TIMEOUT ); - if (status == STATUS_PENDING) + if (is_completion_deferred( status )) queue_async( &sock->read_q, async ); /* always reselect; we changed reported_events above */ sock_reselect( sock ); + if (status == STATUS_ALERTED) + async_start_sync_io_request( async, pending ? 0 : AFD_WINE_IN_NONBLOCKING_MODE ); + reply->wait = async_handoff( async, NULL, 0 ); reply->options = get_fd_options( fd ); release_object( async ); -- 2.31.1
Hi, While running your changed tests, I think I found new failures. Being a bot and all I'm not very good at pattern recognition, so I might be wrong, but could you please double-check? Full results can be found at: https://testbot.winehq.org/JobDetails.pl?Key=105631 Your paranoid android. === debian11 (32 bit WoW report) === ntdll: change.c:241: Test failed: should be ready change.c:247: Test failed: action wrong change.c:277: Test failed: should be ready change.c:280: Test failed: info not set change.c:293: Test failed: status set too soon change.c:294: Test failed: info set too soon
On 1/22/22 08:36, Jinoh Kang wrote:
diff --git a/include/wine/afd.h b/include/wine/afd.h index efd5787e90a..1e26739229d 100644 --- a/include/wine/afd.h +++ b/include/wine/afd.h @@ -37,6 +37,23 @@ struct afd_wsabuf_32 # define WS(x) x #endif
+ +/* Used in the iosb.result field to indicate that the current socket I/O + * operation is in synchronous non-blocking mode. This value is normally + * transmitted via the APC_ASYNC_IO system APC call (with status STATUS_ALERTED) + * when the server gives the client a chance to complete the I/O synchronously + * before resuming the request as fully asynchronous I/O or failing it. + * If the I/O fails with EWOULDBLOCK and the iosb.result field is set to any + * other value, the client shall request the server to resume the asynchronous + * operation. + * + * The value (ULONG_PTR)-1 (the maximum value of ULONG_PTR) is chosen so that + * it will be least likely to be confused with "the number of bytes transferred + * so far." Any I/O operation that has made it to the maximum number of bytes + * shall complete immediately anyway. + */ +#define AFD_WINE_IN_NONBLOCKING_MODE ((ULONG_PTR)-1) +
Why add this instead of returning a separate field from the recv_socket request?
On 1/26/22 03:23, Zebediah Figura (she/her) wrote:
On 1/22/22 08:36, Jinoh Kang wrote:
diff --git a/include/wine/afd.h b/include/wine/afd.h index efd5787e90a..1e26739229d 100644 --- a/include/wine/afd.h +++ b/include/wine/afd.h @@ -37,6 +37,23 @@ struct afd_wsabuf_32 # define WS(x) x #endif + +/* Used in the iosb.result field to indicate that the current socket I/O + * operation is in synchronous non-blocking mode. This value is normally + * transmitted via the APC_ASYNC_IO system APC call (with status STATUS_ALERTED) + * when the server gives the client a chance to complete the I/O synchronously + * before resuming the request as fully asynchronous I/O or failing it. + * If the I/O fails with EWOULDBLOCK and the iosb.result field is set to any + * other value, the client shall request the server to resume the asynchronous + * operation. + * + * The value (ULONG_PTR)-1 (the maximum value of ULONG_PTR) is chosen so that + * it will be least likely to be confused with "the number of bytes transferred + * so far." Any I/O operation that has made it to the maximum number of bytes + * shall complete immediately anyway. + */ +#define AFD_WINE_IN_NONBLOCKING_MODE ((ULONG_PTR)-1) +
Why add this instead of returning a separate field from the recv_socket request?
It's not intended to be returned from recv_socket request.
This value is normally transmitted via the APC_ASYNC_IO system APC call (with status STATUS_ALERTED) transmitted via the APC_ASYNC_IO system APC call (with status STATUS_ALERTED) when the server gives the client a chance to complete the I/O synchronously
Do you have any suggestion as to how to better pass the blocking/nonblocking flag to async_recv_proc()? -- Sincerely, Jinoh Kang
On 1/25/22 20:29, Jinoh Kang wrote:
On 1/26/22 03:23, Zebediah Figura (she/her) wrote:
On 1/22/22 08:36, Jinoh Kang wrote:
diff --git a/include/wine/afd.h b/include/wine/afd.h index efd5787e90a..1e26739229d 100644 --- a/include/wine/afd.h +++ b/include/wine/afd.h @@ -37,6 +37,23 @@ struct afd_wsabuf_32 # define WS(x) x #endif + +/* Used in the iosb.result field to indicate that the current socket I/O + * operation is in synchronous non-blocking mode. This value is normally + * transmitted via the APC_ASYNC_IO system APC call (with status STATUS_ALERTED) + * when the server gives the client a chance to complete the I/O synchronously + * before resuming the request as fully asynchronous I/O or failing it. + * If the I/O fails with EWOULDBLOCK and the iosb.result field is set to any + * other value, the client shall request the server to resume the asynchronous + * operation. + * + * The value (ULONG_PTR)-1 (the maximum value of ULONG_PTR) is chosen so that + * it will be least likely to be confused with "the number of bytes transferred + * so far." Any I/O operation that has made it to the maximum number of bytes + * shall complete immediately anyway. + */ +#define AFD_WINE_IN_NONBLOCKING_MODE ((ULONG_PTR)-1) +
Why add this instead of returning a separate field from the recv_socket request?
It's not intended to be returned from recv_socket request.
This value is normally transmitted via the APC_ASYNC_IO system APC call (with status STATUS_ALERTED) transmitted via the APC_ASYNC_IO system APC call (with status STATUS_ALERTED) when the server gives the client a chance to complete the I/O synchronously
Do you have any suggestion as to how to better pass the blocking/nonblocking flag to async_recv_proc()?
Okay, I see the problem. I guess the approach of reusing APC infrastructure may not be as palatable after all. I'll take a look at the series beginning with 224085; it looks potentially better...
Signed-off-by: Jinoh Kang <jinoh.kang.kr(a)gmail.com> --- Notes: v1 -> v2: - server/request.h - delete autogenerated change - server/thread.c - handle APC handle allocation failure Alternative approaches considered: 1. Don't use thread_queue_apc to deliver the pseudo-APC; instead, use create_apc directly. This approach requires modification or partial duplication of async_terminate() for passing upward the return value of "create_apc". This in turn necessiates changing async_handoff() to also pass over the APC, which complicates the matter. 2. Don't use APC at all (the previous patch). In this case we do not create APCs at all, and this patch becomes unnecessary. server/protocol.def | 7 +++++++ server/thread.c | 49 +++++++++++++++++++++++++++++++++++++++++++++ server/thread.h | 3 +++ server/trace.c | 22 ++++++++++++++++++++ tools/make_requests | 1 + 5 files changed, 82 insertions(+) diff --git a/server/protocol.def b/server/protocol.def index db73f0418a9..f21f7187c4d 100644 --- a/server/protocol.def +++ b/server/protocol.def @@ -695,6 +695,13 @@ typedef union } break_process; } apc_result_t; +typedef struct +{ + apc_call_t call; /* APC call arguments */ + obj_handle_t apc_handle; /* handle to next APC */ + int __pad; +} inline_apc_t; + enum irp_type { IRP_CALL_NONE, diff --git a/server/thread.c b/server/thread.c index 467ccd1f0db..6015b256707 100644 --- a/server/thread.c +++ b/server/thread.c @@ -1211,6 +1211,54 @@ static void clear_apc_queue( struct list *queue ) } } +void try_suspend_apc_interrupt(void) +{ + static const select_op_t select_op_none = { SELECT_NONE }; + unsigned int error = get_error(); + select_on( &select_op_none, sizeof(select_op_none), 0, + SELECT_INTERRUPTIBLE, TIMEOUT_INFINITE ); + set_error( error ); +} + +static void enable_next_apc_interrupt(void) +{ + if (current->wait && !current->wait->cookie) end_wait( current, STATUS_TIMEOUT ); +} + +void resume_apc_interrupt(void) +{ + enable_next_apc_interrupt(); + if (!list_empty( ¤t->system_apc )) + wake_thread( current ); +} + +int dequeue_synchronous_system_apc( inline_apc_t *inline_apc ) +{ + struct thread_apc *apc; + obj_handle_t handle; + int result = 0; + unsigned int error = get_error(); + + if ((apc = thread_dequeue_apc( current, 1 ))) + { + if ((handle = alloc_handle_no_access_check( current->process, &apc->obj, SYNCHRONIZE, 0 ))) + { + memset( inline_apc, 0, sizeof(*inline_apc) ); + inline_apc->call = apc->call; + inline_apc->apc_handle = handle; + result = 1; + } + else + { + apc->executed = 1; + wake_up( &apc->obj, 0 ); + } + release_object( apc ); + } + set_error( error ); + return result; +} + /* add an fd to the inflight list */ /* return list index, or -1 on error */ int thread_add_inflight_fd( struct thread *thread, int client, int server ) @@ -1654,6 +1702,7 @@ DECL_HANDLER(select) release_object( apc ); } + enable_next_apc_interrupt(); reply->signaled = select_on( &select_op, op_size, req->cookie, req->flags, req->timeout ); if (get_error() == STATUS_USER_APC) diff --git a/server/thread.h b/server/thread.h index 8dcf966a90a..d1acd3267b1 100644 --- a/server/thread.h +++ b/server/thread.h @@ -116,6 +116,9 @@ extern void kill_thread( struct thread *thread, int violent_death ); extern void wake_up( struct object *obj, int max ); extern int thread_queue_apc( struct process *process, struct thread *thread, struct object *owner, const apc_call_t *call_data ); extern void thread_cancel_apc( struct thread *thread, struct object *owner, enum apc_type type ); +extern void try_suspend_apc_interrupt(void); +extern void resume_apc_interrupt(void); +extern int dequeue_synchronous_system_apc( inline_apc_t *inline_apc ); extern int thread_add_inflight_fd( struct thread *thread, int client, int server ); extern int thread_get_inflight_fd( struct thread *thread, int client ); extern struct token *thread_get_impersonation_token( struct thread *thread ); diff --git a/server/trace.c b/server/trace.c index a48f00258fe..7a6f5e1c119 100644 --- a/server/trace.c +++ b/server/trace.c @@ -328,6 +328,13 @@ static void dump_apc_result( const char *prefix, const apc_result_t *result ) fputc( '}', stderr ); } +static void dump_inline_apc( const char *prefix, const inline_apc_t *inline_apc ) +{ + fprintf( stderr, "%s{", prefix ); + dump_apc_call( "call=", &inline_apc->call ); + fprintf( stderr, ",apc_handle=%04x}", inline_apc->apc_handle ); +} + static void dump_async_data( const char *prefix, const async_data_t *data ) { fprintf( stderr, "%s{handle=%04x,event=%04x", prefix, data->handle, data->event ); @@ -533,6 +540,21 @@ static void dump_varargs_apc_result( const char *prefix, data_size_t size ) remove_data( size ); } +#ifdef __GNUC__ +__attribute__((unused)) +#endif +static void dump_varargs_inline_apc( const char *prefix, data_size_t size ) +{ + const inline_apc_t *result = cur_data; + + if (size >= sizeof(*result)) + { + dump_inline_apc( prefix, result ); + size = sizeof(*result); + } + remove_data( size ); +} + static void dump_varargs_select_op( const char *prefix, data_size_t size ) { select_op_t data; diff --git a/tools/make_requests b/tools/make_requests index 1c4e5977c8b..e85a5c4a615 100755 --- a/tools/make_requests +++ b/tools/make_requests @@ -47,6 +47,7 @@ my %formats = "rectangle_t" => [ 16, 4, "&dump_rectangle" ], "apc_call_t" => [ 48, 8, "&dump_apc_call" ], "apc_result_t" => [ 40, 8, "&dump_apc_result" ], + "inline_apc_t" => [ 56, 8, "&dump_inline_apc" ], "async_data_t" => [ 40, 8, "&dump_async_data" ], "irp_params_t" => [ 32, 8, "&dump_irp_params" ], "luid_t" => [ 8, 4, "&dump_luid" ], -- 2.31.1
On 1/22/22 08:36, Jinoh Kang wrote:
diff --git a/server/protocol.def b/server/protocol.def index db73f0418a9..f21f7187c4d 100644 --- a/server/protocol.def +++ b/server/protocol.def @@ -695,6 +695,13 @@ typedef union } break_process; } apc_result_t;
+typedef struct +{ + apc_call_t call; /* APC call arguments */ + obj_handle_t apc_handle; /* handle to next APC */ + int __pad; +} inline_apc_t; +
Why introduce this separate type?
On 1/26/22 03:26, Zebediah Figura (she/her) wrote:
On 1/22/22 08:36, Jinoh Kang wrote:
diff --git a/server/protocol.def b/server/protocol.def index db73f0418a9..f21f7187c4d 100644 --- a/server/protocol.def +++ b/server/protocol.def @@ -695,6 +695,13 @@ typedef union } break_process; } apc_result_t; +typedef struct +{ + apc_call_t call; /* APC call arguments */ + obj_handle_t apc_handle; /* handle to next APC */ + int __pad; +} inline_apc_t; +
Why introduce this separate type? As for "why introduce this type at all": Otherwise we have to pass around two arguments (call and apc_handle) instead of one (inline_apc_t) everywhere. This also adds two parameters to select_wait(), and I was not sure about the idea of adding ", NULL, NULL" to every user except the async one.
As for "why introduce a new type instead of using existing equivalent one": There's two substitute candidates: "struct queue_apc_request" and "struct select_reply". I'm not sure reusing them is allowed, though. Perhaps we can insert inline_apc_t inside those other two structs? -- Sincerely, Jinoh Kang
Synchronously deliver the pending system APC via server call reply if possible, instead of interrupting the target thread with a signal. Signed-off-by: Jinoh Kang <jinoh.kang.kr(a)gmail.com> --- Notes: Alternative approaches considered: 1. Don't use thread_queue_apc to deliver the pseudo-APC; instead, use create_apc directly. In this case, this would shorten this patch a lot, but would transfer the complexity of this patch to the previous one in the serie. 2. Don't use APC at all (the previous patch). In this case we do not create APCs at all, and this patch becomes unnecessary. server/async.c | 1 + server/protocol.def | 1 + server/sock.c | 10 ++++++++++ server/trace.c | 3 --- 4 files changed, 12 insertions(+), 3 deletions(-) diff --git a/server/async.c b/server/async.c index 5d0857f3eec..cf49a309da6 100644 --- a/server/async.c +++ b/server/async.c @@ -396,6 +396,7 @@ void async_start_sync_io_request( struct async *async, data_size_t information ) assert( async->thread == current ); assert( !async->pending ); + try_suspend_apc_interrupt(); async->direct_result = 0; /* force APC to fire off */ async->iosb->result = information; } diff --git a/server/protocol.def b/server/protocol.def index f21f7187c4d..348791c28da 100644 --- a/server/protocol.def +++ b/server/protocol.def @@ -1447,6 +1447,7 @@ enum server_fd_type @REPLY obj_handle_t wait; /* handle to wait on for blocking recv */ unsigned int options; /* device open options */ + VARARG(inline_apc,inline_apc); /* Next system APC to execute immediately */ @END diff --git a/server/sock.c b/server/sock.c index 03c867317b2..683cbb21aa6 100644 --- a/server/sock.c +++ b/server/sock.c @@ -3470,6 +3470,16 @@ DECL_HANDLER(recv_socket) reply->wait = async_handoff( async, NULL, 0 ); reply->options = get_fd_options( fd ); + + if (status == STATUS_ALERTED) + { + inline_apc_t inline_apc; + if (get_reply_max_size() == sizeof(inline_apc) && dequeue_synchronous_system_apc( &inline_apc )) + set_reply_data( &inline_apc, sizeof(inline_apc) ); + else + resume_apc_interrupt(); + } + release_object( async ); } release_object( sock ); diff --git a/server/trace.c b/server/trace.c index 7a6f5e1c119..40aa1078cf2 100644 --- a/server/trace.c +++ b/server/trace.c @@ -540,9 +540,6 @@ static void dump_varargs_apc_result( const char *prefix, data_size_t size ) remove_data( size ); } -#ifdef __GNUC__ -__attribute__((unused)) -#endif static void dump_varargs_inline_apc( const char *prefix, data_size_t size ) { const inline_apc_t *result = cur_data; -- 2.31.1
If the "inline_apc" argument is specified, the function will first invoke the APC specified by "inline_apc" and return the result to the wineserver. This is done as a single server call along with the actual wait, saving a round trip to the server. Signed-off-by: Jinoh Kang <jinoh.kang.kr(a)gmail.com> --- Notes: The alternative approach (adding a select_op) is less intrusive since we don't have to modify all callers of server_wait() and server_select(). dlls/ntdll/unix/server.c | 16 +++++++++++----- dlls/ntdll/unix/sync.c | 10 +++++----- dlls/ntdll/unix/thread.c | 4 ++-- dlls/ntdll/unix/unix_private.h | 5 +++-- 4 files changed, 21 insertions(+), 14 deletions(-) diff --git a/dlls/ntdll/unix/server.c b/dlls/ntdll/unix/server.c index 9d0594d3374..5960559c976 100644 --- a/dlls/ntdll/unix/server.c +++ b/dlls/ntdll/unix/server.c @@ -585,7 +585,8 @@ static void invoke_system_apc( const apc_call_t *call, apc_result_t *result, BOO * server_select */ unsigned int server_select( const select_op_t *select_op, data_size_t size, UINT flags, - timeout_t abs_timeout, context_t *context, user_apc_t *user_apc ) + timeout_t abs_timeout, context_t *context, user_apc_t *user_apc, + const inline_apc_t *inline_apc ) { unsigned int ret; int cookie; @@ -601,6 +602,11 @@ unsigned int server_select( const select_op_t *select_op, data_size_t size, UINT do { pthread_sigmask( SIG_BLOCK, &server_block_set, &old_set ); + if (inline_apc && (apc_handle = inline_apc->apc_handle)) + { + invoke_system_apc( &inline_apc->call, &result, FALSE ); + inline_apc = NULL; + } for (;;) { SERVER_START_REQ( select ) @@ -649,7 +655,7 @@ unsigned int server_select( const select_op_t *select_op, data_size_t size, UINT * server_wait */ unsigned int server_wait( const select_op_t *select_op, data_size_t size, UINT flags, - const LARGE_INTEGER *timeout ) + const LARGE_INTEGER *timeout, const inline_apc_t *inline_apc ) { timeout_t abs_timeout = timeout ? timeout->QuadPart : TIMEOUT_INFINITE; unsigned int ret; @@ -663,7 +669,7 @@ unsigned int server_wait( const select_op_t *select_op, data_size_t size, UINT f abs_timeout -= now.QuadPart; } - ret = server_select( select_op, size, flags, abs_timeout, NULL, &apc ); + ret = server_select( select_op, size, flags, abs_timeout, NULL, &apc, inline_apc ); if (ret == STATUS_USER_APC) return invoke_user_apc( NULL, &apc, ret ); /* A test on Windows 2000 shows that Windows always yields during @@ -684,7 +690,7 @@ NTSTATUS WINAPI NtContinue( CONTEXT *context, BOOLEAN alertable ) if (alertable) { - status = server_select( NULL, 0, SELECT_INTERRUPTIBLE | SELECT_ALERTABLE, 0, NULL, &apc ); + status = server_select( NULL, 0, SELECT_INTERRUPTIBLE | SELECT_ALERTABLE, 0, NULL, &apc, NULL ); if (status == STATUS_USER_APC) return invoke_user_apc( context, &apc, status ); } return signal_set_full_context( context ); @@ -699,7 +705,7 @@ NTSTATUS WINAPI NtTestAlert(void) user_apc_t apc; NTSTATUS status; - status = server_select( NULL, 0, SELECT_INTERRUPTIBLE | SELECT_ALERTABLE, 0, NULL, &apc ); + status = server_select( NULL, 0, SELECT_INTERRUPTIBLE | SELECT_ALERTABLE, 0, NULL, &apc, NULL ); if (status == STATUS_USER_APC) invoke_user_apc( NULL, &apc, STATUS_SUCCESS ); return STATUS_SUCCESS; } diff --git a/dlls/ntdll/unix/sync.c b/dlls/ntdll/unix/sync.c index 442243d8bcf..8fe1e1145e7 100644 --- a/dlls/ntdll/unix/sync.c +++ b/dlls/ntdll/unix/sync.c @@ -1406,7 +1406,7 @@ NTSTATUS WINAPI NtWaitForMultipleObjects( DWORD count, const HANDLE *handles, BO if (alertable) flags |= SELECT_ALERTABLE; select_op.wait.op = wait_any ? SELECT_WAIT : SELECT_WAIT_ALL; for (i = 0; i < count; i++) select_op.wait.handles[i] = wine_server_obj_handle( handles[i] ); - return server_wait( &select_op, offsetof( select_op_t, wait.handles[count] ), flags, timeout ); + return server_wait( &select_op, offsetof( select_op_t, wait.handles[count] ), flags, timeout, NULL ); } @@ -1434,7 +1434,7 @@ NTSTATUS WINAPI NtSignalAndWaitForSingleObject( HANDLE signal, HANDLE wait, select_op.signal_and_wait.op = SELECT_SIGNAL_AND_WAIT; select_op.signal_and_wait.wait = wine_server_obj_handle( wait ); select_op.signal_and_wait.signal = wine_server_obj_handle( signal ); - return server_wait( &select_op, sizeof(select_op.signal_and_wait), flags, timeout ); + return server_wait( &select_op, sizeof(select_op.signal_and_wait), flags, timeout, NULL ); } @@ -1458,7 +1458,7 @@ NTSTATUS WINAPI NtYieldExecution(void) NTSTATUS WINAPI NtDelayExecution( BOOLEAN alertable, const LARGE_INTEGER *timeout ) { /* if alertable, we need to query the server */ - if (alertable) return server_wait( NULL, 0, SELECT_INTERRUPTIBLE | SELECT_ALERTABLE, timeout ); + if (alertable) return server_wait( NULL, 0, SELECT_INTERRUPTIBLE | SELECT_ALERTABLE, timeout, NULL ); if (!timeout || timeout->QuadPart == TIMEOUT_INFINITE) /* sleep forever */ { @@ -1703,7 +1703,7 @@ NTSTATUS WINAPI NtWaitForKeyedEvent( HANDLE handle, const void *key, select_op.keyed_event.op = SELECT_KEYED_EVENT_WAIT; select_op.keyed_event.handle = wine_server_obj_handle( handle ); select_op.keyed_event.key = wine_server_client_ptr( key ); - return server_wait( &select_op, sizeof(select_op.keyed_event), flags, timeout ); + return server_wait( &select_op, sizeof(select_op.keyed_event), flags, timeout, NULL ); } @@ -1722,7 +1722,7 @@ NTSTATUS WINAPI NtReleaseKeyedEvent( HANDLE handle, const void *key, select_op.keyed_event.op = SELECT_KEYED_EVENT_RELEASE; select_op.keyed_event.handle = wine_server_obj_handle( handle ); select_op.keyed_event.key = wine_server_client_ptr( key ); - return server_wait( &select_op, sizeof(select_op.keyed_event), flags, timeout ); + return server_wait( &select_op, sizeof(select_op.keyed_event), flags, timeout, NULL ); } diff --git a/dlls/ntdll/unix/thread.c b/dlls/ntdll/unix/thread.c index 618ebb82bfb..5571085c2e2 100644 --- a/dlls/ntdll/unix/thread.c +++ b/dlls/ntdll/unix/thread.c @@ -1434,7 +1434,7 @@ void wait_suspend( CONTEXT *context ) contexts_to_server( server_contexts, context ); /* wait with 0 timeout, will only return once the thread is no longer suspended */ - server_select( NULL, 0, SELECT_INTERRUPTIBLE, 0, server_contexts, NULL ); + server_select( NULL, 0, SELECT_INTERRUPTIBLE, 0, server_contexts, NULL, NULL ); contexts_from_server( context, server_contexts ); errno = saved_errno; } @@ -1483,7 +1483,7 @@ NTSTATUS send_debug_event( EXCEPTION_RECORD *rec, CONTEXT *context, BOOL first_c contexts_to_server( server_contexts, context ); server_select( &select_op, offsetof( select_op_t, wait.handles[1] ), SELECT_INTERRUPTIBLE, - TIMEOUT_INFINITE, server_contexts, NULL ); + TIMEOUT_INFINITE, server_contexts, NULL, NULL ); SERVER_START_REQ( get_exception_status ) { diff --git a/dlls/ntdll/unix/unix_private.h b/dlls/ntdll/unix/unix_private.h index a79edabc37c..fc7ae671314 100644 --- a/dlls/ntdll/unix/unix_private.h +++ b/dlls/ntdll/unix/unix_private.h @@ -161,9 +161,10 @@ extern unsigned int server_call_unlocked( void *req_ptr ) DECLSPEC_HIDDEN; extern void server_enter_uninterrupted_section( pthread_mutex_t *mutex, sigset_t *sigset ) DECLSPEC_HIDDEN; extern void server_leave_uninterrupted_section( pthread_mutex_t *mutex, sigset_t *sigset ) DECLSPEC_HIDDEN; extern unsigned int server_select( const select_op_t *select_op, data_size_t size, UINT flags, - timeout_t abs_timeout, context_t *context, user_apc_t *user_apc ) DECLSPEC_HIDDEN; + timeout_t abs_timeout, context_t *context, user_apc_t *user_apc, + const inline_apc_t *inline_apc ) DECLSPEC_HIDDEN; extern unsigned int server_wait( const select_op_t *select_op, data_size_t size, UINT flags, - const LARGE_INTEGER *timeout ) DECLSPEC_HIDDEN; + const LARGE_INTEGER *timeout, const inline_apc_t *inline_apc ) DECLSPEC_HIDDEN; extern unsigned int server_queue_process_apc( HANDLE process, const apc_call_t *call, apc_result_t *result ) DECLSPEC_HIDDEN; extern int server_get_unix_fd( HANDLE handle, unsigned int wanted_access, int *unix_fd, -- 2.31.1
Signed-off-by: Jinoh Kang <jinoh.kang.kr(a)gmail.com> --- Notes: This essentially replaces the signal_wait_async() function from the previous patch serie. dlls/ntdll/unix/sync.c | 19 +++++++++++++++++++ dlls/ntdll/unix/unix_private.h | 1 + 2 files changed, 20 insertions(+) diff --git a/dlls/ntdll/unix/sync.c b/dlls/ntdll/unix/sync.c index 8fe1e1145e7..05c27cfb5e4 100644 --- a/dlls/ntdll/unix/sync.c +++ b/dlls/ntdll/unix/sync.c @@ -2510,3 +2510,22 @@ NTSTATUS WINAPI NtWaitForAlertByThreadId( const void *address, const LARGE_INTEG } #endif + +NTSTATUS wait_async_after_apc( HANDLE optional_handle, BOOL alertable, NTSTATUS prev_status, const inline_apc_t *inline_apc ) +{ + select_op_t select_op; + UINT flags = SELECT_INTERRUPTIBLE; + + if (alertable) flags |= SELECT_ALERTABLE; + + if (!optional_handle) + { + static const LARGE_INTEGER zero = {{ 0 }}; + server_wait( NULL, 0, flags, &zero, inline_apc ); + return prev_status; + } + + select_op.wait.op = SELECT_WAIT_ALL; + select_op.wait.handles[0] = wine_server_obj_handle( optional_handle ); + return server_wait( &select_op, offsetof( select_op_t, wait.handles[1] ), flags, NULL, inline_apc ); +} diff --git a/dlls/ntdll/unix/unix_private.h b/dlls/ntdll/unix/unix_private.h index fc7ae671314..ea46ff3eb55 100644 --- a/dlls/ntdll/unix/unix_private.h +++ b/dlls/ntdll/unix/unix_private.h @@ -275,6 +275,7 @@ extern NTSTATUS get_device_info( int fd, struct _FILE_FS_DEVICE_INFORMATION *inf extern void init_files(void) DECLSPEC_HIDDEN; extern void init_cpu_info(void) DECLSPEC_HIDDEN; extern void add_completion( HANDLE handle, ULONG_PTR value, NTSTATUS status, ULONG info, BOOL async ) DECLSPEC_HIDDEN; +extern NTSTATUS wait_async_after_apc( HANDLE optional_handle, BOOL alertable, NTSTATUS prev_status, const inline_apc_t *inline_apc ); extern void dbg_init(void) DECLSPEC_HIDDEN; -- 2.31.1
The synchronous completion request APC will be delivered via "inline_apc" instead of interrupting the current thread. Signed-off-by: Jinoh Kang <jinoh.kang.kr(a)gmail.com> --- Notes: The synchronous I/O-case logic was inside the requestor function (sock_recv) in the previous patch, but has now been moved into the asynchronous I/O completion callback (async_recv_proc) for this patch serie. dlls/ntdll/unix/socket.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/dlls/ntdll/unix/socket.c b/dlls/ntdll/unix/socket.c index 7d3795a953e..3ab3c55d6e4 100644 --- a/dlls/ntdll/unix/socket.c +++ b/dlls/ntdll/unix/socket.c @@ -690,6 +690,7 @@ static NTSTATUS sock_recv( HANDLE handle, HANDLE event, PIO_APC_ROUTINE apc, voi NTSTATUS status; unsigned int i; ULONG options; + inline_apc_t inline_apc; if (unix_flags & MSG_OOB) { @@ -751,12 +752,14 @@ static NTSTATUS sock_recv( HANDLE handle, HANDLE event, PIO_APC_ROUTINE apc, voi if (status == STATUS_DEVICE_NOT_READY && force_async) status = STATUS_PENDING; + memset( &inline_apc, 0, sizeof(inline_apc) ); SERVER_START_REQ( recv_socket ) { req->status = status; req->total = information; req->async = server_async( handle, &async->io, event, apc, apc_user, iosb_client_ptr(io) ); req->oob = !!(unix_flags & MSG_OOB); + wine_server_set_reply( req, &inline_apc, sizeof(inline_apc) ); status = wine_server_call( req ); wait_handle = wine_server_ptr_handle( reply->wait ); options = reply->options; @@ -770,8 +773,7 @@ static NTSTATUS sock_recv( HANDLE handle, HANDLE event, PIO_APC_ROUTINE apc, voi if (!is_completion_deferred( status )) release_fileio( &async->io ); - if (wait_handle) status = wait_async( wait_handle, options & FILE_SYNCHRONOUS_IO_ALERT ); - return status; + return wait_async_after_apc( wait_handle, options & FILE_SYNCHRONOUS_IO_ALERT, status, &inline_apc ); } -- 2.31.1
Otherwise, try_recv() call from sock_recv() may race against try_recv() call from async_recv_proc(), shuffling the packet order. Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=52401 Signed-off-by: Jinoh Kang <jinoh.kang.kr(a)gmail.com> --- Notes: Patch split as per review feedback: https://www.winehq.org/pipermail/wine-devel/2022-January/204710.html dlls/ntdll/unix/socket.c | 16 +++------------- dlls/ws2_32/tests/sock.c | 8 ++++---- 2 files changed, 7 insertions(+), 17 deletions(-) diff --git a/dlls/ntdll/unix/socket.c b/dlls/ntdll/unix/socket.c index 3ab3c55d6e4..a5f808c265d 100644 --- a/dlls/ntdll/unix/socket.c +++ b/dlls/ntdll/unix/socket.c @@ -684,7 +684,6 @@ static NTSTATUS sock_recv( HANDLE handle, HANDLE event, PIO_APC_ROUTINE apc, voi struct WS_sockaddr *addr, int *addr_len, DWORD *ret_flags, int unix_flags, int force_async ) { struct async_recv_ioctl *async; - ULONG_PTR information; HANDLE wait_handle; DWORD async_size; NTSTATUS status; @@ -741,22 +740,13 @@ static NTSTATUS sock_recv( HANDLE handle, HANDLE event, PIO_APC_ROUTINE apc, voi } } - status = try_recv( fd, async, &information ); - - if (status != STATUS_SUCCESS && status != STATUS_BUFFER_OVERFLOW && status != STATUS_DEVICE_NOT_READY) - { - release_fileio( &async->io ); - return status; - } - - if (status == STATUS_DEVICE_NOT_READY && force_async) - status = STATUS_PENDING; + status = force_async ? STATUS_PENDING : STATUS_DEVICE_NOT_READY; memset( &inline_apc, 0, sizeof(inline_apc) ); SERVER_START_REQ( recv_socket ) { req->status = status; - req->total = information; + req->total = 0; req->async = server_async( handle, &async->io, event, apc, apc_user, iosb_client_ptr(io) ); req->oob = !!(unix_flags & MSG_OOB); wine_server_set_reply( req, &inline_apc, sizeof(inline_apc) ); @@ -766,7 +756,7 @@ static NTSTATUS sock_recv( HANDLE handle, HANDLE event, PIO_APC_ROUTINE apc, voi if ((!NT_ERROR(status) || wait_handle) && !is_completion_deferred( status )) { io->Status = status; - io->Information = information; + io->Information = 0; } } SERVER_END_REQ; diff --git a/dlls/ws2_32/tests/sock.c b/dlls/ws2_32/tests/sock.c index 054e597b719..4199676f460 100644 --- a/dlls/ws2_32/tests/sock.c +++ b/dlls/ws2_32/tests/sock.c @@ -7750,8 +7750,8 @@ static void test_shutdown(void) WSASetLastError(0xdeadbeef); ret = recv(server, buffer, sizeof(buffer), 0); - todo_wine ok(ret == -1, "got %d\n", ret); - todo_wine ok(WSAGetLastError() == WSAESHUTDOWN, "got error %u\n", WSAGetLastError()); + ok(ret == -1, "got %d\n", ret); + ok(WSAGetLastError() == WSAESHUTDOWN, "got error %u\n", WSAGetLastError()); ret = send(server, "test", 5, 0); ok(ret == 5, "got %d\n", ret); @@ -7845,8 +7845,8 @@ static void test_shutdown(void) WSASetLastError(0xdeadbeef); ret = recv(server, buffer, sizeof(buffer), 0); - todo_wine ok(ret == -1, "got %d\n", ret); - todo_wine ok(WSAGetLastError() == WSAESHUTDOWN, "got error %u\n", WSAGetLastError()); + ok(ret == -1, "got %d\n", ret); + ok(WSAGetLastError() == WSAESHUTDOWN, "got error %u\n", WSAGetLastError()); WSASetLastError(0xdeadbeef); ret = send(server, "test", 5, 0); -- 2.31.1
The 'status' field of recv_socket_request is always either STATUS_PENDING or STATUS_DEVICE_NOT_READY, and the 'total' field is always zero. Replace the 'status' field with 'force_async' boolean field, and get rid of the 'total' field entirely. Also, clean up the recv_socket handler code a bit. Signed-off-by: Jinoh Kang <jinoh.kang.kr(a)gmail.com> --- Notes: Patch split as per review feedback: https://www.winehq.org/pipermail/wine-devel/2022-January/204710.html v1 -> v2: - server/request.h - delete autogenerated change dlls/ntdll/unix/socket.c | 5 +---- server/protocol.def | 3 +-- server/sock.c | 24 ++++++++++-------------- 3 files changed, 12 insertions(+), 20 deletions(-) diff --git a/dlls/ntdll/unix/socket.c b/dlls/ntdll/unix/socket.c index a5f808c265d..a78f4c733a6 100644 --- a/dlls/ntdll/unix/socket.c +++ b/dlls/ntdll/unix/socket.c @@ -740,13 +740,10 @@ static NTSTATUS sock_recv( HANDLE handle, HANDLE event, PIO_APC_ROUTINE apc, voi } } - status = force_async ? STATUS_PENDING : STATUS_DEVICE_NOT_READY; - memset( &inline_apc, 0, sizeof(inline_apc) ); SERVER_START_REQ( recv_socket ) { - req->status = status; - req->total = 0; + req->force_async = force_async; req->async = server_async( handle, &async->io, event, apc, apc_user, iosb_client_ptr(io) ); req->oob = !!(unix_flags & MSG_OOB); wine_server_set_reply( req, &inline_apc, sizeof(inline_apc) ); diff --git a/server/protocol.def b/server/protocol.def index 348791c28da..13a70e8c576 100644 --- a/server/protocol.def +++ b/server/protocol.def @@ -1442,8 +1442,7 @@ enum server_fd_type @REQ(recv_socket) int oob; /* are we receiving OOB data? */ async_data_t async; /* async I/O parameters */ - unsigned int status; /* status of initial call */ - unsigned int total; /* number of bytes already read */ + int force_async; /* Force asynchronous mode? */ @REPLY obj_handle_t wait; /* handle to wait on for blocking recv */ unsigned int options; /* device open options */ diff --git a/server/sock.c b/server/sock.c index 683cbb21aa6..10e7b31c44b 100644 --- a/server/sock.c +++ b/server/sock.c @@ -3397,8 +3397,8 @@ struct object *create_socket_device( struct object *root, const struct unicode_s DECL_HANDLER(recv_socket) { struct sock *sock = (struct sock *)get_handle_obj( current->process, req->async.handle, 0, &sock_ops ); - unsigned int status = req->status; - int pending = 0; + unsigned int status = STATUS_PENDING; + int pending = req->force_async; timeout_t timeout = 0; struct async *async; struct fd *fd; @@ -3406,8 +3406,8 @@ DECL_HANDLER(recv_socket) if (!sock) return; fd = sock->fd; - /* recv() returned EWOULDBLOCK, i.e. no data available yet */ - if (status == STATUS_DEVICE_NOT_READY && !sock->nonblocking) + /* Synchronous, *blocking* I/O requested? */ + if (!req->force_async && !sock->nonblocking) { /* Set a timeout on the async if necessary. * @@ -3418,16 +3418,16 @@ DECL_HANDLER(recv_socket) if (is_fd_overlapped( fd )) timeout = (timeout_t)sock->rcvtimeo * -10000; - status = STATUS_PENDING; + pending = 1; } - if ((status == STATUS_PENDING || status == STATUS_DEVICE_NOT_READY) && sock->rd_shutdown) + if (status == STATUS_PENDING && sock->rd_shutdown) status = STATUS_PIPE_DISCONNECTED; /* NOTE: If read_q is not empty, we cannot really tell if the already queued asyncs * NOTE: will not consume all available data; if there's no data available, * NOTE: the current request won't be immediately satiable. */ - if ((status == STATUS_PENDING || status == STATUS_DEVICE_NOT_READY) && !async_queued( &sock->read_q )) + if (status == STATUS_PENDING && !async_queued( &sock->read_q )) { struct pollfd pollfd; pollfd.fd = get_unix_fd( sock->fd ); @@ -3438,22 +3438,18 @@ DECL_HANDLER(recv_socket) /* Give the client opportunity to complete synchronously. * If it turns out that the I/O request is not actually immediately satiable, * the client may then choose to re-queue the async (with STATUS_PENDING). */ - pending = status == STATUS_PENDING; status = STATUS_ALERTED; } } + if (!pending && status == STATUS_PENDING) + status = STATUS_DEVICE_NOT_READY; /* -> WSAEWOULDBLOCK */ + sock->pending_events &= ~(req->oob ? AFD_POLL_OOB : AFD_POLL_READ); sock->reported_events &= ~(req->oob ? AFD_POLL_OOB : AFD_POLL_READ); if ((async = create_request_async( fd, get_fd_comp_flags( fd ), &req->async ))) { - if (status == STATUS_SUCCESS) - { - struct iosb *iosb = async_get_iosb( async ); - iosb->result = req->total; - release_object( iosb ); - } set_error( status ); if (timeout) -- 2.31.1
participants (3)
-
Jinoh Kang -
Marvin -
Zebediah Figura (she/her)