[PATCH 2/2 resent] ws2_32: Add tests for exclusive flag for IOCTL_AFD_POLL.

[Zebediah Figura (she/her)]

On 9/2/21 12:08 PM, Guillaume Charifi-Hoareau wrote:
> Thank you for your answer!
> 
> 
> I spent a lot of time testing a whole lot of different combinations and the results still
> confused me...
> 
> 
>> There's also a part of me that would like to know what can possibly be
>> the point of this API; it's hard to even think of what "reasonable"
>> behaviour is otherwise. Do you happen to know?
>>
>> My best guess is that it's meant to explicitly interrupt poll requests
>> somehow (what, I/O cancellation wasn't good enough for them?), in which
>> case it'd be good to explicitly confirm or refute that the "exclusive"
>> poll really does act like a normal poll, and doesn't e.g. return
>> immediately.
> 
> I discovered this behavior while investigating an app that seemed to rely on them to
> interrupt some polls, and so far it seems to be its sole purpose.
> There is only one reason I can imagine a use for this instead of CancelIo(): As the
> NtDeviceIoControlFile()'s file handle is independent of the socket in params, it provides a
> way to interrupt another poll that waits on the same socket without the need to know the
> file handle that was originally given to it.
>   
> Regardless its behaviour is kind of crazy.
> I tried to restrict to a subset of the tests to avoid polluting test_poll() too much, but indeed
> it requires an entire function to test it properly.
> 
> 
>> I think these tests could be more extensive. For example:
>>
>> * What happens if you try to perform a non-exclusive wait while an
>> exclusive wait is in progress? i.e. the inverse of the test added here.
> So after some extensive testing, I'm rather confident that the logic for a new poll on a
> socket is:
> (The main poll is an arbitrary designation for one of the poll waiting on the socket)
> If there is no main poll on the socket, then the new poll becomes the main poll.
> If the main poll is terminated, then there is no main poll anymore.
> If the main poll is exclusive and the new poll is exclusive, then the main poll is terminated
> and the new poll becomes the main poll.
> 
> 
> The flag does not seem to do anything more than this (apart from requiring
> 0x7fffffff00000000 <= timeout <= 0x7fffffffffffffff).

Wow. That is some of the bizarrest behaviour I've seen. I'd call it a 
bug, but I can't even figure out what behaviour they were *trying* to 
implement.

I kept reading through your test, trying to describe the behaviour in 
simpler (or saner) terms, and failing. So congratulations, I think 
you've hit the nail on the head :D

>>
>> * Can we verify that exclusive waits are specific to a given socket
>> (i.e. you can simultaneously do an exclusive wait on two different sockets?)
> 
> Yes, the mechanism described earlier seems to be done on a per-socket basis.
>>
>> * Can we test that exclusive waits apply if two unequal sets overlap?
>>
> 
> 
> Yes, it works the same way (a single common socket between exclusive polls is enough to
> wake the poll)
> 
> 
>> * Can you do an exclusive wait on the same socket from two different
>> threads?
> 
> 
> Can yes, didn't want to, but did it anyway :)
> I get the expected result: It works exactly the same as the single threaded version.
> 
> So I guess I now have enough insight to write a proper implementation.
> The patch for the tests is attached to this job: https://testbot.winehq.org/JobDetails.pl?
> Key=97234[1]
> Feel free to tell me if other tests come to your mind.

Yeah, that looks about right, nothing else comes to mind now.

In terms of some informal review on those, I saw that one and the one 
with the comments changed. I think in both cases the comments are kind 
of redundant—reading the code tells me as much and is actually kind of 
easier. Where comments could help (and the way I usually tend to write 
them) is in listing the conclusions that each following chunk of code 
actually proves. That's a bit vague, but I think it would help make the 
tests make sense a bit more readily.

Also, a nitpick—there's a C99 variable declaration in the for loop 
initializer; unfortunately we have to avoid those.