fix async read on mailslots

Rob Shearman robertshearman at gmail.com
Fri Jul 18 07:02:44 CDT 2008


2008/7/18 Ambroz Bizjak <ambro at b4ever.net>:
> Hi,
> In Wine creating a mailslot (CreateMailslot) with non-zero timeout
> (argument 3 to CreateMailslot) and then attempting an asynchronous read on
> it will cause a synchronous read to be performed - ReadFile will block
> instead of failing with ERROR_IO_PENDING.

Good spot, but then the correct fix is to only set
FILE_SYNCHRONOUS_IO_NONALERT on the created fd if the timeout is
non-zero. Asynchronous I/O has a performance penalty in Wine (extra
server round-trips) so we want to avoid it if at all possible.

> I'm attaching a fix as suggested by jil and a test.
> The test creates an empty mailbox with a small timeout and performs an
> asynchronous read - if broken, it will fail with ERROR_SEM_TIMEOUT.
> I've also tried the test on Vista.

It's good that you've included a test, but a few comments:
> +    mailslot = CreateMailslot("\\\\.\\mailslot\\TestMailslot", 100, 10, NULL);
> +    ok(mailslot!=INVALID_HANDLE_VALUE, "CreateMailslot failed\n");
> +    if(mailslot==INVALID_HANDLE_VALUE) return;
> +
> +    event = CreateEvent(NULL, FALSE, FALSE, NULL);
> +    ok(event!=INVALID_HANDLE_VALUE, "CreateEvent failed\n");
> +    if(event==INVALID_HANDLE_VALUE) return;
> +
> +    ol.Offset = 0;
> +    ol.OffsetHigh = 0;
> +    ol.hEvent = event;
> +    res = ReadFile(mailslot, buf, 100, NULL, &ol);
> +    ok(res==FALSE, "ReadFile succeeded; expected failure\n");
> +    if(res!=FALSE) return;
> +
> +    res = GetLastError();
> +    ok(res==ERROR_IO_PENDING, "ReadFile returned error code %d, expected ERROR_IO_PENDING\n", res);

There's no need to return after every unexpected failure. If one of
the tests is likely to fail on some machine due to a configuration
issue or some other feature beyond our control then it is acceptable
to put in a skip statement, but generally we only do this after we've
seen real-world evidence of failures.

You also didn't include an LGPL header for the new file that you added.

Also, in general you should include only one patch per mail to
wine-patches and include an appropriate changelog (I see that you are
using git and there are various tools to do this automatically). It is
helpful if you can submit a test that has appropriate todo_wine's that
show where Wine is failing a test, and then to submit a patch which
does the fix and removes the todo_wine. However, in this case I don't
believe that it is possible so you can send the fix first and then the
tests.

-- 
Rob Shearman



More information about the wine-devel mailing list