On 8/11/21 1:02 PM, Alex Henrie wrote:
On Wed, Aug 11, 2021 at 9:18 AM Zebediah Figura (she/her) zfigura@codeweavers.com wrote:
This is a good idea in principle, but a few things bother me about it:
(1) the tables are hard to read (could maybe be solved with indentation)?
I'd be happy to indent the tables, but honestly they would be a lot cleaner if Wine always called SetLastError(WSAENOPROTOOPT) in setsockopt when an option is not recognized. (It already sets WSAENOPROTOOPT for SOL_SOCKET options, just not for network-level options.) Can I just send a patch to do that, or do the error code tests need to be added first?
I think either approach is reasonable; it'd be nice if it can be added to the series so that I can see what the end result looks like up front.
(2) it would probably make sense to split get/set into separate tables, and not to split tcp/udp? I dunno, looks potentially simpler.
(3) It's a bit ambiguous in many cases whether an operation is supported or not. I.e. it'd be nice to have *successful* calls to most of these. At least WSAENOPROTOOPT is relatively unambiguous, but I can't tell if a given operation always returns WSAEINVAL (like, say, the hardcoded IPv4 option 41) or only returns WSAEINVAL because we're not using the right arguments.
Which is to say that maybe a table isn't that great after all :-/
In these tests, WSAEINVAL generally means that an option is supported on TCP but not UDP or vice-versa.
Since the setsockopt tests depend on getting a valid result from getsockopt, how about if we just skip calling setsockopt if we expect getsockopt to fail? That would be a little less comprehensive, but the tables would be a lot shorter.
That seems reasonable to me, but this does break testing set-only options.
Actually now that I see how you're calling setsockopt(), I guess EINVAL is less unconvincing; we probably expect setsockopt() to work with the same data...
Hmm, do we need TODO_OPT? Can't we derive that from the error code?
(4) It's also kind of unclear what /* win7 */ means: broken on win7, or before win7, or up to and including win7, or what?
Like in most other Wine tests, the comment tells the most recent version of Windows where the option doesn't work correctly. For example, /* win7 */ means that the option does not work on Windows 7, but it works on Windows 8 and later.
By the way, the IPV6_RECVTCLASS patch has its own set of tests. Would you approve it if I resend it by itself while we're still working on the more comprehensive tabular tests?
Probably, yes.
Thanks for the feedback,
-Alex