Re: [PATCH 1/4] msvcp110: Add tr2_sys__File_size implementation and test.(try 3)
On 06/09/15 18:49, YongHao Hu wrote:
try 3: Add errno and SetLastError test. The series looks good for me.
The GetLastError value seems to be inconsistent (it's best visible in _Equivalent tests). I'm not sure why I've asked you about testing GetLastError, I was probably thinking about errno. Anyway I don't think it's useful to test it in future. Thanks, Piotr
Hi, Piotr. Thank you for your review. :) Do you mean that I should not test GetLastError in msvcp? I am afraid it is necessary to test it because the implementation may change it and I found some issues about error handling in kernel32 functions like RemoveDirectoryA and CopyFileA etc. Regards. On 2015年06月10日 04:47, Piotr Caban wrote:
On 06/09/15 18:49, YongHao Hu wrote:
try 3: Add errno and SetLastError test. The series looks good for me.
The GetLastError value seems to be inconsistent (it's best visible in _Equivalent tests). I'm not sure why I've asked you about testing GetLastError, I was probably thinking about errno. Anyway I don't think it's useful to test it in future.
Thanks, Piotr
Hi, On 06/11/15 09:02, YongHao Hu wrote:
Do you mean that I should not test GetLastError in msvcp? The _Equivalent tests shows that applications probably shouldn't depend on GetLastError value. The function sometimes sets error even if it succeeds, sometimes the error depends on arguments order. In general while using msvcp/msvcrt functions applications are supposed to check errno value that is not set in this case.
The tests shows that Microsoft is probably using different functions internally. E.g. this code looks suspicious: +ULONGLONG __cdecl tr2_sys__File_size(const char* path) +{ + WIN32_FILE_ATTRIBUTE_DATA fad; + + TRACE("(%p)\n", path); + if(!path) { + SetLastError(ERROR_INVALID_PARAMETER); + return 0; + } I don't think msvcp should set the error explicitly. It's why I think it's better to just skip GetLastError tests and revisit it if we find any application that depends on GetLastError value. I don't think we will find any.
I am afraid it is necessary to test it because the implementation may change it and I found some issues about error handling in kernel32 functions like RemoveDirectoryA and CopyFileA etc. The tests should go to kernel32/tests in this case.
Thanks, Piotr
Hi, On 2015年06月11日 16:21, Piotr Caban wrote:
The _Equivalent tests shows that applications probably shouldn't depend on GetLastError value. The function sometimes sets error even if it succeeds, sometimes the error depends on arguments order. In general while using msvcp/msvcrt functions applications are supposed to check errno value that is not set in this case.
Do you mean that _Equivalent sets ERROR_SUCCESS when it succeeds?
The tests shows that Microsoft is probably using different functions internally. E.g. this code looks suspicious: +ULONGLONG __cdecl tr2_sys__File_size(const char* path) +{ + WIN32_FILE_ATTRIBUTE_DATA fad; + + TRACE("(%p)\n", path); + if(!path) { + SetLastError(ERROR_INVALID_PARAMETER); + return 0; + } I don't think msvcp should set the error explicitly. It's why I think it's better to just skip GetLastError tests and revisit it if we find any application that depends on GetLastError value. I don't think we will find any.
I had made two implementation before, another one is like this: HANDLE h1; h1 = CreateFileA(path, 0, FILE_SHARE_DELETE | FILE_SHARE_READ | FILE_SHARE_WRITE, NULL, OPEN_EXISTING, 0, 0); if(h1 == INVALID_HANDLE_VALUE) return 0; DWORD size; LPDWORD high; size = GetFileSize( h1, &high ); TRACE("(%p)\n", path); return ((ULONGLONG)(high) << 32) + size; (As I had showed it here, could you please also tell me which implementation is better and why?) This one also needs to set the error explicitly, which I didn't do it. Do you think that we should ignore GetLastError in these series of patches? Ps: My tests[1] show that _Copy_file and _Rename return GetLastError, but they doesn't matter at all. Thank you. [1]: https://github.com/YongHaoWu/wine-hub/commit/8a5af834d63914e7eaed75553399001...
On 06/11/15 11:31, YongHao Hu wrote:
Do you mean that _Equivalent sets ERROR_SUCCESS when it succeeds? No. I mean that native doesn't really care about error returned by GetLastError. It looks like it's just a side effect of the way how it's implemented. Probably there's no need to be compatible here.
I had made two implementation before, another one is like this: HANDLE h1; h1 = CreateFileA(path, 0, FILE_SHARE_DELETE | FILE_SHARE_READ | FILE_SHARE_WRITE, NULL, OPEN_EXISTING, 0, 0); if(h1 == INVALID_HANDLE_VALUE) return 0; DWORD size; LPDWORD high; size = GetFileSize( h1, &high ); TRACE("(%p)\n", path); return ((ULONGLONG)(high) << 32) + size;
(As I had showed it here, could you please also tell me which implementation is better and why?) Both implementations are good.
This one also needs to set the error explicitly, which I didn't do it. Do you think that we should ignore GetLastError in these series of patches? I don't know, probably yes. The code is nicer when the error is not set explicitly with SetLastError call. The functions from kernel32 will set the error anyway. It just will not match error set by native msvcp120.
Thanks, Piotr
Thank you. New patches had sent to wine-patches. : ) On 2015年06月11日 17:51, Piotr Caban wrote:
On 06/11/15 11:31, YongHao Hu wrote:
Do you mean that _Equivalent sets ERROR_SUCCESS when it succeeds? No. I mean that native doesn't really care about error returned by GetLastError. It looks like it's just a side effect of the way how it's implemented. Probably there's no need to be compatible here.
This one also needs to set the error explicitly, which I didn't do it. Do you think that we should ignore GetLastError in these series of patches? I don't know, probably yes. The code is nicer when the error is not set explicitly with SetLastError call. The functions from kernel32 will set the error anyway. It just will not match error set by native msvcp120.
Thanks, Piotr
participants (2)
-
Piotr Caban -
YongHao Hu