Re: [PATCH 1/5] lz32/tests: Test more last errors set by LZOpenFile[AW].
Saulius Krasuckas wrote:
(Headers in this series of patches was slightly edited by hand due to lack of internet connection and newer git version. Don't be surprised for inconsistencies and just drop if that was the case)
---
dlls/lz32/tests/lzexpand_main.c | 76 ++++++++++++++++++++++++++++++++++++++- 1 files changed, 74 insertions(+), 2 deletions(-)
484c0e401dc09dd89fffb55da7f34599638bb1cd diff --git a/dlls/lz32/tests/lzexpand_main.c b/dlls/lz32/tests/lzexpand_main.c index eead144..17c2757 100644 --- a/dlls/lz32/tests/lzexpand_main.c +++ b/dlls/lz32/tests/lzexpand_main.c @@ -131,6 +131,7 @@ static void test_LZOpenFileA_existing_co memset(&filled_0xA5, 0xA5, OFS_MAXPATHNAME); memset(&test, 0xA5, sizeof(test)); full_file_path_name_in_a_CWD(filename_, expected, FALSE); + SetLastError(0xfaceabee);
Please use the common accepted and agreed on 0xdeadbeef. As it is used in all the other tests.
/* d, using underscore-terminated file name. */ file = LZOpenFileA(_terminated, &test, OF_EXIST); ok(file >= 0, "LZOpenFileA failed on switching to a compressed file name\n"); + ok(GetLastError() == ERROR_SUCCESS || GetLastError() == ERROR_BAD_PATHNAME, + "GetLastError() returns %ld\n", GetLastError()); This doesn't look right. It either fails or it doesn't. You can't have both. Or this test is nor correct and should be removed.
Vitaliy
* On Thu, 5 Oct 2006, Vitaliy Margolen wrote:
* Saulius Krasuckas wrote:
--- a/dlls/lz32/tests/lzexpand_main.c +++ b/dlls/lz32/tests/lzexpand_main.c @@ -131,6 +131,7 @@ static void test_LZOpenFileA_existing_co memset(&filled_0xA5, 0xA5, OFS_MAXPATHNAME); memset(&test, 0xA5, sizeof(test)); full_file_path_name_in_a_CWD(filename_, expected, FALSE); + SetLastError(0xfaceabee);
Please use the common accepted and agreed on 0xdeadbeef. As it is used in all the other tests.
I am all in a big wait for some text page, telling about the need to clean non-dead-beefed SetLastError calls for example using some chained greps: $ grep -Iri 'SetLastError[^(]*([^_a-z0-9]*[_a-z0-9]\+[^_a-z0-9]*)' dlls/*/tests/*.c | grep -v '#.*define' | grep -vE '\([^a-z]*(0xdeadbeef|ERROR_SUCCESS|0)[^a-z]*\)' output of which gives a number 180 (when routed via "wc -l") and following list (when routed via "sort | uniq"): dlls/advapi32/tests/crypt.c: SetLastError(NTE_PROV_TYPE_ENTRY_BAD); dlls/advapi32/tests/security.c: SetLastError(NO_ERROR); dlls/crypt32/tests/cert.c: SetLastError(0xbaadcafe); dlls/crypt32/tests/protectdata.c: SetLastError(0xDEADBEEF); dlls/kernel32/tests/alloc.c: SetLastError(NO_ERROR); dlls/kernel32/tests/alloc.c: SetLastError(NO_ERROR); dlls/kernel32/tests/change.c: SetLastError(0xd0b00b00); dlls/kernel32/tests/file.c: SetLastError( 0xb00 ); dlls/kernel32/tests/file.c: SetLastError( 0xdeadbeaf ); dlls/kernel32/tests/file.c: SetLastError(0xfaceabee); dlls/kernel32/tests/file.c: SetLastError(0xfaceabee); dlls/kernel32/tests/file.c: SetLastError(12345678); dlls/kernel32/tests/heap.c: SetLastError(MAGIC_DEAD); dlls/kernel32/tests/thread.c: SetLastError(0xFACEaBAD); dlls/lz32/tests/lzexpand_main.c: SetLastError(0xfaceabee); dlls/mscms/tests/profile.c: SetLastError(0xfaceabee); /* 1st, maybe 2nd and then dereferenced 4th param, */ dlls/mscms/tests/profile.c: SetLastError(0xfaceabee); /* 1st param, */ dlls/mscms/tests/profile.c: SetLastError(0xfaceabee); /* 2nd param, */ dlls/mscms/tests/profile.c: SetLastError(0xfaceabee); /* 3rd param, */ dlls/mscms/tests/profile.c: SetLastError(0xfaceabee); /* 3th param, */ dlls/mscms/tests/profile.c: SetLastError(0xfaceabee); /* 4th param, */ dlls/mscms/tests/profile.c: SetLastError(0xfaceabee); /* dereferenced 4th param, */ dlls/mscms/tests/profile.c: SetLastError(0xfaceabee); /* dereferenced 4th param. */ dlls/mscms/tests/profile.c: SetLastError(0xfaceabee); /* dereferenced 4th param. */ dlls/mscms/tests/profile.c: SetLastError(0xfaceabee); /* maybe 2nd and then 4th param, */ dlls/mscms/tests/profile.c: SetLastError(0xfaceabee); /* maybe 2nd param. */ dlls/mscms/tests/profile.c: SetLastError(0xfaceabee); /* maybe 2nd, then 3rd and dereferenced 4th param, */ dlls/setupapi/tests/query.c: SetLastError(0xbeefcafe); dlls/shell32/tests/shlexec.c: SetLastError(0xcafebabe); dlls/version/tests/info.c: SetLastError(MY_LAST_ERROR); dlls/version/tests/info.c: SetLastError(MY_LAST_ERROR); dlls/version/tests/info.c: SetLastError(MY_LAST_ERROR); dlls/wininet/tests/url.c: SetLastError(0xfaceabad); dlls/winspool.drv/tests/info.c: SetLastError(MAGIC_DEAD); dlls/winspool.drv/tests/info.c: SetLastError(MAGIC_DEAD); Did I persuade you, Vitaliy, or you, Dmitry? Feel free to enhance my composite monster. If no such text is put somewhere near Wine docs, I wouldn't believe this requirement is deadly significant. Unless Alexandre confirms the requirement directly. In such case I am going to put this stuff into our Wiki ^^ even write a patch for cleaning the calls myself ;)
/* d, using underscore-terminated file name. */ file = LZOpenFileA(_terminated, &test, OF_EXIST); ok(file >= 0, "LZOpenFileA failed on switching to a compressed file name\n"); + ok(GetLastError() == ERROR_SUCCESS || GetLastError() == ERROR_BAD_PATHNAME, + "GetLastError() returns %ld\n", GetLastError());
This doesn't look right. It either fails or it doesn't. You can't have both. Or this test is nor correct and should be removed.
Win32 app can easily stuck upon this if being run on WinME and WinXP. The result may be used to distinguish between 9x and NT platforms (just like we are doing in our tests to avoid calling GetVersion;), That was the reason I kept the check in. It doesn't enforce unnecessary complexity in the function, so I doubt the check is wrong. Especially given that this fn on both platforms behaves *quite* differently while operating on compressed file(name)s. If you don't agree, please also comment on this output: $ grep -IriC3 'ERROR_SUCCESS.*||.*GetLastError' dlls/*/tests/*.c dlls/kernel32/tests/heap.c- SetLastError(MAGIC_DEAD); dlls/kernel32/tests/heap.c- res = GlobalUnlock(gbl); /* #0 */ dlls/kernel32/tests/heap.c- /* NT: ERROR_SUCCESS (documented on MSDN), 9x: untouched */ dlls/kernel32/tests/heap.c: ok(!res && ((GetLastError() == ERROR_SUCCESS) || (GetLastError() == MAGIC_DEAD)), dlls/kernel32/tests/heap.c- "returned %ld with %ld (expected '0' with: ERROR_SUCCESS or " \ dlls/kernel32/tests/heap.c- "MAGIC_DEAD)\n", res, GetLastError()); dlls/kernel32/tests/heap.c- SetLastError(MAGIC_DEAD); -- dlls/kernel32/tests/heap.c- SetLastError(MAGIC_DEAD); dlls/kernel32/tests/heap.c- res = LocalUnlock(gbl); /* #0 */ dlls/kernel32/tests/heap.c- /* NT: ERROR_SUCCESS (documented on MSDN), 9x: untouched */ dlls/kernel32/tests/heap.c: ok(!res && ((GetLastError() == ERROR_SUCCESS) || (GetLastError() == MAGIC_DEAD)), dlls/kernel32/tests/heap.c- "returned %ld with %ld (expected '0' with: ERROR_SUCCESS or " \ dlls/kernel32/tests/heap.c- "MAGIC_DEAD)\n", res, GetLastError()); dlls/kernel32/tests/heap.c- SetLastError(MAGIC_DEAD); -- dlls/shlwapi/tests/ordinal.c- switch(retval) { dlls/shlwapi/tests/ordinal.c- case 0L: dlls/shlwapi/tests/ordinal.c- if(buffersize == exactsize) { dlls/shlwapi/tests/ordinal.c: ok( (ERROR_SUCCESS == GetLastError()) || (ERROR_CALL_NOT_IMPLEMENTED == GetLastError()) || dlls/shlwapi/tests/ordinal.c- (ERROR_PROC_NOT_FOUND == GetLastError()) || (ERROR_NO_IMPERSONATION_TOKEN == GetLastError()), dlls/shlwapi/tests/ordinal.c- "last error wrong: got %08lx; expected ERROR_SUCCESS(NT4)/ERROR_CALL_NOT_IMPLEMENTED(98/ME)/" dlls/shlwapi/tests/ordinal.c- "ERROR_PROC_NOT_FOUND(NT4)/ERROR_NO_IMPERSONATION_TOKEN(XP)\n", GetLastError()); Vitaliy, do you believe the GLE checks in last chunk should be removed too?
Saulius Krasuckas <saulius2(a)ar.fi.lt> writes:
Unless Alexandre confirms the requirement directly. In such case I am going to put this stuff into our Wiki ^^ even write a patch for cleaning the calls myself ;)
There's no need to enforce 0xdeadbeef, other similar magic values work just as well.
That was the reason I kept the check in. It doesn't enforce unnecessary complexity in the function, so I doubt the check is wrong. Especially given that this fn on both platforms behaves *quite* differently while operating on compressed file(name)s.
Testing last error on success is not useful. It will only cause spurious test failures, so it should be done only if the API specifies a certain behavior, or if you have a real app that depends on it. -- Alexandre Julliard julliard(a)winehq.org
* On Fri, 6 Oct 2006, Alexandre Julliard wrote:
* Saulius Krasuckas <saulius2(a)ar.fi.lt> writes:
That was the reason I kept the check in. It doesn't enforce unnecessary complexity in the function, so I doubt the check is wrong. Especially given that this fn on both platforms behaves *quite* differently while operating on compressed file(name)s.
Testing last error on success is not useful. It will only cause spurious test failures, so it should be done only if the API specifies a certain behavior, or if you have a real app that depends on it.
Well, seems I had already forgotten whether the essential behaviour is a success here or not. The fn acts quite weird on WinME, so I've been treating almost any its behaviour as a failure. Thanks goes to both of you for opening my eyes, Vitaliy and Alexandre.
Saulius Krasuckas wrote:
* On Thu, 5 Oct 2006, Vitaliy Margolen wrote: Unless Alexandre confirms the requirement directly. In such case I am going to put this stuff into our Wiki ^^ even write a patch for cleaning the calls myself ;)
No wonder people can't get _any_ response to their patches. With such an attitude, you should be working for m$. Vitaliy
* On Fri, 6 Oct 2006, Vitaliy Margolen wrote:
* Saulius Krasuckas wrote:
Unless Alexandre confirms the requirement directly. In such case I am going to put this stuff into our Wiki ^^ even write a patch for cleaning the calls myself ;)
No wonder people can't get _any_ response to their patches.
People != one person.
With such an attitude, you should be working for m$.
Vitaliy, this was not exactly an attitude. This was natural request to extend someone's initiative (not mine) into more stable form and to see what Alexandre thinks about this (esp. given recent flame on patch subission) I am sorry if I hurt some person.
participants (3)
-
Alexandre Julliard -
Saulius Krasuckas -
Vitaliy Margolen