On 2/2/22 13:55, Zebediah Figura wrote:
Sorry for being slow to respond...
No worries. Take your time.
I'm honestly split off as to which side should take the responsibility of setting "async->alerted = 1", async_handoff() or notify_async_direct_result. I suppose I'm going with the former, since that's where STATUS_ALERTED is received.
I think that inasmuch as we're mimicking the usual "alerted" code path, it makes conceptual sense to set async->alerted = 1 in async_handoff().
But then, shall the latter have "assert( async->alerted );"...?
Well, you can't do that, exactly, because we can't crash no matter what input we get from the client.
You're right. I just noted that in principle other assert()s are impossible to be triggered by the client.
But you could check for async->alerted and return STATUS_INVALID_PARAMETER or something if it's not set.
I did just that in the v3 of this patch series (but with STATUS_ACCESS_DENIED).
In v4 I realised that it might be useful to let async be cancellable in the future (CancelSynchronousIo is not implemented yet), and the caller is not ready to handle errors from notify_async() anyway. So I decided to just return whatever the eventual status of the async is, whether alerted or not. Not sure how much it would complicate debugging though...
I suppose this is why you mentioned direct_result in [1]—i.e. we could just async_terminate(STATUS_ALERTED) in recv_socket. I can see both approaches working, but I think that open-coding the relevant parts of async_terminate would be better than trying to get direct_result manipulation exactly right.
I agree. I hope we don't also have to also open code "async_handoff()", though.
Of course, I also haven't tried...
[1] https://www.winehq.org/pipermail/wine-devel/2022-January/205774.html
That said, the more I work with the async code, the more I learn to hate all those status bitfields... I think it's much better off merging as many states as possible into a single enum, so that the lifecycle is explicit (e.g. ASYNC_INITIAL -> ASYNC_QUEUED -> ASYNC_ALERTED -> ...) (I'm aware it was much worse before that, making the status field serve the dual role of async state and I/O status.)
Sure, I think that's something worth looking into.