Re: ntdll / kernel32: #52

[Robert Shearman]

Eric Pouech wrote:
> Moving atom implementation to:
> - dlls/kernel32/atom16.c for the 16 bit part
> - dlls/ntdll/ntdll.c for the 32 bit part
>
> A+
>
> Name:          ntkrnl_52
> ChangeLog:
> - moved 16 bit atom support in (16bit) dlls/kernel/atom16.c
> - added atom support to NTDLL
> - made 32 bit atom support in kernel32 call ntdll's brand new one
> - copied the atom tests for global atom so that we can test local atoms too
>
> License:       X11
> GenDate:       2004/12/11 12:59:19 UTC
> ModifiedFiles: dlls/kernel/Makefile.in dlls/kernel/atom.c dlls/kernel/tests/atom.c dlls/ntdll/Makefile.in dlls/ntdll/ntdll.spec include/winternl.h server/protocol.def server/atom.c
> AddedFiles:    dlls/kernel/atom16.c dlls/ntdll/atom.c

...
> --- /dev/null	1970-01-01 01:00:00.000000000 +0100
> +++ dlls/ntdll/atom.c	2004-12-04 15:31:03.000000000 +0100
> @@ -0,0 +1,331 @@
> +/*
> -
> - Atom table functions
> -
> -
> -
> - Copyright 1993, 1994, 1995 Alexandre Julliard
> -
> - Copyright 2004 Eric Pouech
> -
> -
> -
> - This library is free software; you can redistribute it and/or
> -
> - modify it under the terms of the GNU Lesser General Public
> -
> - License as published by the Free Software Foundation; either
> -
> - version 2.1 of the License, or (at your option) any later version.
> -
> -
> -
> - This library is distributed in the hope that it will be useful,
> -
> - but WITHOUT ANY WARRANTY; without even the implied warranty of
> -
> - MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> -
> - Lesser General Public License for more details.
> -
> -
> -
> - You should have received a copy of the GNU Lesser General Public
> -
> - License along with this library; if not, write to the Free Software
> -
> - Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
> - */
> -
>
> +#include "config.h"
> +#include "wine/port.h"
> -
>
> +#include <stdlib.h>
> +#include <stdarg.h>
> +#include <stdio.h>
> +#include <string.h>
> +#include <ctype.h>
> -
>
> +#include "windef.h"
> +#include "winbase.h"
> +#include "ntstatus.h"
> -
>
> +#include "wine/server.h"
> +#include "wine/unicode.h"
> -
>
> +#include "wine/debug.h"
> -
>
> +WINE_DEFAULT_DEBUG_CHANNEL(atom);
> -
>
> +#define DEFAULT_ATOMTABLE_SIZE    37
> +#define MAX_ATOM_LEN              255
> -
>
> +/******************************************************************
> -
> -
> is_integral_atom
> -
> - Returns STATUS_SUCCESS if integral atom and 'pAtom' is filled
> -
> -
>     STATUS_INVALID_PARAMETER if 'atomstr' is too long
> -
> -
>     STATUS_MORE_ENTRIES otherwise
> - */
>
> +static NTSTATUS is_integral_atom( LPCWSTR atomstr, ATOM* pAtom )
> +{
> - ATOM atom;
> -
> - if (HIWORD( atomstr ))
> - {
> -
>    const WCHAR* ptr = atomstr;
> -
>    if (*ptr++ == '#')
> -
>    {
> -
>        atom = 0;
> -
>        while (*ptr >= '0' && *ptr <= '9')
> -
>        {
> -
>            atom = atom * 10 + *ptr++ - '0';
> -
>        }
> -
>        if (ptr > atomstr + 1 && !*ptr) goto done;
> -
>    }
> -
>    if (strlenW( atomstr ) > MAX_ATOM_LEN) return STATUS_INVALID_PARAMETER;
> -
>    return STATUS_MORE_ENTRIES;
> - }
> - else atom = LOWORD( atomstr );
>
> +done:
> - if (atom >= MAXINTATOM) return STATUS_INVALID_PARAMETER;
> - *pAtom = atom;
> - return STATUS_SUCCESS;
>
> +}
> -
>
> +/******************************************************************
> -
> -
> RtlDeleteAtomFromAtomTable (NTDLL.@)
> - */
>
> +NTSTATUS WINAPI RtlDeleteAtomFromAtomTable( void* table, ATOM atom )
> +{
> - NTSTATUS    status;
> -
> - TRACE( "%p %x\n", table, atom );
> - SERVER_START_REQ( delete_atom )
> - {
> -
>    req->atom = atom;
> -
>    req->table = table;
> -
>    status = wine_server_call( req );
> - }
> - SERVER_END_REQ;
> - return status;
>
> +}

Rtl* functions should generally avoid doing server calls. You should 
implement the Rtl* functions on top of the Nt* functions, not the other 
way around.

...
> +/******************************************************************
> -
> -
> NtDeleteAtom (NTDLL.@)
> - */
>
> +NTSTATUS WINAPI NtDeleteAtom(ATOM atom)
> +{
> - return RtlDeleteAtomFromAtomTable( GLOBAL_ATOM_TABLE, atom );
>
> +}

Rob

ntdll / kernel32: #52

[Eric Pouech]

> Rtl* functions should generally avoid doing server calls. You should 
> implement the Rtl* functions on top of the Nt* functions, not the other 
> way around.

the "real" fix would mean (which I decided not to implement)
- not moving local atom tables to wineserver (and handling them in the local memory)
- that would by the way allow to use the internal representation of local atom 
tables
- then, just Nt atom related functions would call wineserver

Deciding to keep the code in one place (wineserver) leads to the Nt -> Rtl calls.
A+

ntdll / kernel32: #52

[Dmitry Timoshkov]

"Eric Pouech"  wrote:
> > Rtl* functions should generally avoid doing server calls. You should 
> > implement the Rtl* functions on top of the Nt* functions, not the other 
> > way around.
>
> the "real" fix would mean (which I decided not to implement)
> - not moving local atom tables to wineserver (and handling them in the local memory)
> - that would by the way allow to use the internal representation of local atom
>
> tables
> - then, just Nt atom related functions would call wineserver

Moving local atom support to wineserver makes it also a lot slower,
and that's not acceptable IMO.

ntdll / kernel32: #52

[Eric Pouech]

Dmitry Timoshkov a e'crit :
> Moving local atom support to wineserver makes it also a lot slower,
> and that's not acceptable IMO.

local atom is already in wineserver (before the patch) ;-)
only 16 bit atom is in kernel
A+

ntdll / kernel32: #52

[Dmitry Timoshkov]

"Eric Pouech"  wrote:
> > Moving local atom support to wineserver makes it also a lot slower,
> > and that's not acceptable IMO.
>
> local atom is already in wineserver (before the patch) ;-)
> only 16 bit atom is in kernel

Oops, sorry for the confusion then. I didn't really look at the code,
only was reading your comments and was under impression that was the
case. Sorry again. Do you know how NT handles that?

ntdll / kernel32: #52

[Eric Pouech]

> Oops, sorry for the confusion then. I didn't really look at the code,
> only was reading your comments and was under impression that was the
> case. Sorry again. Do you know how NT handles that?

I'd say local & global are really the same stuff, both reside in kernel space, 
but the second is shared across process while the first isn't.

A+