[RFC PATCH v4 1/5] server: Allow calling async_handoff() with status code STATUS_ALERTED.

[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 7aef28355f0..e169bb23225 100644
>> --- a/server/async.c
>> +++ b/server/async.c
>> @@ -338,6 +338,30 @@ obj_handle_t async_handoff( struct async *async, data_size_t *result, int force_
>>           return async->wait_handle;
>>       }
>>   +    if (!async->pending && get_error() == STATUS_ALERTED)
> 
> Why "!async->pending"?

The rationale is twofold:

1. async_handoff() can be called with "async->pending = 1".
   Notably the device manager and some sock IOCTLs call set_async_pending() before returning.
   Also, there exists another conditional statement below which explicitly tests for "async->pending".

2. If "async->pending && get_error() == STATUS_ALERTED", the async will be left in an inconsistent state.

   - There exists an established invariant that "async->unknown_status" implies "!async->pending".
     It would be invalid that "async->unknown_status && async->pending", since an async
     "even the initial state" of which "is not known yet" cannot possibly be queued as STATUS_PENDING.

   - Unless "async->direct_result", an APC_ASYNC_IO is required for the async to progress;
     Our code path however issues no APC calls, leaving the async in a stale, hard-to-debug state.

That said, an alternative would be to simply "assert( !async->pending )" inside the new code path.
However, I don't really see a reason to reject STATUS_ALERTED on a pending async; we can let it
simply follow the normal path, allowing the async to be terminated with STATUS_ALERTED as usual.
I think this best sticks to the principle of least surprise.  It's entirely my own perspective, though...

> 
>> +    {
>> +        /* Give the client opportunity to complete synchronously.
>> +         * If it turns out that the I/O request is not actually immediately satiable,
>> +         * the client may then choose to re-queue the async (with STATUS_PENDING).
> 
> I'd explicitly mention *how* the async should be requeued here.

I worded it that way since each I/O operation can have their own way to manage their async queue
(e.g. socket operations would use sock->read_q/write_q, device managers would use its own IRP queue,
while others may just use fd->read_q/write_q).  I chose to retain the abstractness so that the comment would't get easily stale or out of date.

That said, I suppose you specifically meant mentioning how to _transition_ the async to STATUS_PENDING.
I agree with you in that case.  I'll add a mention of the new wineserver call here.

> 
>> +         */
>> +        async->unknown_status = 1;
>> +
>> +        /* Deactivate async so that it can be safely queued.
>> +         * Otherwise, the async will be erroneously alerted the next time
>> +         * someone calls async_wake_up() on the queue.
> 
> Not to put too much weight on my own perspective, but I think it's clearer to explain *why* to do something, rather than why not.

By "why not" did you meant the "otherwise" part?

> In this case, what we're doing is mimicking a normal "alert" case, except that we're not using an APC.

If I understood correctly, I should just say instead:
> Emulate calling async_terminate() with STATUS_ALERTED (request client to complete I/O),
> but don't actually queue APC_ASYNC_IO under any circumstances.
Is this correct?

> 
>> +         *
>> +         * Don't use async_terminate() here since we'd like to leave the IOSB
>> +         * status as STATUS_PENDING.  
> 
> Wait, why is this important?

Oops, looks like a leftover from v3.  My apologies...
(In v3 it was required for making async_handoff() behave on async "restart" case and properly return STATUS_PENDING; otherwise, it would notice the STATUS_ALERTED in async->iosb->status and return just that, ignoring our request to set the status to STATUS_PENDING and restart the async.)

> 
>> Also we certainly don't want APC_ASYNC_IO
>> +         * to fire in any circumstances.
>> +         */
>> +        async->terminated = 1;
>> +        async->alerted = 1;
>> +
>> +        async_reselect( async );
>> +
>> +        return async->wait_handle;
>> +    }
>> +
>>       async->initial_status = get_error();
>>         if (!async->pending && NT_ERROR( get_error() ))

-- 
Sincerely,
Jinoh Kang