[PATCH v2 1/6] ws2_32/tests: Add tests for setting and getting socket options with shorter length.

[Paul Gofman]

On 3/4/22 20:36, Zebediah Figura wrote:
>
>> +
>> +    /* Test option short length. */
>> +    for (i = 0; i < ARRAY_SIZE(test_optsize); ++i)
>> +    {
>> +        s2 = socket( test_optsize[i].af, test_optsize[i].type, 0 );
>> +        ok(s2 != INVALID_SOCKET, "socket() failed error %d\n", 
>> WSAGetLastError());
>> +        for (j = 0; j < 3; ++j)
>
> ARRAY_SIZE(test_optsize[i].sizes)?
>
> The way this test is structured does not make it very easy to read, 
> but I'm struggling to find a better option. The individual sockopts 
> seem very idiosyncratic...

Yes, those behaviour details don't seem to fit any obvious pattern. 
Maybe I can make it a bit easier if always code the values returned from 
getsockopt() instead of setting the "default" cases in the code?


>
> Finding a way to include TCP_NODELAY in this table, as well as finding 
> a way to test lengths less than 1 and greater than 4, would probably 
> be helpful. I don't know if you have those in your pending patches.

I am introducing TCP_NODELAY in the same table later, yeah. The reason 
that it is not here immediately is that TCP_NODELAY needs output length 
fixup in getsockopt() not to introduce obsucre todo_wine and the remove 
it, thus a separate patch.

I tested greater sizes separately for some options, I will add some 
tests (hopefully they are all work the same as size 4, at least those 
that I tried did). Didn't try length < 0 but I will.


>
>> +        {
>> +            DWORD expected_last_error, expected_value, 
>> expected_size, save_value;
>> +            int expected_err;
>> +
>> +            winetest_push_context("i %u, level %d, optname %d, len %u",
>> +                    i, test_optsize[i].level, 
>> test_optsize[i].optname, 1 << j);
>> +
>> +            size = sizeof(save_value);
>> +            err = getsockopt(s2, test_optsize[i].level, 
>> test_optsize[i].optname, (char*)&save_value, &size);
>> +            ok(!err, "Unexpected getsockopt result %d.\n", err);
>> +
>> +            if (test_optsize[i].accepts_short_len || j == 2)
>
> "j == 2" is not great, maybe "size = 1 << j;" earlier, and then "size 
> == 4" here, would be clearer.
>
>> +            {
>> +                expected_err = 0;
>> +                expected_last_error = ERROR_SUCCESS;
>> +                expected_size = test_optsize[i].sizes[j] ? 
>> test_optsize[i].sizes[j] : 4;
>> +                if (test_optsize[i].values[j])
>> +                    expected_value = test_optsize[i].values[j];
>> +                else if (expected_size == 4)
>> +                    expected_value = 1;
>> +                else
>> +                    expected_value = (0xdeadbeef & ~((1 << 
>> expected_size * 8) - 1)) | 1;
>
> This is not exactly easy to read. Maybe something like the following 
> would be clearer?
>
>     value = 1;
>     expected_value = 0xdeadbeef;
>     memcpy(&expected_value, &value, expected_size);

Yeah, sure.