[ros-dev] bug in vfatfs.sys?(about Alex comments)
Oriol
oripipa at yahoo.es
Fri Jan 4 16:02:11 CET 2008
Thankyou Alex for your comments, I modified some sources without knowing
what was i doing ..i was testing, i'll remove these useless patches.
Alex Ionescu wrote:
>
> On 3-Jan-08, at 4:41 PM, Oriol wrote:
>
>> I get an error while trying to install Firefox 2.0 on a bootcd
>> compilation, finally i found the possibly bug is in vfatfs.sys i
>> modified some lines but is not my code.....it seems to install Firefox.
>> Also, testing firefox i found problems with gdi and also done some
>> modifications, still testing them..
>> Here is my patch..NO WARRANTY he he..
>>
>> Index: tools/rbuild/configuration.cpp
>> ===================================================================
>> --- tools/rbuild/configuration.cpp (revision 31587)
>> +++ tools/rbuild/configuration.cpp (working copy)
>> @@ -28,7 +28,7 @@
>> CheckDependenciesForModuleOnly = false;
>> CompilationUnitsEnabled = true;
>> MakeHandlesInstallDirectories = false;
>> - GenerateProxyMakefilesInSourceTree = false;
>> + GenerateProxyMakefilesInSourceTree = true;
>> InstallFiles = false;
>> UseConfigurationInPath = false;
>> UseVSVersionInPath = false;
>
> What? Why? How does this help fix a VFAT bug?
>
>> Index: ntoskrnl/include/internal/mm.h
>> ===================================================================
>> --- ntoskrnl/include/internal/mm.h (revision 31587)
>> +++ ntoskrnl/include/internal/mm.h (working copy)
>> @@ -103,7 +103,7 @@
>> /*
>> * Maximum size of the kmalloc area (this is totally arbitary)
>> */
>> -#define MM_KERNEL_MAP_SIZE (16*1024*1024)
>> +#define MM_KERNEL_MAP_SIZE (64*1024*1024)
>> #define MM_KERNEL_MAP_BASE (0xf0c00000)
>>
>> /*
>
> What? Nobody uses this value anymore, and why would increasing it to
> 64MB help? These values need to go away.
>
>> Index: ntoskrnl/mm/rpoolmgr.h
>> ===================================================================
>> --- ntoskrnl/mm/rpoolmgr.h (revision 31587)
>> +++ ntoskrnl/mm/rpoolmgr.h (working copy)
>> @@ -32,7 +32,7 @@
>>
>> #ifndef R_QUECOUNT
>> // 16, 32, 64, 128, 256, 512
>> -#define R_QUECOUNT 6
>> +#define R_QUECOUNT 512
>> #endif//R_QUECOUNT
>>
>
> This slows down the pool considerably.
>
>>
>> #ifndef R_RZ
>> @@ -50,7 +50,7 @@
>>
>> #ifndef R_STACK
>> // R_STACK is the number of stack entries to store in blocks for
>> debug purposes
>> -#define R_STACK 6
>> +#define R_STACK 32
>> #else // R_STACK
>> #if R_STACK > 0 && R_STACK < 6
>> /* Increase the frame depth to get a reasonable back trace */
>> @@ -152,7 +152,7 @@
>> }
>> #endif//R_STACK
>
> This just makes debugging easier but uses 6x more memory per pool
> allocation and is much slower.
>
>>
>> -static int
>> +static rulong
>> RQueWhich ( rulong size )
>> {
>> rulong que, quesize;
>> @@ -888,7 +888,7 @@
>> rulong UsedSize;
>> PR_FREE FreeBlock;
>> rulong UserSize;
>> - int que;
>> + long que;
>>
>> ASSERT(pool);
>> if ( !Addr )
>
> What? Huh? How does this fix VFAT or do anything?
>
>> Index: base/system/winlogon/screensaver.c
>> ===================================================================
>> --- base/system/winlogon/screensaver.c (revision 31587)
>> +++ base/system/winlogon/screensaver.c (working copy)
>> @@ -3,7 +3,7 @@
>> * PROJECT: ReactOS Winlogon
>> * FILE: base/system/winlogon/screensaver.c
>> * PURPOSE: Screen saver management
>> - * PROGRAMMERS: Herv� Poussineau (hpoussin at reactos.org)
>> + * PROGRAMMERS: Herv� Poussineau (hpoussin at reactos.org)
>> */
>>
>> /* INCLUDES
>> *****************************************************************/
>> @@ -43,20 +43,30 @@
>> OUT LPDWORD Timeout)
>> {
>> BOOL Enabled;
>> -
>> + if(Timeout==NULL)
>> + {
>> + ERR("TimeOut is NULL!!!");
>> + return;
>> + }
>
> Okay...but...this isn't an optional parameter, so it *should* crash if
> it's 0.
>
>> if (!SystemParametersInfoW(SPI_GETSCREENSAVETIMEOUT, 0, Timeout, 0))
>> {
>> - WARN("WL: Unable to get screen saver timeout (error %lu).
>> Disabling it\n", GetLastError());
>> - *Timeout = INFINITE;
>> + ERR("WL: Unable to get screen saver timeout (error %lu).
>> Disabling it\n", GetLastError());
>> + if(Timeout==NULL)
>> + {
>> + ERR("TimeOut is still NULL!!!");
>> + return;
>> + }
>
> How can it be still NULL if you've already exited the function above?!
>
>
>> + else
>> + *Timeout = INFINITE;
>> }
>> else if (!SystemParametersInfoW(SPI_GETSCREENSAVEACTIVE, 0,
>> &Enabled, 0))
>> {
>> - WARN("WL: Unable to check if screen saver is enabled (error
>> %lu). Disabling it\n", GetLastError());
>> + ERR("WL: Unable to check if screen saver is enabled (error
>> %lu). Disabling it\n", GetLastError());
>> *Timeout = INFINITE;
>> }
>> else if (!Enabled)
>> {
>> - TRACE("WL: Screen saver is disabled\n");
>> + ERR("WL: Screen saver is disabled\n");
>> *Timeout = INFINITE;
>> }
>> else
>> @@ -84,7 +94,7 @@
>> if (!ImpersonateLoggedOnUser(Session->UserToken))
>> {
>> ERR("ImpersonateLoggedOnUser() failed with error %lu\n",
>> GetLastError());
>> - return 0;
>> + goto cleanup;
>> }
>>
>> Session->hUserActivity = CreateEventW(NULL, FALSE, FALSE, NULL);
>
> What do these useless screensaver debug print changes have to do with VFAT?
>
>> Index: base/system/winlogon/setup.c
>> ===================================================================
>> --- base/system/winlogon/setup.c (revision 31587)
>> +++ base/system/winlogon/setup.c (working copy)
>> @@ -19,13 +19,15 @@
>> DWORD
>> GetSetupType(VOID)
>> {
>> + //CONTEXT("GetSetupType");
>> +
>
> Huh?
>
>> DWORD dwError;
>> HKEY hKey;
>> DWORD dwType;
>> DWORD dwSize;
>> DWORD dwSetupType;
>>
>> - TRACE("GetSetupType()\n");
>> + ERR("GetSetupType()\n");
>>
>> /* Open key */
>> dwError = RegOpenKeyExW(
>> @@ -48,11 +50,12 @@
>> &dwSize);
>>
>> /* Close key, and check if returned values are correct */
>> - RegCloseKey(hKey);
>> + if(dwError==ERROR_SUCCESS)
>> + RegCloseKey(hKey);
>
> You're leaking the key handle now!
>
>> if (dwError != ERROR_SUCCESS || dwType != REG_DWORD || dwSize !=
>> sizeof(DWORD))
>> return 0;
>>
>> - TRACE("GetSetupType() returns %lu\n", dwSetupType);
>> + ERR("GetSetupType() returns %lu\n", dwSetupType);
>> return dwSetupType;
>> }
>>
>> @@ -71,7 +74,7 @@
>> DWORD dwSize;
>> DWORD dwExitCode;
>>
>> - TRACE("RunSetup() called\n");
>> + ERR("RunSetup() called\n");
>>
>> /* Open key */
>> dwError = RegOpenKeyExW(
>> @@ -107,7 +110,7 @@
>> else
>> return FALSE;
>>
>> - TRACE("Should run '%s' now\n", debugstr_w(CommandLine));
>> + ERR("Should run '%s' now\n", debugstr_w(CommandLine));
>>
>> /* Start process */
>> StartupInfo.cb = sizeof(StartupInfo);
>> @@ -130,7 +133,7 @@
>> &ProcessInformation);
>> if (!Result)
>> {
>> - TRACE("Failed to run setup process\n");
>> + ERR("Failed to run setup process\n");
>> return FALSE;
>> }
>>
>> @@ -143,7 +146,7 @@
>> CloseHandle(ProcessInformation.hThread);
>> CloseHandle(ProcessInformation.hProcess);
>>
>> - TRACE ("RunSetup() done\n");
>> + ERR ("RunSetup() done\n");
>>
>> return TRUE;
>> }
>
> How does spamming the debug log help VFAT?
>
>> Index: dll/win32/newdev/newdev.c
>> ===================================================================
>> --- dll/win32/newdev/newdev.c (revision 31587)
>> +++ dll/win32/newdev/newdev.c (working copy)
>> @@ -1,7 +1,7 @@
>> /*
>> * New device installer (newdev.dll)
>> *
>> - * Copyright 2005-2006 Herv� Poussineau (hpoussin at reactos.org)
>> + * Copyright 2005-2006 Herv� Poussineau (hpoussin at reactos.org)
>> * 2005 Christoph von Wittich (Christoph at ActiveVB.de)
>> *
>> * This library is free software; you can redistribute it and/or
>> @@ -718,7 +718,7 @@
>> {
>> TRACE("SetupDiGetDeviceRegistryProperty() failed with error
>> 0x%x (InstanceId %s)\n",
>> GetLastError(), debugstr_w(InstanceId));
>> - goto cleanup;
>> + //goto cleanup;
>
> Now this code won't return and who knows what side effects that causes...
>
>> }
>>
>> if (SetupDiGetDeviceRegistryPropertyW(
>> Index: dll/win32/gdi32/objects/dc.c
>> ===================================================================
>> --- dll/win32/gdi32/objects/dc.c (revision 31587)
>> +++ dll/win32/gdi32/objects/dc.c (working copy)
>> @@ -58,12 +58,12 @@
>> hspool,
>> (PVOID) NULL, // NULL for now.
>> (PVOID) &UMdhpdev );
>> -#if 0
>> +#if 1
>> // Handle something other than a normal dc object.
>> if (GDI_HANDLE_GET_TYPE(hDC) != GDI_OBJECT_TYPE_DC)
>> {
>> - PDC_ATTR Dc_Attr;
>> - PLDC pLDC;
>> + PDC_ATTR Dc_Attr=NULL;
>> + PLDC pLDC=NULL;
>>
>> GdiGetHandleUserData((HGDIOBJ) hDC, GDI_OBJECT_TYPE_DC, (PVOID)
>> &Dc_Attr);
>>
>> @@ -254,7 +254,7 @@
>> DeleteDC(HDC hDC)
>> {
>> BOOL Ret = TRUE;
>> -#if 0
>> +#if 1
>> PDC_ATTR Dc_Attr;
>> PLDC pLDC;
>>
>> @@ -1342,9 +1342,9 @@
>> SelectObject(HDC hDC,
>> HGDIOBJ hGdiObj)
>> {
>> - PDC_ATTR pDc_Attr;
>> -// HGDIOBJ hOldObj = NULL;
>> -// PTEB pTeb;
>> + PDC_ATTR pDc_Attr=NULL;
>> + HGDIOBJ hOldObj = NULL;
>> + //PTEB pTeb=NULL;
>>
>> if(!GdiGetHandleUserData(hDC, GDI_OBJECT_TYPE_DC, (PVOID)&pDc_Attr))
>> {
>> @@ -1355,9 +1355,10 @@
>> hGdiObj = GdiFixUpHandle(hGdiObj);
>> if (!GdiIsHandleValid(hGdiObj))
>> {
>> + DPRINT1("SelectObject:Handle not valid");
>> return NULL;
>> }
>> -
>> +
>> UINT uType = GDI_HANDLE_GET_TYPE(hGdiObj);
>>
>> switch (uType)
>> @@ -1369,28 +1370,39 @@
>> return NtGdiSelectBitmap(hDC, hGdiObj);
>>
>> case GDI_OBJECT_TYPE_BRUSH:
>> -#if 0 // enable this when support is ready in win32k
>> - hOldObj = pDc_Attr->hbrush;
>> - pDc_Attr->ulDirty_ |= DC_BRUSH_DIRTY;
>> - pDc_Attr->hbrush = hGdiObj;
>> +#if 1 // enable this when support is ready in win32k
>> + if(pDc_Attr)
>> + {
>> + DPRINT1("SelectObject:Selectng brush & returning old
>> brush");
>> + hOldObj = pDc_Attr->hbrush;
>> + pDc_Attr->ulDirty_ |= DC_BRUSH_DIRTY;
>> + pDc_Attr->hbrush = hGdiObj;
>> + }
>> + NtGdiSelectBrush(hDC, hGdiObj);
>> return hOldObj;
>> #endif
>> - return NtGdiSelectBrush(hDC, hGdiObj);
>> +
>>
>> case GDI_OBJECT_TYPE_PEN:
>> // case GDI_OBJECT_TYPE_EXTPEN:
>> -#if 0 // enable this when support is ready in win32k
>> - hOldObj = pDc_Attr->hpen;
>> - pDc_Attr->ulDirty_ |= DC_PEN_DIRTY;
>> - pDc_Attr->hpen = hGdiObj;
>> +#if 1 // enable this when support is ready in win32k
>> + if(pDc_Attr)
>> + {
>> + DPRINT1("SelectObject:Selectng pen & returning old
>> pen");
>> + hOldObj = pDc_Attr->hpen;
>> + pDc_Attr->ulDirty_ |= DC_PEN_DIRTY;
>> + pDc_Attr->hpen = hGdiObj;
>> + }
>> + NtGdiSelectPen(hDC, hGdiObj);
>> return hOldObj;
>> #endif
>> - return NtGdiSelectPen(hDC, hGdiObj);
>> +
>>
>> case GDI_OBJECT_TYPE_FONT:
>> #if 0
>> pTeb = NtCurrentTeb();
>> - if (((pTeb->GdiTebBatch.HDC == 0) ||
>> +
>> + if (pTeb!=NULL && ((pTeb->GdiTebBatch.HDC == 0) ||
>> (pTeb->GdiTebBatch.HDC == (ULONG)hDC)) &&
>> ((pTeb->GdiTebBatch.Offset + sizeof(GDIBSOBJECT)) <=
>> GDIBATCHBUFSIZE) &&
>> (!(pDc_Attr->ulDirty_ & DC_DIBSECTION)))
>
> The TEB can't be NULL, and why did you re-enable James' disabled code?
> He disabled it for a reason. How is this related to VFAT?
>
>> Index: dll/win32/gdi32/misc/misc.c
>> ===================================================================
>> --- dll/win32/gdi32/misc/misc.c (revision 31587)
>> +++ dll/win32/gdi32/misc/misc.c (working copy)
>> @@ -129,18 +129,23 @@
>> // Need to test if we have Read & Write access to the VM address
>> space.
>> //
>> BOOL Result = TRUE;
>> - if(Entry->UserData)
>> + if(Entry && Entry->UserData)
>> {
>> volatile CHAR *Current = (volatile CHAR*)Entry->UserData;
>> - _SEH_TRY
>> + if(Current==NULL)
>> + Result=FALSE;
>
> How can Current be NULL? You've already checked it's not NULL and you've
> got SEH below...
>
>> + else
>> {
>> - *Current = *Current;
>> + _SEH_TRY
>> + {
>> + *Current = *Current;
>> + }
>> + _SEH_HANDLE
>> + {
>> + Result = FALSE;
>> + }
>> + _SEH_END
>> }
>> - _SEH_HANDLE
>> - {
>> - Result = FALSE;
>> - }
>> - _SEH_END
>> }
>> else
>> Result = FALSE; // Can not be zero.
>> _______________________________________________
>> Ros-dev mailing list
>> Ros-dev at reactos.org
>> http://www.reactos.org/mailman/listinfo/ros-dev
>
>
> This almost look like a robot-generated patch of completely random,
> useless and broken code changes. How is any of this useful to FireFox
> and VFAT?
>
> Best regards,
> Alex Ionescu
>
>
>
> ------------------------------------------------------------------------
>
> _______________________________________________
> Ros-dev mailing list
> Ros-dev at reactos.org
> http://www.reactos.org/mailman/listinfo/ros-dev
______________________________________________
LLama Gratis a cualquier PC del Mundo.
Llamadas a fijos y móviles desde 1 céntimo por minuto.
http://es.voice.yahoo.com
More information about the Ros-dev
mailing list