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

[Zebediah Figura]

On 2/3/22 09:28, Jinoh Kang wrote:
> 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".

Yes, but they don't set the last error to STATUS_ALERTED, do they?

Anything that's triggered below should only apply to asyncs whose 
initial status is known. Consider that "unknown_status" not only means 
the initial NTSTATUS is unknown, but it also means the initial pending 
state is unknown.

> 
> 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...

If you consider the above, "async->pending" is semantically undefined if 
async->unknown_status is 1. We shouldn't depend on it being one thing or 
the other, and checking for that (even in an assert) I think gives the 
wrong idea that we care.

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

Yeah, that's what I meant :-)

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

Something more like that, yeah. Although, to contradict my earlier 
assertion, I think it'd also help to explain what's wrong with using 
APC_ASYNC_IO here.