help fixing up SetConsoleCtrlHandler()

[Peter Riocreux]

Having seen a lot of messages like:

-----------------
fixme:console:SetConsoleCtrlHandler (0x41a8af,1) - no error checking or testing yet
-----------------

lately, I thought I would actually see if I could fix it up and remove the FIXME.

It is in dlls/kernel/console.c, and the MSDN reference is at

http://msdn.microsoft.com/library/default.asp?url=/library/en-us/dllproc/base/setconsolectrlhandler.asp.

I think it is obvious enough that even I, a Wine newbie could do it,
except for a couple of things that, as a newbie, are not obvious to
me. Please point me at docs if this is covered somewhere - it is hard
getting into a new project ....

1) If I am using SetLastError, it is not obvious to me what error
values I should be using there. I assume there is a generic
couldnt-allocate-on-heap error, but other causes of error are specific
to the function, and I cannot find any documentation. Should I just
use a non-zero error number and put FIXMEs in for any number I had to
invent?

2) Can I assume that Rtl{Enter,Leave}CriticalSection(), HeapFree(),
ERR() and WARN() cannot fail (the latter two are presumably macros).

3) The docs say that it behaves eeeeeeever so slightly differently in
ME/98/95 than in others. In this case it is simply that one of the
arguments may not be NULL. Should I, either in general, or in this
case in particular, detect which Windows version is being used and
behave appropriately, or implement the more flexible approach and
assume that people writing for the more restricted OS will not have
used that case. If I shoudl detect, how does one go about this?

Peter

[PATCH] Re: help fixing up SetConsoleCtrlHandler()

[Peter Riocreux]

Here is my first cut.

Remember that this is my first patch, so go easy on me with he
flamethrowers. I know the indenting isn't quite right.

Probably easiest to review by comparing old and new rather than
looking at old and patch.

Comments, especially regarding more appropriate error values to use,
are welcome.

I think that, despite what Juan Lang advised, I need to detect version
numbers as the FIXME can be triggered by perfecly legal use in
XP/NT/?? but illegal code in ME/98/95. I am surprised that there is
not some global enum-type variable containing a value that tells us
which Windows we are pretending to be. There must be many more
functions than this whose behaviour changed between OSes.

Peter

[PATCH] Re: help fixing up SetConsoleCtrlHandler()

[Dimitrie O. Paun]

On April 15, 2004 7:58 am, Peter Riocreux wrote:
> Here is my first cut.
>
> Remember that this is my first patch, so go easy on me with he
> flamethrowers. I know the indenting isn't quite right.

The formatting doesn't seem too bad, but the format is wrong.
Please use uniffied diff (-u):
    http://www.winehq.org/site/sending_patches
> I think that, despite what Juan Lang advised, I need to detect version
> numbers as the FIXME can be triggered by perfecly legal use in
> XP/NT/?? but illegal code in ME/98/95.

Then allow it. Are there apps that depend on the 9x behaviour
of not allowing it?

[PATCH] Re: help fixing up SetConsoleCtrlHandler()

[Peter Riocreux]

"Dimitrie O. Paun"  writes:
> On April 15, 2004 7:58 am, Peter Riocreux wrote:
> > Here is my first cut.
> >
> > Remember that this is my first patch, so go easy on me with he
> > flamethrowers. I know the indenting isn't quite right.
>
> The formatting doesn't seem too bad, but the format is wrong.
> Please use uniffied diff (-u):
>     http://www.winehq.org/site/sending_patches

D'oh! I did the first time I generated the patch, but forgot this
time. I *had* read that page, but ..... old age don' you know. :-^

I will include at the end the new patch.
> > I think that, despite what Juan Lang advised, I need to detect version
> > numbers as the FIXME can be triggered by perfecly legal use in
> > XP/NT/?? but illegal code in ME/98/95.
>
> Then allow it. Are there apps that depend on the 9x behaviour
> of not allowing it?

I don't know, I just had thought that Wine was attempting to be
bug-for-bug compatible, and assumed that part of that was honouring
all the restrictions on usage of functions and how these varied
between versions.

I am not seeing this FIXME coming out, so I am not concerned with
leaving it in, but have no strong opinion.

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	15 Apr 2004 12:08:08 -0000
@@ -1448,17 +1448,20 @@
 {
     BOOL        ret = TRUE;

-    FIXME("(%p,%i) - no error checking or testing yet\n", func, add);
-
     if (!func)
     {
+        FIXME("(%p,%i) - this 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,37 @@
     }
     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 %p\n", func);
+	    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);
+		SetLastError(ERROR_INVALID_ADDRESS);
+		ret = FALSE;
+	    }
+	}
         RtlLeaveCriticalSection(&CONSOLE_CritSect);
     }
     return ret;

[PATCH] Re: help fixing up SetConsoleCtrlHandler()

[Dimitrie O. Paun]

On Thu, 15 Apr 2004, Peter Riocreux wrote:
> I don't know, I just had thought that Wine was attempting to be
> bug-for-bug compatible, and assumed that part of that was honouring
> all the restrictions on usage of functions and how these varied
> between versions.

We are not *that* compatible. it would lead to madness. Soon.
In general, unless applications really depend on 9x specific
functionality, we just follow the NT behaviour.
> I am not seeing this FIXME coming out, so I am not concerned with
> leaving it in, but have no strong opinion.

Yes, I think it should either go, or be made into a TRACE.

[PATCH] Re: help fixing up SetConsoleCtrlHandler()

[Peter Riocreux]

"Dimitrie O. Paun"  writes:
> On Thu, 15 Apr 2004, Peter Riocreux wrote:
> > I don't know, I just had thought that Wine was attempting to be
> > bug-for-bug compatible, and assumed that part of that was honouring
> > all the restrictions on usage of functions and how these varied
> > between versions.
>
> We are not *that* compatible. it would lead to madness. Soon.
> In general, unless applications really depend on 9x specific
> functionality, we just follow the NT behaviour.

I can't see anyone relying on using something they know to be
wrong. *That* would also be madness....
> > I am not seeing this FIXME coming out, so I am not concerned with
> > leaving it in, but have no strong opinion.
>
> Yes, I think it should either go, or be made into a TRACE.

I have turned it into a trace.

Patch below.

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	15 Apr 2004 14:20:57 -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,37 @@
     }
     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 %p\n", func);
+	    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);
+		SetLastError(ERROR_INVALID_ADDRESS);
+		ret = FALSE;
+	    }
+	}
         RtlLeaveCriticalSection(&CONSOLE_CritSect);
     }
     return ret;

[PATCH] Re: help fixing up SetConsoleCtrlHandler()

[Dimitrie O. Paun]

On Thu, 15 Apr 2004, Peter Riocreux wrote:
> I can't see anyone relying on using something they know to be
> wrong. *That* would also be madness....

Right. And hence our stance.
> I have turned it into a trace.
>
> Patch below.

Good. Now, for inclusion in the tree, you need to submit it
to [e-mail].