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