Re: Janitorial dlls/advapi32/registry.c W->A cleanup

[Dmitry Timoshkov]

"Tony Lambregts"  wrote:
> -LONG WINAPI RegSaveKeyW( HKEY hkey, LPCWSTR file, LPSECURITY_ATTRIBUTES sa )
> +LONG WINAPI RegSaveKeyA( HKEY hkey, LPCSTR file, LPSECURITY_ATTRIBUTES sa )
>  {
> - LPSTR fileA = HEAP_strdupWtoA( GetProcessHeap(), 0, file );
> - DWORD ret = RegSaveKeyA( hkey, fileA, sa );
> - if (fileA) HeapFree( GetProcessHeap(), 0, fileA );
> - UNICODE_STRING fileW;
> - LONG ret;
> -
> - RtlCreateUnicodeStringFromAsciiz( &fileW, file );
> - ret = RegSaveKeyW( hkey, fileW.Buffer, sa );
> - RtlFreeUnicodeString( &fileW );
> return ret;

In the vast majority of cases where an API takes file name, ANSI version
of the API is supposed to have a limitation of MAX_PATH characters for
the name.

Thus, there is no need to waste CPU cycles by allocating/deallocating
memory, but instead having an automatic buffer on the stack will be
quite enough. See files/drive.c,GetCurrentDirectoryA for a sample.

All other APIs which get a file name as a parameter should be rewritten
that way too. Probably that's the task for yet another janitorial project...

All the said above doesn't mean that your patch can't applied as is,
that just means that there is a better way of doing this task.

Janitorial dlls/advapi32/registry.c W->A cleanup

[Tony Lambregts]

Dmitry Timoshkov wrote:
> "Tony Lambregts"  wrote:
> > -LONG WINAPI RegSaveKeyW( HKEY hkey, LPCWSTR file, LPSECURITY_ATTRIBUTES sa )
> > +LONG WINAPI RegSaveKeyA( HKEY hkey, LPCSTR file, LPSECURITY_ATTRIBUTES sa )
> > {
> > - LPSTR fileA = HEAP_strdupWtoA( GetProcessHeap(), 0, file );
> > - DWORD ret = RegSaveKeyA( hkey, fileA, sa );
> > - if (fileA) HeapFree( GetProcessHeap(), 0, fileA );
> > - UNICODE_STRING fileW;
> > - LONG ret;
> > -
> > - RtlCreateUnicodeStringFromAsciiz( &fileW, file );
> > - ret = RegSaveKeyW( hkey, fileW.Buffer, sa );
> > - RtlFreeUnicodeString( &fileW );
> > return ret;
>
> In the vast majority of cases where an API takes file name, ANSI version
> of the API is supposed to have a limitation of MAX_PATH characters for
> the name.
>
> Thus, there is no need to waste CPU cycles by allocating/deallocating
> memory, but instead having an automatic buffer on the stack will be
> quite enough. See files/drive.c,GetCurrentDirectoryA for a sample.
>
> All other APIs which get a file name as a parameter should be rewritten
> that way too. Probably that's the task for yet another janitorial project...
>
> All the said above doesn't mean that your patch can't applied as is,
> that just means that there is a better way of doing this task.

Yeah I actually had it that way at one point due to my familairity with 
drive.c and well... I ran into some compiler errors (in RegSaveKeyW) 
that ended up making the problem worse. Perhaps it was something I 
simple I missed. I can take another look at it if it will save some CPU 
cycles.

Actually I was more concerned about this section

if (!(hkey = get_special_root_hkey( hkey ))) return ERROR_INVALID_HANDLE;

err = GetLastError();
-    GetFullPathNameA( file, sizeof(buffer), buffer, &name );
+    GetFullPathNameW( file, sizeof(buffer), buffer, &nameW );
+    len = WideCharToMultiByte(CP_ACP, 0, nameW, -1, NULL, 0, NULL, NULL);
+    nameA = HeapAlloc(GetProcessHeap(), 0, len);
+    WideCharToMultiByte(CP_ACP, 0, nameW, -1, nameA, len, NULL, NULL);
+    
     for (;;)
     {
-        sprintf( name, "reg%04x.tmp", count++ );
-        handle = CreateFileA( buffer, GENERIC_WRITE, 0, NULL,
+        sprintf( nameA, "reg%04x.tmp", count++ );
+        handle = CreateFileW( buffer, GENERIC_WRITE, 0, NULL,
                             CREATE_NEW, FILE_ATTRIBUTE_NORMAL, 0 );
         if (handle != INVALID_HANDLE_VALUE) break;
         if ((ret = GetLastError()) != ERROR_ALREADY_EXISTS) goto done;

I could not use swprintf here. At least not without msvcrt <grin>  So I 
resorted to converting to ASCII<frown>

Janitorial dlls/advapi32/registry.c W->A cleanup

[Tony Lambregts]

Tony Lambregts wrote:
> Dmitry Timoshkov wrote:
> > "Tony Lambregts"  wrote:
> > > -LONG WINAPI RegSaveKeyW( HKEY hkey, LPCWSTR file, 
> > > LPSECURITY_ATTRIBUTES sa )
> > > +LONG WINAPI RegSaveKeyA( HKEY hkey, LPCSTR file, 
> > > LPSECURITY_ATTRIBUTES sa )
> > > {
> > > - LPSTR fileA = HEAP_strdupWtoA( GetProcessHeap(), 0, file );
> > > - DWORD ret = RegSaveKeyA( hkey, fileA, sa );
> > > - if (fileA) HeapFree( GetProcessHeap(), 0, fileA );
> > > - UNICODE_STRING fileW;
> > > - LONG ret;
> > > -
> > > - RtlCreateUnicodeStringFromAsciiz( &fileW, file );
> > > - ret = RegSaveKeyW( hkey, fileW.Buffer, sa );
> > > - RtlFreeUnicodeString( &fileW );
> > > return ret;
> >
> > In the vast majority of cases where an API takes file name, ANSI version
> > of the API is supposed to have a limitation of MAX_PATH characters for
> > the name.
> >
> > Thus, there is no need to waste CPU cycles by allocating/deallocating
> > memory, but instead having an automatic buffer on the stack will be
> > quite enough. See files/drive.c,GetCurrentDirectoryA for a sample.
> >
> > All other APIs which get a file name as a parameter should be rewritten
> > that way too. Probably that's the task for yet another janitorial 
> > project...
> >
> > All the said above doesn't mean that your patch can't applied as is,
> > that just means that there is a better way of doing this task.
>
> Yeah I actually had it that way at one point due to my familairity 
> with drive.c and well... I ran into some compiler errors (in 
> RegSaveKeyW) that ended up making the problem worse. Perhaps it was 
> something I simple I missed. I can take another look at it if it will 
> save some CPU cycles.

when I change the code to the following
LONG WINAPI RegSaveKeyA( HKEY hkey, LPCSTR file, LPSECURITY_ATTRIBUTES sa )
{
    WCHAR fileW[MAX_PATH];
    LONG  ret, len;

len = WideCharToMultiByte(CP_ACP, 0, fileW, -1, file, 0, NULL, NULL);
    WideCharToMultiByte(CP_ACP, 0, fileW, -1, file, len, NULL, NULL);
    ret = RegSaveKeyW( hkey, fileW.Buffer, sa );
    return ret;
}

I get the following warning

warning: passing arg 5 of `WideCharToMultiByte' discards qualifiers from 
pointer target type

WideCharToMultiByte does not like LPCSTR. So what to do?

Janitorial dlls/advapi32/registry.c W->A cleanup

[Vincent Béron]

Tony Lambregts a écrit:
> when I change the code to the following
> LONG WINAPI RegSaveKeyA( HKEY hkey, LPCSTR file, LPSECURITY_ATTRIBUTES sa )
> {
>    WCHAR fileW[MAX_PATH];
>    LONG  ret, len;
>      len = WideCharToMultiByte(CP_ACP, 0, fileW, -1, file, 0, NULL, NULL);
>    WideCharToMultiByte(CP_ACP, 0, fileW, -1, file, len, NULL, NULL);
>    ret = RegSaveKeyW( hkey, fileW.Buffer, sa );
>    return ret;
> }
>
> I get the following warning
>
> warning: passing arg 5 of `WideCharToMultiByte' discards qualifiers from 
> pointer target type
>
> WideCharToMultiByte does not like LPCSTR. So what to do?

Of course it doesn't, it's the destination :)
MultiByteToWideChar will do the file ==> fileW conversion.

Vincent

Janitorial dlls/advapi32/registry.c W->A cleanup

[Alexandre Julliard]

"Dmitry Timoshkov"  writes:
> Thus, there is no need to waste CPU cycles by allocating/deallocating
> memory, but instead having an automatic buffer on the stack will be
> quite enough. See files/drive.c,GetCurrentDirectoryA for a sample.
>
> All other APIs which get a file name as a parameter should be rewritten
> that way too. Probably that's the task for yet another janitorial project...

I don't think I agree with that. Our file I/O functions already use
way too much stack precisely because we use MAX_PATH stack buffers all
over the place, and we should be getting rid of them, not adding even
more.

Note that in advapi32, you can use the static Unicode buffer in the
TEB to avoid memory allocations. We could probably use it in at least
some of the kernel ANSI functions too.

Janitorial dlls/advapi32/registry.c W->A cleanup

[Dmitry Timoshkov]

"Alexandre Julliard"  wrote:
> > Thus, there is no need to waste CPU cycles by allocating/deallocating
> > memory, but instead having an automatic buffer on the stack will be
> > quite enough. See files/drive.c,GetCurrentDirectoryA for a sample.
> >
> > All other APIs which get a file name as a parameter should be rewritten
> > that way too. Probably that's the task for yet another janitorial project...
>
> I don't think I agree with that. Our file I/O functions already use
> way too much stack precisely because we use MAX_PATH stack buffers all
> over the place, and we should be getting rid of them, not adding even
> more.
>
> Note that in advapi32, you can use the static Unicode buffer in the
> TEB to avoid memory allocations. We could probably use it in at least
> some of the kernel ANSI functions too.

I was going to object, but decided to look at the real thing first.
You are right Alexandre, kernel32.dll from win2k uses unicode buffer in
the TEB for all the ANSI APIs I looked at. So yes, that's the way to go.

Janitorial dlls/advapi32/registry.c W->A cleanup

[Tony Lambregts]

Dmitry Timoshkov wrote:
> "Alexandre Julliard"  wrote:
> > > Thus, there is no need to waste CPU cycles by allocating/deallocating
> > > memory, but instead having an automatic buffer on the stack will be
> > > quite enough. See files/drive.c,GetCurrentDirectoryA for a sample.
> > >
> > > All other APIs which get a file name as a parameter should be rewritten
> > > that way too. Probably that's the task for yet another janitorial project...
> >
> > I don't think I agree with that. Our file I/O functions already use
> > way too much stack precisely because we use MAX_PATH stack buffers all
> > over the place, and we should be getting rid of them, not adding even
> > more.
> >
> > Note that in advapi32, you can use the static Unicode buffer in the
> > TEB to avoid memory allocations. We could probably use it in at least
> > some of the kernel ANSI functions too.
>
> I was going to object, but decided to look at the real thing first.
> You are right Alexandre, kernel32.dll from win2k uses unicode buffer in
> the TEB for all the ANSI APIs I looked at. So yes, that's the way to go.

For the benefit of those of us who are a little thick could someone 
explain what that means. I understand that the TEB has TlsSlots for 
local storage but.. there is so much I do not understand. A little help...

Janitorial dlls/advapi32/registry.c W->A cleanup

[Dmitry Timoshkov]

"Tony Lambregts"  wrote:
> For the benefit of those of us who are a little thick could someone 
> explain what that means. I understand that the TEB has TlsSlots for 
> local storage but.. there is so much I do not understand. A little help...

The TEB (see include/thread.h) has a specially reserved buffer (StaticUnicodeBuffer)
with the size of MAX_PATH+1 unicode characters and UNICODE_STRING structure
(StaticUnicodeString) pointing to that buffer. That space could be used for
in order to speed up things by avoiding memory allocation.

Here is a sample implementation of CreateDirectoryA that uses TEB static
unicode buffer. I have created a helper function asciiz_to_unicode which
takes an ASCIIZ string, converts it to unicode and returns a pointer to
the UNICODE_STRING structure (i.e. it also returns the size of the resulting
unicode string). I think that using a helper can save a bit of memory
isolating a commonly used code into a single place, and makes it simpler
 to modify the code if we will later decide to change the implementation.

Feel free to use it as a guide for converting ANSI file APIs to use
that scheme.

#include "winbase.h"
#include "winerror.h"
#include "winternl.h"
#include "thread.h"

/*
 * asciiz_to_unicode (internal)
 * Does not check for NULL. This is a duty of a higher level API.
 */
UNICODE_STRING *asciiz_to_unicode(LPCSTR src)
{
    UNICODE_STRING *unicode = &NtCurrentTeb()->StaticUnicodeString;
    STRING ansi;
    NTSTATUS status;

RtlInitAnsiString(&ansi, src);
    status = RtlAnsiStringToUnicodeString(unicode, &ansi, FALSE);
    if (!status) return unicode;

if (status == STATUS_BUFFER_OVERFLOW)
        SetLastError(ERROR_FILENAME_EXCED_RANGE);
    else
        SetLastError(RtlNtStatusToDosError(status));
    return NULL;
}

/***********************************************************************
 *           CreateDirectoryA   (KERNEL32.@)
 */
BOOL WINAPI CreateDirectoryA( LPCSTR path,
                                  LPSECURITY_ATTRIBUTES lpsecattribs )
{
    UNICODE_STRING *pathW;

if (!path || !*path)
    {
        SetLastError(ERROR_PATH_NOT_FOUND);
        return FALSE;
    }

pathW = asciiz_to_unicode(path);
    if (pathW) return CreateDirectoryW(pathW->Buffer, lpsecattribs);
    return FALSE;
}

Janitorial dlls/advapi32/registry.c W->A cleanup

[Dimitrie O. Paun]

On March 7, 2003 01:18 am, Dmitry Timoshkov wrote:
> I think that using a helper [...] makes it simpler
>  to modify the code if we will later decide to change the implementation.

I don't think this is a consideration. The way the wrapper is
written allows for no other implementation, really, since
you don't have a free call...

But I don't think it's worth worrying about such flexibility.
Using the TEB static buffer makes sense, let's do that, with
or without the wrapper.