On 2/3/22 08:45, Zebediah Figura wrote:
On 1/29/22 01:47, Jinoh Kang wrote:
...
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,
We still need to assert "async->terminated && async->alerted" so that "async_set_result()" works correctly on STATUS_PENDING case. The last one seems actually unnecessary though (looks like a leftover from v3 patch, my apologies).
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,
Sounds good.
returning early, and possibly returning some error condition (STATUS_INVALID_PARAMETER?) to make it clear that the client is misbehaving.
In that case, I'd like to hear some suggestions on how the client would handle (or not handle) the "STATUS_INVALID_PARAMETER" error.
v3 -> v4: - 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)
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.
"async->unknown_status" should imply "!async->pending" anyway, so I see what you're saying here.
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".
I concur.
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.
async_handoff() is one of those few places where 'we check for "iosb->status == STATUS_PENDING"'. Since the "async_terminate()" call inside "async_handoff()" doesn't work here (async is in terminated state), the async->iosb->status value set here will be used to determine the eventual pending status of async. If STATUS_PENDING, some necessary flags (async->direct_result and async->pending) will also be set/cleared. Note that the set_error() below is still required so that initial_status is properly set and synchronous failure case properly done, etc.
+ /* 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().
You're right--I just had to explain that above and it already sounds too complicated. Perhaps only open code the relevant parts?
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?
Yes. Especially this part:
if (async->iosb->status != STATUS_PENDING) { if (result) *result = async->iosb->result; async->signaled = 1; } else { async->direct_result = 0; async->pending = 1; if (!async->blocking) { close_handle( async->thread->process, async->wait_handle); async->wait_handle = 0; } }
The flags need to be properly set (especially in the else branch) so that the subsequent wait_async() would properly function. Also note the blocking part, synchronous vs. asynchronous failure, etc.
+ /* 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?
It isn't.
If the client gave us a handle in the first place then async->wait_handle must be nonzero.
It can be set to zero in the middle by async_handoff() (see above).
+ { + 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?
I don't think it normally should, but I don't exactly feel easy about silencing error from the server, either.
+ 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 */ +@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 */ +@REPLY + obj_handle_t handle; /* wait handle, or NULL if closed */
Why do we need this?
We could have just returned a boolean to indicate whether the wait handle has been closed by us. (Note: the wait handle is closed when we're transitioning to STATUS_PENDING, _and_ async->blocking == 0.) However, I found that just returning either the original handle or NULL to indicate that simplifies code a lot.
+@END
/* Perform a read on a file object */ @REQ(read) async_data_t async; /* async I/O parameters */