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() ))