[RFC PATCH v4 2/5] server: Add a new server request "notify_async_direct_result."

[Zebediah Figura]

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 at 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 */
> + at 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 */
> + at REPLY
> +    obj_handle_t   handle;        /* wait handle, or NULL if closed */

Why do we need this?

> + at END
> +
> +
>   /* Perform a read on a file object */
>   @REQ(read)
>       async_data_t   async;         /* async I/O parameters */