On 2/3/22 10:23, Jinoh Kang wrote:
In that case, I'd like to hear some suggestions on how the client would handle (or not handle) the "STATUS_INVALID_PARAMETER" error.
In a sense the client doesn't need to. If the client is behaving correctly it shouldn't happen. In that case I think it's fine to assert() [or ERR(), or just ignore the status]—we just don't want the *server* to assert() if the *client* misbehaves. [One day in the far off future the server may be a higher-privileged process.]
+ /* 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.
Of course I haven't tried it, but I think it probably wouldn't be too bad to open-code the relevant parts.
+ /* 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).
Ah right, async_handoff() closes the handle early in the case of synchronous failure. Don't know how I missed that yesterday.
Open-coding sort of obviates this point, though. <shrug>
+ { + 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.
I guess. If that's a concern, it seems potentially clearer to clear the error instead if it's not going to be used by the client.
I.e. if you return the same status as the client gives you, my intuitive reading is "this function returns the status that the client should return, which is normally the same status as the client gives us but might be different in some cases." But then I can't tell what those cases are.
+ 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.
Oh, right, blocking asyncs are a concern, and even for this one...
Returning the wait handle like that does feel weird, and we don't really have a precedent for doing anything like this, but I'll leave it alone I guess.