This patch sequence fixes Wolfenstein II, Bug 43935.
Signed-off-by: Andrew Eikum aeikum@codeweavers.com --- dlls/kernel32/tests/file.c | 53 +++++++++++++++++++++++++++++++++++++++------- dlls/ntdll/file.c | 2 +- 2 files changed, 46 insertions(+), 9 deletions(-)
diff --git a/dlls/kernel32/tests/file.c b/dlls/kernel32/tests/file.c index 0b8cef23f8..8f5f392822 100644 --- a/dlls/kernel32/tests/file.c +++ b/dlls/kernel32/tests/file.c @@ -4353,7 +4353,8 @@ static void test_WriteFileGather(void) FILE_SEGMENT_ELEMENT fse[2]; OVERLAPPED ovl, *povl = NULL; SYSTEM_INFO si; - LPVOID buf = NULL; + LPVOID wbuf = NULL, rbuf1; + BOOL br;
ret = GetTempPathA( MAX_PATH, temp_path ); ok( ret != 0, "GetTempPathA error %d\n", GetLastError() ); @@ -4371,12 +4372,16 @@ static void test_WriteFileGather(void) ok( hiocp2 != 0, "CreateIoCompletionPort failed err %u\n", GetLastError() );
GetSystemInfo( &si ); - buf = VirtualAlloc( NULL, si.dwPageSize, MEM_COMMIT, PAGE_READWRITE ); - ok( buf != NULL, "VirtualAlloc failed err %u\n", GetLastError() ); + wbuf = VirtualAlloc( NULL, si.dwPageSize, MEM_COMMIT, PAGE_READWRITE ); + ok( wbuf != NULL, "VirtualAlloc failed err %u\n", GetLastError() ); + + rbuf1 = VirtualAlloc( NULL, si.dwPageSize, MEM_COMMIT, PAGE_READWRITE ); + ok( rbuf1 != NULL, "VirtualAlloc failed err %u\n", GetLastError() );
memset( &ovl, 0, sizeof(ovl) ); memset( fse, 0, sizeof(fse) ); - fse[0].Buffer = buf; + memset( wbuf, 0x42, si.dwPageSize ); + fse[0].Buffer = wbuf; if (!WriteFileGather( hfile, fse, si.dwPageSize, NULL, &ovl )) ok( GetLastError() == ERROR_IO_PENDING, "WriteFileGather failed err %u\n", GetLastError() );
@@ -4384,20 +4389,52 @@ static void test_WriteFileGather(void) ok( ret, "GetQueuedCompletionStatus failed err %u\n", GetLastError()); ok( povl == &ovl, "wrong ovl %p\n", povl );
+ /* read exact size */ memset( &ovl, 0, sizeof(ovl) ); memset( fse, 0, sizeof(fse) ); - fse[0].Buffer = buf; - if (!ReadFileScatter( hfile, fse, si.dwPageSize, NULL, &ovl )) - ok( GetLastError() == ERROR_IO_PENDING, "ReadFileScatter failed err %u\n", GetLastError() ); + fse[0].Buffer = rbuf1; + memset( rbuf1, 0, si.dwPageSize ); + br = ReadFileScatter( hfile, fse, si.dwPageSize, NULL, &ovl ); + ok( br == FALSE, "ReadFileScatter should be asynchronous\n" ); + ok( GetLastError() == ERROR_IO_PENDING, "ReadFileScatter failed err %u\n", GetLastError() );
ret = GetQueuedCompletionStatus( hiocp2, &size, &key, &povl, 1000 ); ok( ret, "GetQueuedCompletionStatus failed err %u\n", GetLastError()); ok( povl == &ovl, "wrong ovl %p\n", povl );
+ ok( memcmp( rbuf1, wbuf, si.dwPageSize ) == 0, + "data was not read into buffer\n" ); + + /* start read at EOF */ + memset( &ovl, 0, sizeof(ovl) ); + S(U(ovl)).OffsetHigh = 0; + S(U(ovl)).Offset = si.dwPageSize; + memset( fse, 0, sizeof(fse) ); + fse[0].Buffer = rbuf1; + br = ReadFileScatter( hfile, fse, si.dwPageSize, NULL, &ovl ); + ok( br == FALSE, "ReadFileScatter should have failed\n" ); + ok( GetLastError() == ERROR_HANDLE_EOF || + GetLastError() == ERROR_IO_PENDING, "ReadFileScatter gave wrong error %u\n", GetLastError() ); + if (GetLastError() == ERROR_IO_PENDING) + { + ret = GetQueuedCompletionStatus( hiocp2, &size, &key, &povl, 1000 ); + ok( !ret, "GetQueuedCompletionStatus should have returned failure\n" ); + ok( GetLastError() == ERROR_HANDLE_EOF, "Got wrong error: %u\n", GetLastError() ); + ok( povl == &ovl, "wrong ovl %p\n", povl ); + } + else + { + ret = GetQueuedCompletionStatus( hiocp2, &size, &key, &povl, 100 ); + ok( !ret, "GetQueuedCompletionStatus failed err %u\n", GetLastError() ); + ok( GetLastError() == WAIT_TIMEOUT, "GetQueuedCompletionStatus gave wrong error %u\n", GetLastError() ); + ok( povl == NULL, "wrong ovl %p\n", povl ); + } + CloseHandle( hfile ); CloseHandle( hiocp1 ); CloseHandle( hiocp2 ); - VirtualFree( buf, 0, MEM_RELEASE ); + VirtualFree( wbuf, 0, MEM_RELEASE ); + VirtualFree( rbuf1, 0, MEM_RELEASE ); DeleteFileA( filename ); }
diff --git a/dlls/ntdll/file.c b/dlls/ntdll/file.c index ca2afa0e89..12b1d0f6c5 100644 --- a/dlls/ntdll/file.c +++ b/dlls/ntdll/file.c @@ -1105,7 +1105,7 @@ NTSTATUS WINAPI NtReadFileScatter( HANDLE file, HANDLE event, PIO_APC_ROUTINE ap
if (send_completion) NTDLL_AddCompletion( file, cvalue, status, total );
- return status; + return STATUS_PENDING; }
Hi Andrew,
On 30.11.2017 18:23, Andrew Eikum wrote:
diff --git a/dlls/ntdll/file.c b/dlls/ntdll/file.c index ca2afa0e89..12b1d0f6c5 100644 --- a/dlls/ntdll/file.c +++ b/dlls/ntdll/file.c @@ -1105,7 +1105,7 @@ NTSTATUS WINAPI NtReadFileScatter( HANDLE file, HANDLE event, PIO_APC_ROUTINE ap
if (send_completion) NTDLL_AddCompletion( file, cvalue, status, total );
- return status;
- return STATUS_PENDING;
Are you sure it should always be a case? Your tests use overlaped file handle, I guess that's what makes differences. At least that's what I've seen with other similar APIs (eg. recent Windows versions always return STATUS_PENDING for ioctls if handle is overlapped).
Thanks,
Jacek
On Thu, Nov 30, 2017 at 07:59:37PM +0100, Jacek Caban wrote:
Hi Andrew,
On 30.11.2017 18:23, Andrew Eikum wrote:
diff --git a/dlls/ntdll/file.c b/dlls/ntdll/file.c index ca2afa0e89..12b1d0f6c5 100644 --- a/dlls/ntdll/file.c +++ b/dlls/ntdll/file.c @@ -1105,7 +1105,7 @@ NTSTATUS WINAPI NtReadFileScatter( HANDLE file, HANDLE event, PIO_APC_ROUTINE ap
if (send_completion) NTDLL_AddCompletion( file, cvalue, status, total );
- return status;
- return STATUS_PENDING;
Are you sure it should always be a case? Your tests use overlaped file handle, I guess that's what makes differences. At least that's what I've seen with other similar APIs (eg. recent Windows versions always return STATUS_PENDING for ioctls if handle is overlapped).
I thought it was weird that this behaved different from other ntdll functions. I hadn't thought of the file handle type. I'll try with a non-overlapped file and see what changes.
Thanks, Andrew
On Thu, Nov 30, 2017 at 01:16:35PM -0600, Andrew Eikum wrote:
On Thu, Nov 30, 2017 at 07:59:37PM +0100, Jacek Caban wrote:
Hi Andrew,
On 30.11.2017 18:23, Andrew Eikum wrote:
diff --git a/dlls/ntdll/file.c b/dlls/ntdll/file.c index ca2afa0e89..12b1d0f6c5 100644 --- a/dlls/ntdll/file.c +++ b/dlls/ntdll/file.c @@ -1105,7 +1105,7 @@ NTSTATUS WINAPI NtReadFileScatter( HANDLE file, HANDLE event, PIO_APC_ROUTINE ap
if (send_completion) NTDLL_AddCompletion( file, cvalue, status, total );
- return status;
- return STATUS_PENDING;
Are you sure it should always be a case? Your tests use overlaped file handle, I guess that's what makes differences. At least that's what I've seen with other similar APIs (eg. recent Windows versions always return STATUS_PENDING for ioctls if handle is overlapped).
I thought it was weird that this behaved different from other ntdll functions. I hadn't thought of the file handle type. I'll try with a non-overlapped file and see what changes.
Actually, MSDN says the handle must be created with FILE_FLAG_OVERLAPPED, and a quick test confirms that.
Andrew
Andrew Eikum aeikum@codeweavers.com wrote:
- /* start read at EOF */
- memset( &ovl, 0, sizeof(ovl) );
- S(U(ovl)).OffsetHigh = 0;
- S(U(ovl)).Offset = si.dwPageSize;
- memset( fse, 0, sizeof(fse) );
- fse[0].Buffer = rbuf1;
- br = ReadFileScatter( hfile, fse, si.dwPageSize, NULL, &ovl );
- ok( br == FALSE, "ReadFileScatter should have failed\n" );
- ok( GetLastError() == ERROR_HANDLE_EOF ||
GetLastError() == ERROR_IO_PENDING, "ReadFileScatter gave wrong error %u\n", GetLastError() );
A test needs to reset last error before an API call if the error code is going to be checked after that. Existing tests also fail to do this, so they should be fixed as well.
On Fri, Dec 01, 2017 at 10:18:19AM +0800, Dmitry Timoshkov wrote:
Andrew Eikum aeikum@codeweavers.com wrote:
- /* start read at EOF */
- memset( &ovl, 0, sizeof(ovl) );
- S(U(ovl)).OffsetHigh = 0;
- S(U(ovl)).Offset = si.dwPageSize;
- memset( fse, 0, sizeof(fse) );
- fse[0].Buffer = rbuf1;
- br = ReadFileScatter( hfile, fse, si.dwPageSize, NULL, &ovl );
- ok( br == FALSE, "ReadFileScatter should have failed\n" );
- ok( GetLastError() == ERROR_HANDLE_EOF ||
GetLastError() == ERROR_IO_PENDING, "ReadFileScatter gave wrong error %u\n", GetLastError() );A test needs to reset last error before an API call if the error code is going to be checked after that. Existing tests also fail to do this, so they should be fixed as well.
Thanks for the review. I'll add that to the existing tests, and onto the new ones.
Andrew
-
Andrew Eikum -
Dmitry Timoshkov -
Jacek Caban