Peter Riocreux par+wine_devel@silistix.com writes:
It gives every impression of working for me, but I have only tested it with one application.
Suggestions for better extended error codes appreciated.
Error codes should not be based on suggestions, what you should do is write a small test program under Windows to find out the real codes.
On April 15, 2004 7:54 pm, Alexandre Julliard wrote:
Error codes should not be based on suggestions, what you should do is write a small test program under Windows to find out the real codes.
Hopefully, this sort of thing will be easier once we have the testing framework in place. For example, it's difficult for me to run a test program on Windows, because I don't have (an easily accessible) one.
So maybe with the testing framework in place, people can start by submitting a test, waiting for results, and then submitting the implementation. Very XP-ish :), but also rather cool. One would be able to easily test a hypothesis in about 24h on a large number of different Windows versions, configurations, etc.
Once small detail missing: Paul has vanished off the web, so we don't actually have anything to run on the Windows boxes :(
"Dimitrie O. Paun" dpaun@rogers.com writes:
On April 15, 2004 7:54 pm, Alexandre Julliard wrote:
Error codes should not be based on suggestions, what you should do is write a small test program under Windows to find out the real codes.
Hopefully, this sort of thing will be easier once we have the testing framework in place. For example, it's difficult for me to run a test program on Windows, because I don't have (an easily accessible) one.
So maybe with the testing framework in place, people can start by submitting a test, waiting for results, and then submitting the implementation. Very XP-ish :), but also rather cool. One would be able to easily test a hypothesis in about 24h on a large number of different Windows versions, configurations, etc.
Once small detail missing: Paul has vanished off the web, so we don't actually have anything to run on the Windows boxes :(
I am not well enough versed in the Windows world to try to write a test-case, but if someone can point me at a framework for a basic prog that I can fill in bits of, then I will test it on the one platform I have access to - XP - and use the values from there.
I figured that *some* value, even guessed-at, was better than none - it would allow someone to distinguish between causes of failure. I completely agree that the correct values should be used when they are available - and hence I have now put WARNs in where the guessed-at values are for the moment.
Peter
Index: dlls/kernel/console.c =================================================================== RCS file: /home/wine/wine/dlls/kernel/console.c,v retrieving revision 1.30 diff -u -r1.30 console.c --- dlls/kernel/console.c 13 Apr 2004 21:16:26 -0000 1.30 +++ dlls/kernel/console.c 16 Apr 2004 09:55:24 -0000 @@ -1448,17 +1448,21 @@ { BOOL ret = TRUE;
- FIXME("(%p,%i) - no error checking or testing yet\n", func, add); - if (!func) { + TRACE("(%p,%i) - first arg being NULL is not allowed in WinME, Win98 or Win95\n", func, add); CONSOLE_IgnoreCtrlC = add; } else if (add) { struct ConsoleHandler* ch = HeapAlloc(GetProcessHeap(), 0, sizeof(struct ConsoleHandler));
- if (!ch) return FALSE; + if (!ch) + { + SetLastError(ERROR_NOT_ENOUGH_MEMORY); + WARN("SetLastError() used guessed-at value\n"); + return FALSE; + } ch->handler = func; RtlEnterCriticalSection(&CONSOLE_CritSect); ch->next = CONSOLE_Handlers; @@ -1467,34 +1471,39 @@ } else { - struct ConsoleHandler** ch; RtlEnterCriticalSection(&CONSOLE_CritSect); - for (ch = &CONSOLE_Handlers; *ch; *ch = (*ch)->next) - { - if ((*ch)->handler == func) break; - } - if (*ch) - { - struct ConsoleHandler* rch = *ch; + if (func == CONSOLE_DefaultHandler) + { + ERR("Attempt to remove default CtrlHandler %\n", func); + WARN("SetLastError() used guessed-at value\n"); + SetLastError(ERROR_FUNCTION_FAILED); + ret = FALSE; + } + else + { + struct ConsoleHandler* ch, * prev ; + prev = NULL; + for (ch = CONSOLE_Handlers; ch; prev = ch, ch = ch->next) + { + if (ch->handler == func) + { + if (ch == CONSOLE_Handlers) + CONSOLE_Handlers = ch->next; + else + prev->next = ch->next; + HeapFree(GetProcessHeap(), 0, ch); + break; + } + }
- /* sanity check */ - if (rch == &CONSOLE_DefaultConsoleHandler) - { - ERR("Who's trying to remove default handler???\n"); - ret = FALSE; - } - else - { - rch = *ch; - *ch = (*ch)->next; - HeapFree(GetProcessHeap(), 0, rch); - } - } - else - { - WARN("Attempt to remove non-installed CtrlHandler %p\n", func); - ret = FALSE; - } + if (! ch) + { + WARN("Attempt to remove non-installed CtrlHandler %p\n", func); + WARN("SetLastError() used guessed-at value\n"); + SetLastError(ERROR_INVALID_ADDRESS); + ret = FALSE; + } + } RtlLeaveCriticalSection(&CONSOLE_CritSect); } return ret;
Peter Riocreux wrote:
"Dimitrie O. Paun" dpaun@rogers.com writes:
On April 15, 2004 7:54 pm, Alexandre Julliard wrote:
Error codes should not be based on suggestions, what you should do is write a small test program under Windows to find out the real codes.
I wrote a test program but I haven't integrated it with the Wine test framework (attempting to remove the default handler is not practical anyway).
I figured that *some* value, even guessed-at, was better than none - it would allow someone to distinguish between causes of failure. I completely agree that the correct values should be used when they are available - and hence I have now put WARNs in where the guessed-at values are for the moment.
I would agree with you (especially as you have noted that the errors may not be correct), but many of us have been stuck debugging an application that depends on a particular error being returned and fails otherwise. I have commented on the error codes below:
Index: dlls/kernel/console.c
RCS file: /home/wine/wine/dlls/kernel/console.c,v retrieving revision 1.30 diff -u -r1.30 console.c --- dlls/kernel/console.c 13 Apr 2004 21:16:26 -0000 1.30 +++ dlls/kernel/console.c 16 Apr 2004 09:55:24 -0000 @@ -1448,17 +1448,21 @@ { BOOL ret = TRUE;
- FIXME("(%p,%i) - no error checking or testing yet\n", func, add);
- if (!func) {
TRACE("(%p,%i) - first arg being NULL is not allowed in
WinME, Win98 or Win95\n", func, add); CONSOLE_IgnoreCtrlC = add; } else if (add) { struct ConsoleHandler* ch = HeapAlloc(GetProcessHeap(), 0, sizeof(struct ConsoleHandler));
if (!ch) return FALSE;
if (!ch)
- {
SetLastError(ERROR_NOT_ENOUGH_MEMORY);
This error code is right.
WARN("SetLastError() used guessed-at value\n");
return FALSE;
- } ch->handler = func; RtlEnterCriticalSection(&CONSOLE_CritSect); ch->next = CONSOLE_Handlers;
@@ -1467,34 +1471,39 @@ } else {
struct ConsoleHandler** ch; RtlEnterCriticalSection(&CONSOLE_CritSect);
for (ch = &CONSOLE_Handlers; *ch; *ch = (*ch)->next)
{
if ((*ch)->handler == func) break;
}
if (*ch)
{
struct ConsoleHandler* rch = *ch;
- if (func == CONSOLE_DefaultHandler)
- {
ERR("Attempt to remove default CtrlHandler %\n", func);
WARN("SetLastError() used guessed-at value\n");
SetLastError(ERROR_FUNCTION_FAILED);
This should be ERROR_INVALID_PARAMETER.
ret = FALSE;
- }
- else
- {
struct ConsoleHandler* ch, * prev ;
prev = NULL;
for (ch = CONSOLE_Handlers; ch; prev = ch, ch = ch->next)
{
if (ch->handler == func)
{
if (ch == CONSOLE_Handlers)
CONSOLE_Handlers = ch->next;
else
prev->next = ch->next;
HeapFree(GetProcessHeap(), 0, ch);
break;
}
}
/* sanity check */
if (rch == &CONSOLE_DefaultConsoleHandler)
{
ERR("Who's trying to remove default handler???\n");
ret = FALSE;
}
else
{
rch = *ch;
*ch = (*ch)->next;
HeapFree(GetProcessHeap(), 0, rch);
}
}
else
{
WARN("Attempt to remove non-installed CtrlHandler
%p\n", func);
ret = FALSE;
}
if (! ch)
{
WARN("Attempt to remove non-installed CtrlHandler
%p\n", func);
WARN("SetLastError() used guessed-at value\n");
SetLastError(ERROR_INVALID_ADDRESS);
This should be ERROR_INVALID_PARAMETER.
ret = FALSE;
}
- } RtlLeaveCriticalSection(&CONSOLE_CritSect); } return ret;
Rob
Peter Riocreux par+wine_devel@silistix.com writes:
"Dimitrie O. Paun" dpaun@rogers.com writes:
So maybe with the testing framework in place, people can start by submitting a test, waiting for results, and then submitting the implementation. Very XP-ish :), but also rather cool. One would be able to easily test a hypothesis in about 24h on a large number of different Windows versions, configurations, etc.
I am not well enough versed in the Windows world to try to write a test-case, but if someone can point me at a framework for a basic prog that I can fill in bits of, then I will test it on the one platform I have access to - XP - and use the values from there.
Slight misunderstanding here. The testing framework is part of Wine -- look in dlls/*/tests for examples. The developers' guide covers the topic: see
http://winehq.org/site/docs/wine-devel/testing
If you contribute to this testing suite, your tests will be automatically run on all platforms from time to time. It provides great value for the developers, so please add your tests by all means and ask questions if you are stuck.
Dimitrie O. Paun wrote:
So maybe with the testing framework in place, people can start by submitting a test, waiting for results, and then submitting the implementation. Very XP-ish :), but also rather cool. One would be able to easily test a hypothesis in about 24h on a large number of different Windows versions, configurations, etc.
Once small detail missing: Paul has vanished off the web, so we don't actually have anything to run on the Windows boxes :(
Is everything else, the Windows service and so on already done?
Jakob
On April 17, 2004 10:47 am, Jakob Eriksson wrote:
Is everything else, the Windows service and so on already done?
I think so. Here is the current workflow and status:
1. Tester downloads winrash.exe installer and runs it (one time only) Chris Morgan / [IN PROGRESS] 2. Cron job builds winetest.exe and publishes it on WineHQ Paul Miller / [IN PROGRESS] 3. winrash service runs, polls WineHQ for updated info Chris Morgan [DONE] 4. A CGI processes the request, and returns appropriate instructions Brian Vincent [DONE] 5. winetest.exe is run by winrash to execute tests and submit results Ferenc Wagner [DONE] 6. Results are interpreted on WineHQ and a page is generated Ferenc Wagner [DONE] 7. An index page is adjusted to include the newly generated page TBD [TODO] 8. Everything starts over again at (2).
Now, (8) is not essential for testing, without it we can just go directly to the generated page. Again (1) is just nice to have for end users, we can test without it. We are however blocking at (2) ATM, and I'm not sure what the latest situation is.
BTW, if you guys find any inaccuracies in my status, please feel free to correct me.
As it was written in the Book of "Dimitrie O. Paun" dpaun@rogers.com:
Now, (8) is not essential for testing, without it we can just go directly to the generated page. Again (1) is just nice to have for end users, we can test without it. We are however blocking at (2) ATM, and I'm not sure what the latest situation is.
Nor do I. Paul mentioned he might have something up last week, I may have accidentally read an email of his with the delete key. He also mentioned a problem with other WRT triggers.
I noticed Kevin Koltzau has been playing with cross-compiling, and given that winetest.exe is working now on Windows (we think!) I asked him if he could put something together for testing.
-Brian
I am capable of building this if needed. I've put up a copy of winetest.exe built from current CVS on http://www.plop.org/winetest.exe if you are interested in taking a look
On Saturday 17 April 2004 01:12 pm, Brian Vincent wrote:
I noticed Kevin Koltzau has been playing with cross-compiling, and given that winetest.exe is working now on Windows (we think!) I asked him if he could put something together for testing.
On April 17, 2004 4:14 pm, Kevin Koltzau wrote:
I am capable of building this if needed. I've put up a copy of winetest.exe built from current CVS on http://www.plop.org/winetest.exe if you are interested in taking a look
OK, perfect. Can you do this on a daily basis (from cron, say at 4am EST), and do the publishing stuff?
I can yes, just need to work out the details
On Saturday 17 April 2004 05:09 pm, Dimitrie O. Paun wrote:
OK, perfect. Can you do this on a daily basis (from cron, say at 4am EST), and do the publishing stuff?
On Sat, 17 Apr 2004, Kevin Koltzau wrote:
I can yes, just need to work out the details
Ah, but the devil's in the detail. I might just be me, but it seems to be an inordinately fiddly process, but one I think I've finally got figured out.
Its now part of the new version of WRT, which I'll just tidy up a bit before releasing.
HTH,
Paul.
On Saturday 17 April 2004 05:09 pm, Dimitrie O. Paun wrote:
OK, perfect. Can you do this on a daily basis (from cron, say at 4am EST), and do the publishing stuff?
---- Paul Millar
Kevin Koltzau wrote:
I am capable of building this if needed. I've put up a copy of winetest.exe built from current CVS on http://www.plop.org/winetest.exe if you are interested in taking a look
Excellent!
Running winetest.exe is one of my favourite things to do. :-)
Anyway, I packed it with UPX and it went down from 4 megs to 1:
http://vmlinux.org/jakov/winetest.exe
Jakob
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
On Sat, 17 Apr 2004, Dimitrie O. Paun wrote:
- Cron job builds winetest.exe and publishes it on WineHQ Paul Miller / [IN PROGRESS]
( s/Miller/Millar/ BTW, its the Scottish spelling ;)
We are however blocking at (2) ATM, and I'm not sure what the latest situation is.
The cross-building was blocked because of missing DirectX stuff in mingw. Hans has fixed this and I've incorporated his fixes into my copy of mingw. With this, I've been bale to shake out the last of the bugs.
The system is now live, with output going in http://www.astro.gla.ac.uk/users/paulm/WRT/CrossBuilt
Each build is identified by its time/date stamp (e.g. winetest-20040418-1000.zip). There's a fair number of different files available:
winetest-<date>.zip - zip compressed version of winetest.exe winetest-<date>.zip.cookie - the md5sum of winetest.exe (sort of) winetest-<date>.zip.sig - detached GPG signature of the .zip file winetest-<date>.exe - UPX compressed version of winetest.exe winetest-<date>.exe.cookie - same as .exe.cookie winetest-<date>.exe.sig - detached GPG signature of the .exe file
(I'll make the auto-build public key available in a bit)
There's also sym-links that always point to the latest version. These have "latest" instead of the time-date stamp, for example winetest-latest.zip will always point to the latest version of winetest.zip
Cheers,
- ---- Paul Millar
Paul Millar wrote:
Each build is identified by its time/date stamp (e.g. winetest-20040418-1000.zip). There's a fair number of different files available:
winetest-<date>.zip - zip compressed version of winetest.exe winetest-<date>.zip.cookie - the md5sum of winetest.exe (sort of) winetest-<date>.zip.sig - detached GPG signature of the .zip file winetest-<date>.exe - UPX compressed version of winetest.exe winetest-<date>.exe.cookie - same as .exe.cookie winetest-<date>.exe.sig - detached GPG signature of the .exe file
Thank you, thank you, thank you! :-)
Jakob
On Sun, 18 Apr 2004, Jakob Eriksson wrote:
Thank you, thank you, thank you! :-)
Happy to help :)
---- Paul Millar
Alexandre Julliard julliard@winehq.org writes:
Peter Riocreux par+wine_devel@silistix.com writes:
It gives every impression of working for me, but I have only tested it with one application.
Suggestions for better extended error codes appreciated.
Error codes should not be based on suggestions, what you should do is write a small test program under Windows to find out the real codes.
Robert Shearman provided some info on this and the patch is updated appropriately.
I have also (thanks to a pointer from Ferenc Wagner) created some test stuff. The test compiles but is not run yet as I haven't learned my way around the build process enough to figure out how to make the tests work. I thought I would run this by people now though so I can be steered straight if I have deviated from TheOneTrueWay.
I may have completely misunderstood the macros in the test system and misused them, but it was not obvious (to mine eyes at least) what they all did.
The big issue is that I cannot work out how to test attempting to remove the default handler without knowing what real Windows OSes call both the default handler and the handler list and or having a function to find them, or a fixed address that they always occur at. As a result I have #if 0-ed the thing that needs that. Can anyone point me at anything about this so I can make the patch test those things? Also, is there a way to make the HeapAlloc fail so I can test that failure path?
Note also the commented deficiencies of the test.
Peter
Index: dlls/kernel/console.c =================================================================== RCS file: /home/wine/wine/dlls/kernel/console.c,v retrieving revision 1.30 diff -u -r1.30 console.c --- dlls/kernel/console.c 13 Apr 2004 21:16:26 -0000 1.30 +++ dlls/kernel/console.c 16 Apr 2004 14:12:31 -0000 @@ -1448,17 +1448,20 @@ { BOOL ret = TRUE;
- FIXME("(%p,%i) - no error checking or testing yet\n", func, add); - if (!func) { + TRACE("(%p,%i) - first arg being NULL is not allowed in WinME, Win98 or Win95\n", func, add); CONSOLE_IgnoreCtrlC = add; } else if (add) { struct ConsoleHandler* ch = HeapAlloc(GetProcessHeap(), 0, sizeof(struct ConsoleHandler));
- if (!ch) return FALSE; + if (!ch) + { + SetLastError(ERROR_NOT_ENOUGH_MEMORY); + return FALSE; + } ch->handler = func; RtlEnterCriticalSection(&CONSOLE_CritSect); ch->next = CONSOLE_Handlers; @@ -1467,34 +1470,46 @@ } else { - struct ConsoleHandler** ch; + /* + ** As CONSOLE_DefaultHandler will always be the last in the + ** chain (new handlers are added at the front), should we + ** remove any instances of CONSOLE_DefaultHandler except the + ** last one when requested? I.e., <philosophy> is any but the + ** last *actually* the default handler? </philosophy> When we + ** remove a handler, should we remove every instance of it, + ** just the first (current behaviour), or just the last? + */ RtlEnterCriticalSection(&CONSOLE_CritSect); - for (ch = &CONSOLE_Handlers; *ch; *ch = (*ch)->next) - { - if ((*ch)->handler == func) break; - } - if (*ch) + if (func == CONSOLE_DefaultHandler) { - struct ConsoleHandler* rch = *ch; + ERR("Attempt to remove default CtrlHandler %p\n", func); + SetLastError(ERROR_INVALID_PARAMETER); + ret = FALSE; + } + else + { + struct ConsoleHandler* ch, * prev ; + prev = NULL; + for (ch = CONSOLE_Handlers; ch; prev = ch, ch = ch->next) + { + if (ch->handler == func) + { + if (ch == CONSOLE_Handlers) + CONSOLE_Handlers = ch->next; + else + prev->next = ch->next; + HeapFree(GetProcessHeap(), 0, ch); + break; + } + }
- /* sanity check */ - if (rch == &CONSOLE_DefaultConsoleHandler) - { - ERR("Who's trying to remove default handler???\n"); - ret = FALSE; - } - else - { - rch = *ch; - *ch = (*ch)->next; - HeapFree(GetProcessHeap(), 0, rch); - } - } - else - { - WARN("Attempt to remove non-installed CtrlHandler %p\n", func); - ret = FALSE; - } + if (! ch) + { + WARN("Attempt to remove non-installed CtrlHandler %p\n", func); + SetLastError(ERROR_INVALID_PARAMETER); + ret = FALSE; + } + } RtlLeaveCriticalSection(&CONSOLE_CritSect); } return ret; Index: dlls/kernel/tests/console.c =================================================================== RCS file: /home/wine/wine/dlls/kernel/tests/console.c,v retrieving revision 1.4 diff -u -r1.4 console.c --- dlls/kernel/tests/console.c 26 Jan 2004 20:23:25 -0000 1.4 +++ dlls/kernel/tests/console.c 16 Apr 2004 14:12:31 -0000 @@ -537,6 +537,56 @@ } #endif
+/* DEFICIENCIES: +** - Does not attempt to test whether disabling CtrlC actually has +** any effect (either by looking at the variable or synthesising an + event). +** - Does not check whether the handler was *actually* installed or +** removed. +*/ +static BOOL WINAPI testDummyHandler(DWORD dwCtrlType) +{ + ExitProcess(0); + /* should never go here */ + return TRUE; +} + +static void testSetConsoleCtrlHandler(void) +{ + ok(SetConsoleCtrlHandler(NULL, FALSE) == TRUE, + "SetConsoleCtrlHandler: could not enable CtrlC handling: GetLastError() returned %lu\n", + GetLastError()); + + ok(SetConsoleCtrlHandler(NULL, TRUE) == TRUE, + "SetConsoleCtrlHandler: could not disable CtrlC handling: GetLastError() returned %lu\n", + GetLastError()); + + ok(SetConsoleCtrlHandler(NULL, FALSE) == TRUE, + "SetConsoleCtrlHandler: could not enable CtrlC handling: GetLastError() returned %lu\n", + GetLastError()); + +#if 0 + ok(SetConsoleCtrlHandler(CONSOLE_DefaultHandler, FALSE) == FALSE, + "SetConsoleCtrlHandler: attempt to remove the default handler apparently succeeded!\n"); + ok(GetLastError() == ERROR_INVALID_PARAMETER, + "SetConsoleCtrlHandler: attempt to remove the default handler: GetLastError() should have returned %u, actually %lu\n", + ERROR_INVALID_PARAMETER, GetLastError()); +#endif + + ok(SetConsoleCtrlHandler(testDummyHandler, FALSE) == FALSE, + "SetConsoleCtrlHandler: attempt to remove a non-existent handler apparently succeeded!\n"); + ok(GetLastError() == ERROR_INVALID_PARAMETER, + "SetConsoleCtrlHandler: attempt to remove a non-existent handler: GetLastError() should have returned %u, actually %lu\n", + ERROR_INVALID_PARAMETER, GetLastError()); + + ok(SetConsoleCtrlHandler(testDummyHandler, TRUE) == TRUE, + "SetConsoleCtrlHandler: attempt to install a handler failed: GetLastError() returned %lu\n", + GetLastError()); + ok(SetConsoleCtrlHandler(testDummyHandler, FALSE) == TRUE, + "SetConsoleCtrlHandler: attempt to uninstall a handler failed: GetLastError() returned %lu\n", + GetLastError()); +} + START_TEST(console) { HANDLE hConIn, hConOut; @@ -579,6 +629,8 @@ /* testScroll(hCon, sbi.dwSize); */ /* will test sb creation / modification... */ /* testScreenBuffer() */ + /* tests adding and removing handlers for CtrlC etc */ + testSetConsoleCtrlHandler();
/* still to be done: events generation, access rights & access on objects */ }