Re: [ros-dev] bug in vfatfs.sys?(about Alex comments)

[Oriol]

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 ([e-mail])
> > PROGRAMMERS:     Herv� Poussineau ([e-mail])
> > */
> >
> > /* 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 ([e-mail])
> > Copyright 2005-2006 Herv� Poussineau ([e-mail])
> >       2005 Christoph von Wittich ([e-mail])
> > 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
> > [e-mail]
> > 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
> [e-mail]
> 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