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

[Jinoh Kang]

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 */
>> + 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?

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.

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


-- 
Sincerely,
Jinoh Kang