[PATCH v2 1/5] kernel32/tests: Test ReadFileScatter() completion status with FILE_SKIP_COMPLETION_PORT_ON_SUCCESS.
Signed-off-by: Paul Gofman gofmanp@gmail.com --- v2: - no changes.
dlls/kernel32/tests/file.c | 27 +++++++++++++++++++++++++++ 1 file changed, 27 insertions(+)
diff --git a/dlls/kernel32/tests/file.c b/dlls/kernel32/tests/file.c index 18cd18d154..a4fba43242 100644 --- a/dlls/kernel32/tests/file.c +++ b/dlls/kernel32/tests/file.c @@ -61,6 +61,7 @@ static BOOL (WINAPI *pSetFileInformationByHandle)(HANDLE, FILE_INFO_BY_HANDLE_CL static BOOL (WINAPI *pGetQueuedCompletionStatusEx)(HANDLE, OVERLAPPED_ENTRY*, ULONG, ULONG*, DWORD, BOOL); static void (WINAPI *pRtlInitAnsiString)(PANSI_STRING,PCSZ); static void (WINAPI *pRtlFreeUnicodeString)(PUNICODE_STRING); +static BOOL (WINAPI *pSetFileCompletionNotificationModes)(HANDLE, UCHAR);
static char filename[MAX_PATH]; static const char sillytext[] = @@ -109,6 +110,7 @@ static void InitFunctionPointers(void) pGetFinalPathNameByHandleW = (void *) GetProcAddress(hkernel32, "GetFinalPathNameByHandleW"); pSetFileInformationByHandle = (void *) GetProcAddress(hkernel32, "SetFileInformationByHandle"); pGetQueuedCompletionStatusEx = (void *) GetProcAddress(hkernel32, "GetQueuedCompletionStatusEx"); + pSetFileCompletionNotificationModes = (void *)GetProcAddress(hkernel32, "SetFileCompletionNotificationModes"); }
static void test__hread( void ) @@ -4565,6 +4567,31 @@ static void test_WriteFileGather(void) ok( memcmp( rbuf1 + si.dwPageSize / 2, rbuf2, si.dwPageSize - si.dwPageSize / 2 ) == 0, "invalid data was read into buffer\n" );
+ if (pSetFileCompletionNotificationModes) + { + br = pSetFileCompletionNotificationModes(hfile, FILE_SKIP_COMPLETION_PORT_ON_SUCCESS); + ok(br, "SetFileCompletionNotificationModes failed, error %u.\n", GetLastError()); + + br = ReadFileScatter(hfile, fse, si.dwPageSize, NULL, &ovl); + ok(br == FALSE, "ReadFileScatter should be asynchronous.\n"); + ok(GetLastError() == ERROR_IO_PENDING, "ReadFileScatter failed, error %u.\n", GetLastError()); + + br = GetQueuedCompletionStatus(hiocp2, &size, &key, &povl, 1000); + todo_wine ok(br, "GetQueuedCompletionStatus failed, err %u.\n", GetLastError()); + todo_wine ok(povl == &ovl, "Wrong ovl %p.\n", povl); + + br = GetOverlappedResult(hfile, &ovl, &tx, TRUE); + ok(br, "GetOverlappedResult failed, err %u.\n", GetLastError()); + ok(tx == si.dwPageSize, "Got unexpected size %u.\n", tx); + + ResetEvent(evt); + + br = pSetFileCompletionNotificationModes(hfile, 0); + ok(br, "SetFileCompletionNotificationModes failed, error %u.\n", GetLastError()); + } + else + win_skip("SetFileCompletionNotificationModes not available.\n"); + CloseHandle( hfile ); CloseHandle( hiocp1 ); CloseHandle( hiocp2 );
Signed-off-by: Paul Gofman gofmanp@gmail.com --- v2: - no changes.
dlls/ntdll/tests/file.c | 35 +++++++++++++++++++++++++++++++++++ 1 file changed, 35 insertions(+)
diff --git a/dlls/ntdll/tests/file.c b/dlls/ntdll/tests/file.c index e210a6e280..1c1143fdab 100644 --- a/dlls/ntdll/tests/file.c +++ b/dlls/ntdll/tests/file.c @@ -3403,6 +3403,7 @@ static void _test_completion_flags(unsigned line, HANDLE handle, DWORD expected_
static void test_file_completion_information(void) { + DECLSPEC_ALIGN(TEST_OVERLAPPED_READ_SIZE) static unsigned char aligned_buf[TEST_OVERLAPPED_READ_SIZE]; static const char buf[] = "testdata"; FILE_IO_COMPLETION_NOTIFICATION_INFORMATION info; OVERLAPPED ov, *pov; @@ -3546,6 +3547,40 @@ static void test_file_completion_information(void) else win_skip("WriteFile never returned TRUE\n");
+ CloseHandle(port); + CloseHandle(h); + + if (!(h = create_temp_file(FILE_FLAG_OVERLAPPED | FILE_FLAG_NO_BUFFERING))) + return; + + port = CreateIoCompletionPort(h, NULL, 0xdeadbeef, 0); + ok(port != NULL, "CreateIoCompletionPort failed, error %u.\n", GetLastError()); + + info.Flags = FILE_SKIP_COMPLETION_PORT_ON_SUCCESS; + status = pNtSetInformationFile(h, &io, &info, sizeof(info), FileIoCompletionNotificationInformation); + ok(status == STATUS_SUCCESS, "Expected STATUS_SUCCESS, got %#x.\n", status); + test_completion_flags(h, FILE_SKIP_COMPLETION_PORT_ON_SUCCESS); + + ret = WriteFile(h, aligned_buf, sizeof(aligned_buf), &num_bytes, &ov); + if (!ret && GetLastError() == ERROR_IO_PENDING) + { + ret = GetOverlappedResult(h, &ov, &num_bytes, TRUE); + ok(ret, "GetOverlappedResult failed, error %u.\n", GetLastError()); + ok(num_bytes == sizeof(aligned_buf), "expected sizeof(aligned_buf), got %u.\n", num_bytes); + ret = GetQueuedCompletionStatus(port, &num_bytes, &key, &pov, 1000); + ok(ret, "GetQueuedCompletionStatus failed, error %u.\n", GetLastError()); + } + ok(num_bytes == sizeof(aligned_buf), "expected sizeof(buf), got %u.\n", num_bytes); + + SetLastError(0xdeadbeef); + ret = ReadFile(h, aligned_buf, sizeof(aligned_buf), &num_bytes, &ov); + ok(!ret && GetLastError() == ERROR_IO_PENDING, "Unexpected result, ret %#x, error %u.\n", + ret, GetLastError()); + ret = GetOverlappedResult(h, &ov, &num_bytes, TRUE); + ok(ret, "GetOverlappedResult failed, error %u.\n", GetLastError()); + ret = GetQueuedCompletionStatus(port, &num_bytes, &key, &pov, 1000); + todo_wine ok(ret, "GetQueuedCompletionStatus failed, error %u.\n", GetLastError()); + CloseHandle(ov.hEvent); CloseHandle(port); CloseHandle(h);
Signed-off-by: Paul Gofman gofmanp@gmail.com --- v2: - Pass real status (not ret_status) to NTDLL_AddCompletion() in NtReadFile().
dlls/kernel32/tests/file.c | 4 ++-- dlls/ntdll/file.c | 17 +++++++++-------- dlls/ntdll/ntdll_misc.h | 2 +- dlls/ntdll/sync.c | 3 ++- dlls/ntdll/tests/file.c | 2 +- 5 files changed, 15 insertions(+), 13 deletions(-)
diff --git a/dlls/kernel32/tests/file.c b/dlls/kernel32/tests/file.c index a4fba43242..77ae140023 100644 --- a/dlls/kernel32/tests/file.c +++ b/dlls/kernel32/tests/file.c @@ -4577,8 +4577,8 @@ static void test_WriteFileGather(void) ok(GetLastError() == ERROR_IO_PENDING, "ReadFileScatter failed, error %u.\n", GetLastError());
br = GetQueuedCompletionStatus(hiocp2, &size, &key, &povl, 1000); - todo_wine ok(br, "GetQueuedCompletionStatus failed, err %u.\n", GetLastError()); - todo_wine ok(povl == &ovl, "Wrong ovl %p.\n", povl); + ok(br, "GetQueuedCompletionStatus failed, err %u.\n", GetLastError()); + ok(povl == &ovl, "Wrong ovl %p.\n", povl);
br = GetOverlappedResult(hfile, &ovl, &tx, TRUE); ok(br, "GetOverlappedResult failed, err %u.\n", GetLastError()); diff --git a/dlls/ntdll/file.c b/dlls/ntdll/file.c index 1cf3ae2d05..0380544459 100644 --- a/dlls/ntdll/file.c +++ b/dlls/ntdll/file.c @@ -831,7 +831,7 @@ NTSTATUS WINAPI NtReadFile(HANDLE hFile, HANDLE hEvent, int result, unix_handle, needs_close; unsigned int options; struct io_timeouts timeouts; - NTSTATUS status; + NTSTATUS status, ret_status; ULONG total = 0; enum server_fd_type type; ULONG_PTR cvalue = apc ? 0 : (ULONG_PTR)apc_user; @@ -1013,10 +1013,11 @@ err: if (status != STATUS_PENDING && hEvent) NtResetEvent( hEvent, NULL ); }
- if (send_completion) NTDLL_AddCompletion( hFile, cvalue, status, total ); - if (async_read && (options & FILE_NO_INTERMEDIATE_BUFFERING) && status == STATUS_SUCCESS) - return STATUS_PENDING; - return status; + ret_status = async_read && (options & FILE_NO_INTERMEDIATE_BUFFERING) && status == STATUS_SUCCESS + ? STATUS_PENDING : status; + + if (send_completion) NTDLL_AddCompletion( hFile, cvalue, status, total, ret_status == STATUS_PENDING ); + return ret_status; }
@@ -1089,7 +1090,7 @@ NTSTATUS WINAPI NtReadFileScatter( HANDLE file, HANDLE event, PIO_APC_ROUTINE ap if (event) NtSetEvent( event, NULL ); if (apc) NtQueueApcThread( GetCurrentThread(), (PNTAPCFUNC)apc, (ULONG_PTR)apc_user, (ULONG_PTR)io_status, 0 ); - if (send_completion) NTDLL_AddCompletion( file, cvalue, status, total ); + if (send_completion) NTDLL_AddCompletion( file, cvalue, status, total, TRUE );
return STATUS_PENDING;
@@ -1408,7 +1409,7 @@ err: if (status != STATUS_PENDING && hEvent) NtResetEvent( hEvent, NULL ); }
- if (send_completion) NTDLL_AddCompletion( hFile, cvalue, status, total ); + if (send_completion) NTDLL_AddCompletion( hFile, cvalue, status, total, status == STATUS_PENDING );
return status; } @@ -1500,7 +1501,7 @@ NTSTATUS WINAPI NtWriteFileGather( HANDLE file, HANDLE event, PIO_APC_ROUTINE ap if (status != STATUS_PENDING && event) NtResetEvent( event, NULL ); }
- if (send_completion) NTDLL_AddCompletion( file, cvalue, status, total ); + if (send_completion) NTDLL_AddCompletion( file, cvalue, status, total, status == STATUS_PENDING );
return status; } diff --git a/dlls/ntdll/ntdll_misc.h b/dlls/ntdll/ntdll_misc.h index 6a54fafc7e..084c5123c3 100644 --- a/dlls/ntdll/ntdll_misc.h +++ b/dlls/ntdll/ntdll_misc.h @@ -197,7 +197,7 @@ extern BYTE* CDECL __wine_user_shared_data(void);
/* completion */ extern NTSTATUS NTDLL_AddCompletion( HANDLE hFile, ULONG_PTR CompletionValue, - NTSTATUS CompletionStatus, ULONG Information ) DECLSPEC_HIDDEN; + NTSTATUS CompletionStatus, ULONG Information, BOOL async) DECLSPEC_HIDDEN;
/* code pages */ extern int ntdll_umbstowcs(DWORD flags, const char* src, int srclen, WCHAR* dst, int dstlen) DECLSPEC_HIDDEN; diff --git a/dlls/ntdll/sync.c b/dlls/ntdll/sync.c index 3293c52859..cc70fde2b3 100644 --- a/dlls/ntdll/sync.c +++ b/dlls/ntdll/sync.c @@ -1529,7 +1529,7 @@ NTSTATUS WINAPI NtQueryIoCompletion( HANDLE CompletionPort, IO_COMPLETION_INFORM }
NTSTATUS NTDLL_AddCompletion( HANDLE hFile, ULONG_PTR CompletionValue, - NTSTATUS CompletionStatus, ULONG Information ) + NTSTATUS CompletionStatus, ULONG Information, BOOL async ) { NTSTATUS status;
@@ -1539,6 +1539,7 @@ NTSTATUS NTDLL_AddCompletion( HANDLE hFile, ULONG_PTR CompletionValue, req->cvalue = CompletionValue; req->status = CompletionStatus; req->information = Information; + req->async = async; status = wine_server_call( req ); } SERVER_END_REQ; diff --git a/dlls/ntdll/tests/file.c b/dlls/ntdll/tests/file.c index 1c1143fdab..3320ccecfa 100644 --- a/dlls/ntdll/tests/file.c +++ b/dlls/ntdll/tests/file.c @@ -3579,7 +3579,7 @@ static void test_file_completion_information(void) ret = GetOverlappedResult(h, &ov, &num_bytes, TRUE); ok(ret, "GetOverlappedResult failed, error %u.\n", GetLastError()); ret = GetQueuedCompletionStatus(port, &num_bytes, &key, &pov, 1000); - todo_wine ok(ret, "GetQueuedCompletionStatus failed, error %u.\n", GetLastError()); + ok(ret, "GetQueuedCompletionStatus failed, error %u.\n", GetLastError());
CloseHandle(ov.hEvent); CloseHandle(port);
Hi Paul,
The patch looks mostly good for me.
On 2/20/19 6:33 PM, Paul Gofman wrote:
- if (send_completion) NTDLL_AddCompletion( hFile, cvalue, status, total );
- if (async_read && (options & FILE_NO_INTERMEDIATE_BUFFERING) && status == STATUS_SUCCESS)
return STATUS_PENDING;- return status;
- ret_status = async_read && (options & FILE_NO_INTERMEDIATE_BUFFERING) && status == STATUS_SUCCESS
? STATUS_PENDING : status;- if (send_completion) NTDLL_AddCompletion( hFile, cvalue, status, total, ret_status == STATUS_PENDING );
- return ret_status; }
Do you really need a separated ret_status variable? It seems that you could just set status to STATUS_PENDING instead.
@@ -1089,7 +1090,7 @@ NTSTATUS WINAPI NtReadFileScatter( HANDLE file, HANDLE event, PIO_APC_ROUTINE ap if (event) NtSetEvent( event, NULL ); if (apc) NtQueueApcThread( GetCurrentThread(), (PNTAPCFUNC)apc, (ULONG_PTR)apc_user, (ULONG_PTR)io_status, 0 );
- if (send_completion) NTDLL_AddCompletion( file, cvalue, status, total );
if (send_completion) NTDLL_AddCompletion( file, cvalue, status, total, TRUE );
return STATUS_PENDING;
@@ -1408,7 +1409,7 @@ err: if (status != STATUS_PENDING && hEvent) NtResetEvent( hEvent, NULL ); }
- if (send_completion) NTDLL_AddCompletion( hFile, cvalue, status, total );
- if (send_completion) NTDLL_AddCompletion( hFile, cvalue, status, total, status == STATUS_PENDING );
Unless I'm missing, send_completion will never be set if status == STATUS_PENDING in this case, so you could just pass FALSE.
return status;} @@ -1500,7 +1501,7 @@ NTSTATUS WINAPI NtWriteFileGather( HANDLE file, HANDLE event, PIO_APC_ROUTINE ap if (status != STATUS_PENDING && event) NtResetEvent( event, NULL ); }
- if (send_completion) NTDLL_AddCompletion( file, cvalue, status, total );
- if (send_completion) NTDLL_AddCompletion( file, cvalue, status, total, status == STATUS_PENDING );
Same as above.
Thanks,
Jacek
Hello Jacek,
thanks for the review.
On 2/20/19 6:33 PM, Paul Gofman wrote:
- if (send_completion) NTDLL_AddCompletion( hFile, cvalue, status, total );
- if (async_read && (options & FILE_NO_INTERMEDIATE_BUFFERING) && status == STATUS_SUCCESS)
return STATUS_PENDING;- return status;
- ret_status = async_read && (options & FILE_NO_INTERMEDIATE_BUFFERING) && status == STATUS_SUCCESS
? STATUS_PENDING : status;- if (send_completion) NTDLL_AddCompletion( hFile, cvalue, status, total, ret_status == STATUS_PENDING );
- return ret_status; }
Do you really need a separated ret_status variable? It seems that you could just set status to STATUS_PENDING instead.
This way I will be passing STATUS_PENDING instead of STATUS_SUCCESS to NTDLL_AddCompletion() status parameter. This looks like a separate change which I tried to avoid. Or do you think it is ok to do that?
@@ -1089,7 +1090,7 @@ NTSTATUS WINAPI NtReadFileScatter( HANDLE file, HANDLE event, PIO_APC_ROUTINE ap if (event) NtSetEvent( event, NULL ); if (apc) NtQueueApcThread( GetCurrentThread(), (PNTAPCFUNC)apc, (ULONG_PTR)apc_user, (ULONG_PTR)io_status, 0 );
- if (send_completion) NTDLL_AddCompletion( file, cvalue, status, total );
if (send_completion) NTDLL_AddCompletion( file, cvalue, status, total, TRUE );
return STATUS_PENDING;
@@ -1408,7 +1409,7 @@ err: if (status != STATUS_PENDING && hEvent) NtResetEvent( hEvent, NULL ); }
- if (send_completion) NTDLL_AddCompletion( hFile, cvalue, status, total );
- if (send_completion) NTDLL_AddCompletion( hFile, cvalue, status, total, status == STATUS_PENDING );
Unless I'm missing, send_completion will never be set if status == STATUS_PENDING in this case, so you could just pass FALSE.
Yeah, looks so, I will change that.
On 2/20/19 6:59 PM, Paul Gofman wrote:
Hello Jacek,
thanks for the review.
On 2/20/19 6:33 PM, Paul Gofman wrote:
- if (send_completion) NTDLL_AddCompletion( hFile, cvalue, status, total );
- if (async_read && (options & FILE_NO_INTERMEDIATE_BUFFERING) && status == STATUS_SUCCESS)
return STATUS_PENDING;- return status;
- ret_status = async_read && (options & FILE_NO_INTERMEDIATE_BUFFERING) && status == STATUS_SUCCESS
? STATUS_PENDING : status;- if (send_completion) NTDLL_AddCompletion( hFile, cvalue, status, total, ret_status == STATUS_PENDING );
- return ret_status; }
Do you really need a separated ret_status variable? It seems that you could just set status to STATUS_PENDING instead.
This way I will be passing STATUS_PENDING instead of STATUS_SUCCESS to NTDLL_AddCompletion() status parameter. This looks like a separate change which I tried to avoid. Or do you think it is ok to do that?
Right, I missed that. Separate variable is fine then.
Thanks,
Jacek
Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=42982
This matches Vista+ behaviour.
Signed-off-by: Paul Gofman gofmanp@gmail.com --- v2: - no changes.
dlls/ntdll/file.c | 7 ++++--- dlls/ntdll/tests/file.c | 6 +++--- 2 files changed, 7 insertions(+), 6 deletions(-)
diff --git a/dlls/ntdll/file.c b/dlls/ntdll/file.c index 0380544459..04b2432875 100644 --- a/dlls/ntdll/file.c +++ b/dlls/ntdll/file.c @@ -1199,7 +1199,7 @@ NTSTATUS WINAPI NtWriteFile(HANDLE hFile, HANDLE hEvent, int result, unix_handle, needs_close; unsigned int options; struct io_timeouts timeouts; - NTSTATUS status; + NTSTATUS status, ret_status; ULONG total = 0; enum server_fd_type type; ULONG_PTR cvalue = apc ? 0 : (ULONG_PTR)apc_user; @@ -1409,9 +1409,10 @@ err: if (status != STATUS_PENDING && hEvent) NtResetEvent( hEvent, NULL ); }
- if (send_completion) NTDLL_AddCompletion( hFile, cvalue, status, total, status == STATUS_PENDING ); + ret_status = async_write && status == STATUS_SUCCESS ? STATUS_PENDING : status; + if (send_completion) NTDLL_AddCompletion( hFile, cvalue, status, total, ret_status == STATUS_PENDING );
- return status; + return ret_status; }
diff --git a/dlls/ntdll/tests/file.c b/dlls/ntdll/tests/file.c index 3320ccecfa..ec339891e7 100644 --- a/dlls/ntdll/tests/file.c +++ b/dlls/ntdll/tests/file.c @@ -3491,7 +3491,7 @@ static void test_file_completion_information(void) ok(pov == &ov, "expected %p, got %p\n", &ov, pov); } else - win_skip("WriteFile never returned TRUE\n"); + skip("WriteFile never returned TRUE\n");
info.Flags = FILE_SKIP_COMPLETION_PORT_ON_SUCCESS; status = pNtSetInformationFile(h, &io, &info, sizeof(info), FileIoCompletionNotificationInformation); @@ -3517,7 +3517,7 @@ static void test_file_completion_information(void) ok(pov == NULL, "expected NULL, got %p\n", pov); } else - win_skip("WriteFile never returned TRUE\n"); + skip("WriteFile never returned TRUE\n");
info.Flags = 0; status = pNtSetInformationFile(h, &io, &info, sizeof(info), FileIoCompletionNotificationInformation); @@ -3545,7 +3545,7 @@ static void test_file_completion_information(void) ok(pov == NULL, "expected NULL, got %p\n", pov); } else - win_skip("WriteFile never returned TRUE\n"); + skip("WriteFile never returned TRUE\n");
CloseHandle(port); CloseHandle(h);
Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=43071
This matches Vista+ behaviour.
Signed-off-by: Paul Gofman gofmanp@gmail.com --- v2: - no changes.
dlls/ntdll/file.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/dlls/ntdll/file.c b/dlls/ntdll/file.c index 04b2432875..5bd900d8c6 100644 --- a/dlls/ntdll/file.c +++ b/dlls/ntdll/file.c @@ -1013,8 +1013,7 @@ err: if (status != STATUS_PENDING && hEvent) NtResetEvent( hEvent, NULL ); }
- ret_status = async_read && (options & FILE_NO_INTERMEDIATE_BUFFERING) && status == STATUS_SUCCESS - ? STATUS_PENDING : status; + ret_status = async_read && status == STATUS_SUCCESS ? STATUS_PENDING : status;
if (send_completion) NTDLL_AddCompletion( hFile, cvalue, status, total, ret_status == STATUS_PENDING ); return ret_status;
Hi Paul,
On 2/20/19 6:33 PM, Paul Gofman wrote:
br = pSetFileCompletionNotificationModes(hfile, 0);ok(br, "SetFileCompletionNotificationModes failed, error %u.\n", GetLastError());
It doesn't really hurt, but note that this call is no-op. You can't reset mode bits that are already set. Since the handle is closed just after this test, you could just skip this part.
Jacek
-
Jacek Caban -
Paul Gofman