[PATCH v3 1/5] kernel32/tests: Test ReadFileScatter() completion status with FILE_SKIP_COMPLETION_PORT_ON_SUCCESS.
Signed-off-by: Paul Gofman gofmanp@gmail.com --- v3: - remove non working file flags reset.
dlls/kernel32/tests/file.c | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+)
diff --git a/dlls/kernel32/tests/file.c b/dlls/kernel32/tests/file.c index 18cd18d154..dd84557238 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,28 @@ 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); + } + else + win_skip("SetFileCompletionNotificationModes not available.\n"); + CloseHandle( hfile ); CloseHandle( hiocp1 ); CloseHandle( hiocp2 );
Signed-off-by: Paul Gofman gofmanp@gmail.com --- v2, v3: 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: Jacek Cabanjacek@codeweavers.com
Signed-off-by: Paul Gofman gofmanp@gmail.com --- v2: - Pass real status (not ret_status) to NTDLL_AddCompletion() in NtReadFile(). v3: - pass FALSE instead of 'status == STATUS_PENDING' to NTDLL_AddCompletion().
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 dd84557238..33b17aa327 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..ffedbeb534 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, 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, FALSE );
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);
Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=42982
This matches Vista+ behaviour.
Signed-off-by: Paul Gofman gofmanp@gmail.com --- v2, v3: 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 ffedbeb534..b6307a2436 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, FALSE ); + 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);
Hi Paul,
On 2/20/19 7:50 PM, Paul Gofman wrote:
Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=42982
This matches Vista+ behaviour.
Signed-off-by: Paul Gofman gofmanp@gmail.com
v2, v3: 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 ffedbeb534..b6307a2436 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, FALSE );
- 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;
While it generally looks like Vista+ indeed returns STATUS_PENDING in quite a lot of situations, doing it always in all cases is probably not the right thing. For example, read()/write() on overlapped named pipes does not return STATUS_PENDING if the request may be completed immediately. Although named pipe read/write use different code path in Wine, the general concern remains: we can't assume that Windows does that unconditionally for all all overlapped requests.
That said, I think it needs more investigation. I know that our tests may not be great to check that as they accept both pre-Vista and Vista+ behaviour as valid, but maybe marking pre-Vista results as broken() would be a good start so that we can see an evidence that it's a correct thing to do (depending on outcome, those broken()s are may or may not be something we want in Wine, but it would surely be useful for investigation). Some experimentation with different object types could give us useful info as well.
Also note that such change may need throughout review of Wine to make sure that we don't depend on pre-Vista behaviour in our code. read()/write() should be safer, but I've seen STATUS_PENDING returned from ioctl()s as well (even on pipes), and changing that would surely break kernel32 code.
Thanks,
Jacek
Hello Jacek,
On 2/26/19 17:23, Jacek Caban wrote:
While it generally looks like Vista+ indeed returns STATUS_PENDING in quite a lot of situations, doing it always in all cases is probably not the right thing. For example, read()/write() on overlapped named pipes does not return STATUS_PENDING if the request may be completed immediately. Although named pipe read/write use different code path in Wine, the general concern remains: we can't assume that Windows does that unconditionally for all all overlapped requests.
Do you think this can be considered for regular files? Ignoring fd type in my patch is definitely a mistake. The motivation under this are applications which now sometimes strictly expect async status when reading / writing regular files asynchronously (they are not XP compatible of course). Is it absolutely no go to make this behaviour dependant on defined win version?
That said, I think it needs more investigation. I know that our tests may not be great to check that as they accept both pre-Vista and Vista+ behaviour as valid, but maybe marking pre-Vista results as broken() would be a good start so that we can see an evidence that it's a correct thing to do (depending on outcome, those broken()s are may or may not be something we want in Wine, but it would surely be useful for investigation).
Yes, sure, I can mark those tests.
Also note that such change may need throughout review of Wine to make sure that we don't depend on pre-Vista behaviour in our code. read()/write() should be safer, but I've seen STATUS_PENDING returned from ioctl()s as well (even on pipes), and changing that would surely break kernel32 code.
I can search for it. Hopefully this is not so common case to request an overlapped file IO and then always expect sync results.
On 2/26/19 7:45 PM, Paul Gofman wrote:
Hello Jacek,
On 2/26/19 17:23, Jacek Caban wrote:
While it generally looks like Vista+ indeed returns STATUS_PENDING in quite a lot of situations, doing it always in all cases is probably not the right thing. For example, read()/write() on overlapped named pipes does not return STATUS_PENDING if the request may be completed immediately. Although named pipe read/write use different code path in Wine, the general concern remains: we can't assume that Windows does that unconditionally for all all overlapped requests.
Do you think this can be considered for regular files?
I don't know, is STATUS_PENDING specific to regular files?
Ignoring fd type in my patch is definitely a mistake. The motivation under this are applications which now sometimes strictly expect async status when reading / writing regular files asynchronously (they are not XP compatible of course). Is it absolutely no go to make this behaviour dependant on defined win version?
If it can be avoided, yes. I think we should carefully change it to STATUS_PENDING because we have an evidence that some applications need it. We don't know if anything will break.
Also note that such change may need throughout review of Wine to make sure that we don't depend on pre-Vista behaviour in our code. read()/write() should be safer, but I've seen STATUS_PENDING returned from ioctl()s as well (even on pipes), and changing that would surely break kernel32 code.
I can search for it. Hopefully this is not so common case to request an overlapped file IO and then always expect sync results.
More like a code that calls ReadFile on a handle passed from an application that happens to be overlapped. Or overlapped file handle is created by accident, since with current code it doesn't matter for file handles. Anyway, it doesn't seem likely.
Jacek
On 2/27/19 21:28, Jacek Caban wrote:
I don't know, is STATUS_PENDING specific to regular files?
No, it is not specific. ntdll read / write can aleady return either _PENDING or _SUCCESS for special files, depending on whether the operation is actually performed asynchronously. But regular file operations are currently never queued asynchronously in Wine. In the bugs I am aware of the application expects _PENDING just for regular files. I suppose it is not something to be unified across different file handle types. If you agree with that it is probably much easier to deal with regular files without relation to other cases.
More like a code that calls ReadFile on a handle passed from an application that happens to be overlapped. Or overlapped file handle is created by accident, since with current code it doesn't matter for file handles. Anyway, it doesn't seem likely.
I think I can grep for read / write file calls and check cases which seem suspicious in this regard. The amount of read / write calls must be huge but hopefully there are limited number of places where _OVERLAPPED is used explicitly or read / write on app provided handle is used.
Hello Jacek,
so far I grepped the source tree for the WriteFile. This was supposed to show me WriteFileEx and NtWriteFile calls too. If LPOVERLAPPED (the last parameter to WriteFile) is NULL, WriteFile is going to fail with async file handles (at least for regular / serial / device files) regardless of actual sync or async. After skipping the calls which supply NULL (or 0, or FALSE like source in programs/ often does) for overlapped structure, here is what left:
- ole32/rpc.c, local_server_thread(): works on pipe, passes overlapped structure and calls GetOverlappedResult() uncodintionally after; - user.exe16/comm.c, COMM16_WriteFile(): passes OV structure, calls GetOverlappedResult() if GetLastError()==ERROR_IO_PENDING - programs/services/rpc.c, process_send_command(): passes OV structure, then processes GetLastError()==ERROR_IO_PENDING.
Do you think it makes sense if I proceed to change ntdll tests as described before (marking pre-Vista behaviour as invalid) and then resend the patch concerning NtWriteFile() with the async return for regular files only?
Thanks, Paul.
On 2/27/19 23:16, Paul Gofman wrote:
On 2/27/19 21:28, Jacek Caban wrote:
I don't know, is STATUS_PENDING specific to regular files?
No, it is not specific. ntdll read / write can aleady return either _PENDING or _SUCCESS for special files, depending on whether the operation is actually performed asynchronously. But regular file operations are currently never queued asynchronously in Wine. In the bugs I am aware of the application expects _PENDING just for regular files. I suppose it is not something to be unified across different file handle types. If you agree with that it is probably much easier to deal with regular files without relation to other cases.
More like a code that calls ReadFile on a handle passed from an application that happens to be overlapped. Or overlapped file handle is created by accident, since with current code it doesn't matter for file handles. Anyway, it doesn't seem likely.
I think I can grep for read / write file calls and check cases which seem suspicious in this regard. The amount of read / write calls must be huge but hopefully there are limited number of places where _OVERLAPPED is used explicitly or read / write on app provided handle is used.
On 3/4/19 3:03 PM, Paul Gofman wrote:
Hello Jacek,
so far I grepped the source tree for the WriteFile. This was supposed to show me WriteFileEx and NtWriteFile calls too. If LPOVERLAPPED (the last parameter to WriteFile) is NULL, WriteFile is going to fail with async file handles (at least for regular / serial / device files) regardless of actual sync or async.
Why would WriteFile with NULL overlapped fail? It will wait on provided handle, which is suboptimal, but it should work. Anyway, it's fine to skip those cases in your case.
After skipping the calls which supply NULL (or 0, or FALSE like source in programs/ often does) for overlapped structure, here is what left:
- ole32/rpc.c, local_server_thread(): works on pipe, passes overlapped
structure and calls GetOverlappedResult() uncodintionally after;
- user.exe16/comm.c, COMM16_WriteFile(): passes OV structure, calls
GetOverlappedResult() if GetLastError()==ERROR_IO_PENDING
- programs/services/rpc.c, process_send_command(): passes OV
structure, then processes GetLastError()==ERROR_IO_PENDING.
Sounds good, thanks.
Do you think it makes sense if I proceed to change ntdll tests as described before (marking pre-Vista behaviour as invalid) and then resend the patch concerning NtWriteFile() with the async return for regular files only?
Mostly yes. I'm not sure about doing this only for regular files, through. I know it addresses my concern, but your testing seems to indicate that it's pipes that are unusual and, as far as I understand, all other tested types return STATUS_PENDING. If that's the case, maybe we should return STATUS_PENDING without depending on handle type?
Thanks,
Jacek
On 3/4/19 20:10, Jacek Caban wrote:
On 3/4/19 3:03 PM, Paul Gofman wrote:
Hello Jacek,
so far I grepped the source tree for the WriteFile. This was supposed to show me WriteFileEx and NtWriteFile calls too. If LPOVERLAPPED (the last parameter to WriteFile) is NULL, WriteFile is going to fail with async file handles (at least for regular / serial / device files) regardless of actual sync or async.
Why would WriteFile with NULL overlapped fail? It will wait on provided handle, which is suboptimal, but it should work. Anyway, it's fine to skip those cases in your case.
While kernel32.WriteFile() is ok with NULL overlapped structure and is ready to wait, NtWriteFile() is not: it will get 'PLARGE_INTEGER offset' parameter as NULL from kernel32.WriteFile, and will fail with STATUS_INVALID_PARAMETER for async files (at least regular, serial or device files). I also just tested that on a minimal example which attempts to call kernel32.WriteFile() without overlapped structure, this fails for me for file created with FILE_FLAG_OVERLAPPED both under Wine and Windows 7.
Do you think it makes sense if I proceed to change ntdll tests as described before (marking pre-Vista behaviour as invalid) and then resend the patch concerning NtWriteFile() with the async return for regular files only?
Mostly yes. I'm not sure about doing this only for regular files, through. I know it addresses my concern, but your testing seems to indicate that it's pipes that are unusual and, as far as I understand, all other tested types return STATUS_PENDING. If that's the case, maybe we should return STATUS_PENDING without depending on handle type?
My testing covered regular files so far. The change (in its initial form) can pass the existing tests for special files (as STATUS_PENDING return is surely possible for the other types), but I don't think we have enough tests yet to be sure it is always async for all the file types. Besides pipes (for which we probably should avoid always returning async, even if NtReadFile() is not used in Wine for them?), I would expect sync return to be possible at least for sockets. It is also hard to be sure whether we really can never get sync result from a special file, or just did not reproduce the right case. I also thought that in any case, even if sockets and all the other types turn out to work async always, it is safer to change the behaviour in smaller parts for more precise bisect in case of regressions (yes, my initial patch did it for all file types together, but I tend to treat that as a very unfortunate overlook from my side).
Anyway, if you feel like it is better to cover that in respect to all the possible file types, I can start thinking of adding fd type specific tests. In this case, do you think we should try bringing all the tests directly to ntdll/tests, or rather extend the tests existing elsewhere (e. g. in ws2_32 for sockets)? I would probably stick to the latter unless there are reasons not to.
On 3/4/19 6:57 PM, Paul Gofman wrote:
On 3/4/19 20:10, Jacek Caban wrote:
On 3/4/19 3:03 PM, Paul Gofman wrote:
Hello Jacek,
so far I grepped the source tree for the WriteFile. This was supposed to show me WriteFileEx and NtWriteFile calls too. If LPOVERLAPPED (the last parameter to WriteFile) is NULL, WriteFile is going to fail with async file handles (at least for regular / serial / device files) regardless of actual sync or async.
Why would WriteFile with NULL overlapped fail? It will wait on provided handle, which is suboptimal, but it should work. Anyway, it's fine to skip those cases in your case.
While kernel32.WriteFile() is ok with NULL overlapped structure and is ready to wait, NtWriteFile() is not: it will get 'PLARGE_INTEGER offset' parameter as NULL from kernel32.WriteFile, and will fail with STATUS_INVALID_PARAMETER for async files (at least regular, serial or device files). I also just tested that on a minimal example which attempts to call kernel32.WriteFile() without overlapped structure, this fails for me for file created with FILE_FLAG_OVERLAPPED both under Wine and Windows 7.
Ah, right.
Do you think it makes sense if I proceed to change ntdll tests as described before (marking pre-Vista behaviour as invalid) and then resend the patch concerning NtWriteFile() with the async return for regular files only?
Mostly yes. I'm not sure about doing this only for regular files, through. I know it addresses my concern, but your testing seems to indicate that it's pipes that are unusual and, as far as I understand, all other tested types return STATUS_PENDING. If that's the case, maybe we should return STATUS_PENDING without depending on handle type?
My testing covered regular files so far. The change (in its initial form) can pass the existing tests for special files (as STATUS_PENDING return is surely possible for the other types), but I don't think we have enough tests yet to be sure it is always async for all the file types. Besides pipes (for which we probably should avoid always returning async, even if NtReadFile() is not used in Wine for them?), I would expect sync return to be possible at least for sockets.
Named pipes on Wine use NtWriteFile, just not the code path that you change. They just call server_write_file. The same is true for device files.
It is also hard to be sure whether we really can never get sync result from a special file, or just did not reproduce the right case. I also thought that in any case, even if sockets and all the other types turn out to work async always, it is safer to change the behaviour in smaller parts for more precise bisect in case of regressions (yes, my initial patch did it for all file types together, but I tend to treat that as a very unfortunate overlook from my side).
Sounds reasonable to me.
Anyway, if you feel like it is better to cover that in respect to all the possible file types, I can start thinking of adding fd type specific tests. In this case, do you think we should try bringing all the tests directly to ntdll/tests, or rather extend the tests existing elsewhere (e. g. in ws2_32 for sockets)? I would probably stick to the latter unless there are reasons not to.
I didn't really mean sockets specifically. Mailslot may be easier to test just as a data point. I just quickly tested that and mailslots, like named pipes, don't return STATUS_PENDING. With that information, making the change only for regular files seems indeed better.
Thanks,
Jacek
Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=43071
This matches Vista+ behaviour.
Signed-off-by: Paul Gofman gofmanp@gmail.com --- v2, v3: 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 b6307a2436..43f391a6e1 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;
-
Jacek Caban -
Paul Gofman