On 1/29/22 01:47, Jinoh Kang wrote:
Some I/O operations need a way to "queue" the async to the target object first, even if the operation itself is to be completed synchronously. After synchronous completion, it needs a way to notify the IO_STATUS_BLOCK values back to the server.
Add a new wineserver request, "notify_async_direct_result", which notifies direct (i.e. synchronous) completion of async from the same thread.
Signed-off-by: Jinoh Kang <jinoh.kang.kr(a)gmail.com> ---
Notes: v1 -> v2: dequeue async after failed completion v2 -> v3: rewrite handling of pending/failed asyncs. v3 -> v4: - add assert( async->iosb ); - make robust against abrupt transition/cancellation: don't return STATUS_ACCESS_DENIED on precondition failure (client won't know what to do with STATUS_ACCESS_DENIED anyway) - always return wait_handle if possible - clarify and rearrange comments
server/async.c | 62 +++++++++++++++++++++++++++++++++++++++++++++ server/protocol.def | 10 ++++++++ 2 files changed, 72 insertions(+)
diff --git a/server/async.c b/server/async.c index e169bb23225..8852ff1c6d5 100644 --- a/server/async.c +++ b/server/async.c @@ -752,3 +752,65 @@ DECL_HANDLER(get_async_result) } set_error( iosb->status ); } + +/* Notify direct completion of async and close the wait handle if not blocking */ +DECL_HANDLER(notify_async_direct_result) +{ + struct async *async = (struct async *)get_handle_obj( current->process, req->handle, 0, &async_ops ); + + if (!async) return; + + /* we only return an async handle for asyncs created via create_request_async() */ + assert( async->iosb ); + + if (async->unknown_status && !async->pending && + async->terminated && async->alerted && + async->iosb->status == STATUS_PENDING)
So, these are all things that happen to be true assuming the client is behaving correctly, but the way this is written doesn't exactly make that clear (in particular, with a large if block, I can't see what the "else" case looks like immediately). I would recommend inverting the condition, returning early, and possibly returning some error condition (STATUS_INVALID_PARAMETER?) to make it clear that the client is misbehaving. Note also that "async->pending" is kind of irrelevant here, it semantically is only meaningful when considering things like whether the request should queue completion. I.e. it can kind of be considered undefined if async->unknown_status == 1. I'm not sure async->iosb->status is meaningful either, is it? We check for "iosb->status == STATUS_PENDING" in a few places, but I'm not really sure that's a good idea; we should probably be treating it more like "the value that gets returned in the userspace IOSB". In any case I don't think it has semantic value here.
+ { + /* async->terminated is 1, so async_terminate() won't do anything. + * We need to set the status and result for ourselves. + */ + async->iosb->status = req->status; + async->iosb->result = req->information;
Except that neither of these variables actually matter past async_set_result(), right? We shouldn't need to bother.
+ + /* Set status for async_handoff(). */ + set_error( async->iosb->status ); + + /* The async_handoff() call prior to the current server request was + * effectively a no-op since async->unknown_status is 1. Calling it + * again with async->unknown_status = 0 will do the remaining steps. + * + * NOTE: async_handoff() is being called with async->terminated = 1. + */ + async->unknown_status = 0; + async_handoff( async, NULL, 0 );
I kind of get the idea here, but my intuition tells me that this is a bad idea. I have to think too hard about what's involved in async_handoff(). Do we even get anything out of this that's not covered by async_set_initial_status(), plus async_set_result() and the wait_handle management below?
+ + /* If the I/O has completed successfully, the client would have already + * set the IOSB. Therefore, we can skip waiting on wait_handle and do + * async_set_result() directly. + */ + async_set_result( &async->obj, async->iosb->status, async->iosb->result ); + + /* Close wait handle here to avoid extra server round trip if the I/O + * has completed successfully. + * + * - If STATUS_PENDING, async_handoff() decides whether to close + * the wait handle for us. The wait handle is kept open if + * async->blocking is set (i.e. we're going to block on it). + * + * - If failed (NT_ERROR), async_handoff() always closes + * the wait_handle, which means async->wait_handle == 0. + */ + if (async->wait_handle && async->iosb->status != STATUS_PENDING)
Isn't the first part of this condition redundant? If the client gave us a handle in the first place then async->wait_handle must be nonzero.
+ { + close_handle( async->thread->process, async->wait_handle ); + async->wait_handle = 0; + } + } /* Otherwise, async have been abruptly transitioned (cancelled?). */ + + /* async_set_result() may have overriden the error value. Reset it. */ + set_error( async->iosb->status );
Does the client even need to care what this request returns?
+ reply->handle = async->wait_handle; + + release_object( &async->obj ); +} diff --git a/server/protocol.def b/server/protocol.def index 02e73047f9b..d79eca074a0 100644 --- a/server/protocol.def +++ b/server/protocol.def @@ -2163,6 +2163,16 @@ enum message_type @END
+/* Notify direct completion of async and close the wait handle if not blocking */ +(a)REQ(notify_async_direct_result)
I might recommend calling this "set" instead of "notify". Consider e.g. "set_irp_result".
+ obj_handle_t handle; /* wait handle */ + unsigned int status; /* completion status */ + apc_param_t information; /* IO_STATUS_BLOCK Information */ +(a)REPLY + obj_handle_t handle; /* wait handle, or NULL if closed */
Why do we need this?
+(a)END + + /* Perform a read on a file object */ @REQ(read) async_data_t async; /* async I/O parameters */