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

[Zebediah Figura]

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

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.