RE: dlls/kernel/console.c::SetConsoleCtrlHandler() cleanup/fixup

[Robert Shearman]

Peter Riocreux wrote:
> "Dimitrie O. Paun" <dpaun(a)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