[PATCH v3 4/5] ntdll: Return STATUS_PENDING from NtWriteFile() for async write.

[Jacek Caban]

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