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

[Jinoh Kang]

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.


-- 
Sincerely,
Jinoh Kang