2008/7/18 Ambroz Bizjak ambro@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