Re: [ros-svn] [ion] 13832: - FreeLdr Part II (ntoskrnl is now relocated, removes 3GB compiler flag). Note that there is a bug in LD which Filip and I are examining, so do not try this yet.

[James Tabor]

BugCheck Alert!

ion(a)svn.reactos.com wrote:
>       - FreeLdr Part II (ntoskrnl is now relocated,
> removes 3GB compiler flag). Note that there is a bug in LD which Filip and I are
> examining, so do not try this yet. 
>      - Fix Registry ObRef/ObDeref bug -- Hartmut
>      - Fix SID Capture Bug -- Thomas
>      - Use KPRCB pointer properly (results in more portable and much faster code)
>   
(ob/object.c:1034) Object 81d6ad68/81d6ad90 has invalid handle count (-858993460
)
KeBugCheck at ob/object.c:1035
A problem has been detected and ReactOS has been shut down to prevent damage to
your computer.

The bug code is undefined. Please use an existing code instead.

Technical information:

*** STOP: 0x00000000 (0x00000000,0x00000000,0x00000000,0x00000000)

Frames:
<ntoskrnl.exe: bf15 (ke/bug.c:425 (KeBugCheckEx))>
<ntoskrnl.exe: bf51 (ke/bug.c:445 (KeBugCheck))>
<ntoskrnl.exe: a98ba (ob/object.c:1035 (ObfDereferenceObject))>
<ntoskrnl.exe: a7310 (ob/handle.c:959 (NtClose))>
<ntoskrnl.exe: 33c2 (/tmp/cch6Lxn6.s:178 (KiSystemService))>
<658D9845>
<win32k.sys: 4b9bc (objects/text.c:2928 (TextIntRealizeFont))>
<win32k.sys: 3811d (objects/dc.c:1867 (NtGdiSelectObject))>
<ntoskrnl.exe: 33c2 (/tmp/cch6Lxn6.s:178 (KiSystemService))>
<user32.dll: 3be70 (controls/edit.c:2977 (EDIT_EM_PosFromChar))>

Entered debugger on exception number 3 (Breakpoint)

Entered kernel debugger (type "help" for a list of commands)

kdb:> bt
Frames:
<ntoskrnl.exe: 50d7 (ke/i386/brkpoint.c:50 (DbgBreakPointWithStatus))>
<ntoskrnl.exe: be64 (ke/bug.c:397 (KeBugCheckWithTf))>
<ntoskrnl.exe: bf15 (ke/bug.c:425 (KeBugCheckEx))>
<ntoskrnl.exe: bf51 (ke/bug.c:445 (KeBugCheck))>
<ntoskrnl.exe: a98ba (ob/object.c:1035 (ObfDereferenceObject))>
<ntoskrnl.exe: a7310 (ob/handle.c:959 (NtClose))>
<ntoskrnl.exe: 33c2 (/tmp/cch6Lxn6.s:178 (KiSystemService))>

<win32k.sys: 4b9bc (objects/text.c:2928 (TextIntRealizeFont))>
<win32k.sys: 3811d (objects/dc.c:1867 (NtGdiSelectObject))>
<ntoskrnl.exe: 33c2 (/tmp/cch6Lxn6.s:178 (KiSystemService))>
<user32.dll: 3be70 (controls/edit.c:2977 (EDIT_EM_PosFromChar))>
<user32.dll: 3c2e0 (controls/edit.c:1583 (EDIT_GetLineRect))>
<user32.dll: 3dd6e (controls/edit.c:1711 (EDIT_InvalidateText))>
<user32.dll: 3f927 (controls/edit.c:4791 (EDIT_WM_SetText))>
<user32.dll: 41a88 (controls/edit.c:1002 (EditWndProc_common))>
<user32.dll: 42636 (controls/edit.c:1064 (EditWndProcW))>
<user32.dll: 11ea1 (windows/message.c:964 (IntCallWindowProcW))>
<user32.dll: 13207 (windows/message.c:1868 (User32CallWindowProcFromKernel))>
<ntdll.dll: 9124 (rtl/callback.c:36 (KiUserCallbackDispatcher))>

<user32.dll: 1df55 (windows/dialog.c:528 (DIALOG_DoDialogBox))>
<user32.dll: 1e0f2 (windows/dialog.c:1632 (DialogBoxParamA))>
<taskmgr.exe: fa62 (taskmgr.c:109 (WinMain))>
<taskmgr.exe: 1242a (graphctl.c:605 (GraphCtrl_WndProc))>
<taskmgr.exe: 11e7>
<taskmgr.exe: 1258>
<kernel32.dll: 26562 (process/create.c:339 (BaseProcessStart))>

kdb:> regs
CS:EIP 0008:800050d7, EAX 00000004 EBX 00000000 ECX 8013aa10 EDX 00000000
ESI 00000000 EDI 00000000 EBP 9e9eba38 SS:ESP be64:9e9eba78
EFLAGS: IF IOPL0

kdb:> dregs
Trap   : DR0 00000000 DR1 00000000 DR2 00000000 DR3 00000000 DR6 ffff0ff0 DR7 00
000400

kdb:> cregs
CR0: PE MP TS NE WP PG
CR1 00000000 CR2 000bf000 CR3 3aa57000 CR4 00000680 TR 9e9eb9c4 LDTR 00000000

kdb:> plist
Process list: 308 taskmgr.296 ctm.EXE284 cmd.exe
268 cmd.exe256 explorer244 userinit224 umpnpmgr
208 eventlog192 services160 winlogon132 csrss.ex
104 smss.exe4 System

Wow!
James

[Alex Ionescu]

James Tabor wrote:

>   BugCheck Alert!
>
>  
Hmm...looks like after reverting Thomas's patch and using Hartmut's, the 
problems are happening again.
So, it would seem that Thomas' patch is more correct? Or perhaps both 
should be used?
T/H: Let me know!

Best regards
Alex Ionescu

[Hartmut Birr]

Alex Ionescu schrieb:

>   James Tabor wrote:
>
> >   BugCheck Alert!
> >
> >  
>  Hmm...looks like after reverting Thomas's patch and using Hartmut's,
>  the problems are happening again.
>  So, it would seem that Thomas' patch is more correct? Or perhaps both
>  should be used?
>  T/H: Let me know!
>  
Hi,  

I've written a little test program which does always crash ros with my
patch on the smp machine (up not tested). The program does create 10
threads which try to open a non existing registry key in an endless
loop. It does not crash with Thomas' patch. His patch adds one reference
to much to the first parsed/opened key. This makes it impossible to
delete a key and to unload the registry at shut down. I think we have a
problem outside from the registry. It seems there is a gap between the
deleting of an object after the last dereference operation and the next
create operation for a new object with the same name. One thread has
reopend the object and use it and the other delete the same object. I
see very often values of 0xcccccxxx on bug checks and the wrong
reference value -858993460 is also 0xcccccccc.   

- Hartmut


#include <stdlib.h>
#include <stdio.h>
#include <process.h>
#include <windows.h>

ULONG Count = 0;
volatile ULONG Terminate = 0;

DWORD WINAPI
RegTest(LPVOID param)
{
   HKEY hKey;
   LONG Result;

   while(!Terminate)
   {
#if 0
      Result = RegOpenKey(HKEY_LOCAL_MACHINE, "Software\\Microsoft\\Windows
NT\\CurrentVersion\\SysFontSubstitutes", &hKey);
      if (Result == ERROR_SUCCESS)
      {
         InterlockedIncrement(&Count);
         RegCloseKey(hKey);
      }
#endif
      Result = RegOpenKey(HKEY_LOCAL_MACHINE, "Software\\Microsoft\\Windows
NT\\CurrentVersion\\FontSubstitutes", &hKey);
      if (Result == ERROR_SUCCESS)
      {
         InterlockedIncrement(&Count);
         RegCloseKey(hKey);
      }
   }
   return 0;
}


int main(int argc, char* argv[])
{
   int i;

   printf("RegTest\n");

   for (i = 0; i < 10; i++)
   {
      CreateThread(NULL, 0, RegTest, NULL, 0, NULL);
   }

   Sleep(5000);

   Terminate = 1;

   Sleep(200);

   printf("Done (%ld)\n", Count);


   return 0;


}

[Hartmut Birr]

Hi,

I can fix my problem by protecting the access to some entries of the
object header with a global spinnlock, but I doesn't like this. Has
anyone a better idea?

- Hartmut



Hartmut Birr schrieb:

>  Alex Ionescu schrieb:
>
>   
>
> >  James Tabor wrote:
> >
> >     
> >
> > >  BugCheck Alert!
> > >
> > >
> > >       
> > >  
> > Hmm...looks like after reverting Thomas's patch and using Hartmut's,
> > the problems are happening again.
> > So, it would seem that Thomas' patch is more correct? Or perhaps both
> > should be used?
> > T/H: Let me know!
> >
> >     
> >  
> Hi,  
>
> I've written a little test program which does always crash ros with my
> patch on the smp machine (up not tested). The program does create 10
> threads which try to open a non existing registry key in an endless
> loop. It does not crash with Thomas' patch. His patch adds one reference
> to much to the first parsed/opened key. This makes it impossible to
> delete a key and to unload the registry at shut down. I think we have a
> problem outside from the registry. It seems there is a gap between the
> deleting of an object after the last dereference operation and the next
> create operation for a new object with the same name. One thread has
> reopend the object and use it and the other delete the same object. I
> see very often values of 0xcccccxxx on bug checks and the wrong
> reference value -858993460 is also 0xcccccccc.   
>
> - Hartmut
>
>   
>
> ------------------------------------------------------------------------
>
> _______________________________________________
> Ros-dev mailing list
> Ros-dev(a)reactos.com
> http://reactos.com:8080/mailman/listinfo/ros-dev
>  


M:\Sandbox\ros_work\reactos>set SVN_EDITOR=notepad 

M:\Sandbox\ros_work\reactos>d:\programme\subversion\bin\svn.exe diff
ntoskrnl\ob\object.c        
Index: ntoskrnl/ob/object.c
===================================================================
--- ntoskrnl/ob/object.c	(revision 13857)
+++ ntoskrnl/ob/object.c	(working copy)
@@ -22,6 +22,7 @@
   POBJECT_HEADER ObjectHeader;
 } RETENTION_CHECK_PARAMS, *PRETENTION_CHECK_PARAMS;
 
+KSPIN_LOCK ObpLock = 0;
 
 /* FUNCTIONS ************************************************************/
 
@@ -818,6 +819,8 @@
 			   IN KPROCESSOR_MODE AccessMode)
 {
    POBJECT_HEADER Header;
+   KIRQL oldIrql;
+   NTSTATUS Status;
    
    /* NOTE: should be possible to reference an object above APC_LEVEL! */
 
@@ -849,22 +852,32 @@
 	DPRINT("eip %x\n", ((PULONG)&Object)[-1]);
      }
  
-   if (Header->CloseInProcess)
-   {
-      if (Header->ObjectType == PsProcessType)
-        {
-	  return STATUS_PROCESS_IS_TERMINATING;
-	}
-      if (Header->ObjectType == PsThreadType)
-        {
-	  return STATUS_THREAD_IS_TERMINATING;
-	}
-      return(STATUS_UNSUCCESSFUL);
-   }
+   KeAcquireSpinLock(&ObpLock, &oldIrql);
 
-   InterlockedIncrement(&Header->RefCount);
+   if (Header->CloseInProcess ||
+       (0 == Header->RefCount && 0 == Header->HandleCount &&
!Header->Permanent))
+     {
+       if (Header->ObjectType == PsProcessType)
+         {
+	   Status = STATUS_PROCESS_IS_TERMINATING;
+	 }
+       else if (Header->ObjectType == PsThreadType)
+         {
+	   Status = STATUS_THREAD_IS_TERMINATING;
+	 }
+       else
+         {
+           Status = STATUS_UNSUCCESSFUL;
+         }
+     }
+   else
+     {
+       InterlockedIncrement(&Header->RefCount);
+       Status = STATUS_SUCCESS;
+     } 
+   KeReleaseSpinLock(&ObpLock, oldIrql);
    
-   return(STATUS_SUCCESS);
+   return Status;
 }
 
 
@@ -960,7 +973,7 @@
 
 STATIC NTSTATUS
 ObpDeleteObjectDpcLevel(IN POBJECT_HEADER ObjectHeader,
-			IN LONG OldRefCount)
+			IN KIRQL oldIrql)
 {
   if (ObjectHeader->RefCount < 0)
     {
@@ -981,11 +994,14 @@
   if (ObjectHeader->CloseInProcess)
     {
       KEBUGCHECK(0);
+      KeReleaseSpinLock(&ObpLock, oldIrql);
       return STATUS_UNSUCCESSFUL;
     }
   ObjectHeader->CloseInProcess = TRUE;
   
-  switch (KeGetCurrentIrql ())
+  KeReleaseSpinLock(&ObpLock, oldIrql);
+  
+  switch (oldIrql)
     {
     case PASSIVE_LEVEL:
       return ObpDeleteObject (ObjectHeader);
@@ -1043,18 +1059,23 @@
 ObfReferenceObject(IN PVOID Object)
 {
   POBJECT_HEADER Header;
+  KIRQL oldIrql;
 
   ASSERT(Object);
 
   Header = BODY_TO_HEADER(Object);
 
+  KeAcquireSpinLock(&ObpLock, &oldIrql);
+
   /* No one should be referencing an object once we are deleting it. */
-  if (Header->CloseInProcess)
+  if (Header->CloseInProcess || 
+      (0 == Header->RefCount && 0 == Header->HandleCount &&
!Header->Permanent))
     {
       KEBUGCHECK(0);
     }
 
   (VOID)InterlockedIncrement(&Header->RefCount);
+  KeReleaseSpinLock(&ObpLock, oldIrql);
 }
 
 
@@ -1081,9 +1102,12 @@
   LONG NewRefCount;
   BOOL Permanent;
   ULONG HandleCount;
+  KIRQL oldIrql;
 
   ASSERT(Object);
 
+  KeAcquireSpinLock(&ObpLock, &oldIrql);
+
   /* Extract the object header. */
   Header = BODY_TO_HEADER(Object);
   Permanent = Header->Permanent;
@@ -1101,8 +1125,12 @@
       HandleCount == 0 &&
       !Permanent)
     {
-      ObpDeleteObjectDpcLevel(Header, NewRefCount);
+      ObpDeleteObjectDpcLevel(Header, oldIrql);
     }
+  else
+    {
+      KeReleaseSpinLock(&ObpLock, oldIrql);
+    }
 }

[Mike Nordell]

Hartmut Birr wrote:

>   I can fix my problem by protecting the access to some
> entries of the
>  object header with a global spinnlock, but I doesn't like this. Has
>  anyone a better idea? 
I don't like it either. If this fixes your problem, it means it will affect
performance and power usage negatively (else it would have no effect for
you).

I think we might be barking up the wrong tree here, working around a problem
in the registry code (?) by adding bloat and slowing-down code to one of the
central code "services". One particular reason I believe that, is the
following I just put together from looking at what NT5sp4 seems to do.
That's btw also all it does - nothing else. Feel free to play with and
commit it.

{
if (Header->ObjectType != ObjectType /* type mismatch */&&
 (AccessMode != KernelMode /* but mismatches are OK i kmode */ ||
  ObjectType == ObSymbolicLinkType /* so long as we're not requesting a
symbolic link */))
 {
  return STATUS_OBJECT_TYPE_MISMATCH;
 }
}

InterlockedIncrement(&Header->RefCount);
}


/Mike

P.S. I think a global replacement s/ObSymbolicLinkType/ObpSymbolicLinkType/
could be prudent (adding the "p").

[Hartmut Birr]

Mike Nordell schrieb:

>  I think we might be barking up the wrong tree here,
> working around a problem
> in the registry code (?) by adding bloat and slowing-down code to one of the
> central code "services". 
>  
It isn't a problem of the registry. It is a design problem of the object
manger for all objects which have a Close and/or Delete function. There
is always a gap between dropping the reference or handle count and
calling the Close or Delete function.

- Hartmut

[Hartmut Birr]

Hi,

I've looked a little bit more in the registry and object manager
problem. I think we have a little bug in the object manager and a huge
bug in the registry code. I start with the object manager. If a handle
is created, the reference count of an object is dropped to zero. If an
object is dereferenced we must look to the handle and the reference
count. At the moment this is not an atomic operation. It is not easy to
make it to an atomic operation. If we increase the reference count at
the first created handle and decrease it on the last deleted handle, it
is not necessary to check the handle count in ObDereferenceObject or
related functions. The registry problem is a little bit more difficult.
If we look for a registry key, the parse function is used. They look for
an existing sub key object. If one is found, the parse routine reference
this object and return it. But this is not correct. The parse routine
should only use and reference this existing object, if the reference
count is greater than zero. But the status of this object isn't clear.
My idea is to split the data for the key object and the key object
itself. The parsing routine creates the data only once, but it creates
for each request a seperate key object. This is like a file systems,
which use many file objects but only one fcb for an on disk file. Any
ideas or suggestions?

- Hartmut


Hartmut Birr schrieb:

>  Mike Nordell schrieb:
>
>   
>
> >  I think we might be barking up the wrong tree
> > here, working around a problem
> > in the registry code (?) by adding bloat and slowing-down code to one of the
> > central code "services". 
> >
> >     
> >  
> It isn't a problem of the registry. It is a design problem of the object
> manger for all objects which have a Close and/or Delete function. There
> is always a gap between dropping the reference or handle count and
> calling the Close or Delete function.
>
> - Hartmut
> _______________________________________________
> Ros-dev mailing list
> Ros-dev(a)reactos.com
> http://reactos.com:8080/mailman/listinfo/ros-dev
>
>
>   
>

[Thomas Weidenmueller]

Hartmut Birr wrote:

>  If a handle
> is created, the reference count of an object is dropped to zero. If an
> object is dereferenced we must look to the handle and the reference
> count.
>  
after an object was created the reference counter should be 1. 
dereferencing that object directly after it was created should decrement 
it to 0 and delete it. the handle count should match the number of 
handles that exist for the object. When creating the first handle the 
reference counter should be incremented by 1, when deleting the last 
handle to the object it should be decremented by 1. For additional 
handles it should remain unchanged. I implemented this behavior in a 
local branch that completely replaces the handle implementation using 
generic executive handle tables. But i still have to track down a few bugs.

>  If we increase the reference count at
> the first created handle and decrease it on the last deleted handle, it
> is not necessary to check the handle count in ObDereferenceObject or
> related functions.
>  
As mentioned above I implemented this behavior in my local tree.

>  They look for
> an existing sub key object. If one is found, the parse routine reference
> this object and return it. But this is not correct. The parse routine
> should only use and reference this existing object, if the reference
> count is greater than zero.
>  
This doesn't sound right, the reference count should never be zero 
because then the object should be deleted, which means accessing it 
would be dangerous. The object in any case needs to be removed from the 
list before it gets deleted.

>  My idea is to split the data for the key object and the
> key object
> itself. The parsing routine creates the data only once, but it creates
> for each request a seperate key object.
>  
I don't really understand the advantage of this. For file systems it's 
obvious because these objects represent data streams with different 
offsets, ... But i don't really understand how it could be useful for 
registry key objects.

Best Regards,
Thomas

[Alex Ionescu]

Thomas Weidenmueller wrote:

>   Hartmut Birr wrote:
>
> >   If a handle
> >  is created, the reference count of an object is dropped to zero. If an
> >  object is dereferenced we must look to the handle and the reference
> >  count.
> >  
>  after an object was created the reference counter should be 1. 
>  dereferencing that object directly after it was created should 
>  decrement it to 0 and delete it. the handle count should match the 
>  number of handles that exist for the object. When creating the first 
>  handle the reference counter should be incremented by 1, when deleting 
>  the last handle to the object it should be decremented by 1. For 
>  additional handles it should remain unchanged. I implemented this 
>  behavior in a local branch that completely replaces the handle 
>  implementation using generic executive handle tables. But i still have 
>  to track down a few bugs.
>
>  
Of course, it is interesting to note that one of the reasons I have made 
an Ob rewrite is because of the way we incorrectly manage handles and 
references. Not only are our ObCreate/Insert routines badly implemented 
(which can create problems with drivers) but we also sometimes create 
handles out of thin air. There are other problems as well which 
influence the registry, like the fact that we don't send a parse context 
to the parse routine, and many more implementation issues. I have been 
waiting on Thomas to get the handle table working, because that will 
directly plug in with my Ob. Expect a branch in around 2 weeks (which 
won't compile or boot), and hopefully my design will have worked around 
this problem; it's entirely based on Windows'.

Best regards,
Alex Ionescu

[Casper Hornstrup]

I hope the parties involved don't take this personally, but I have to speak my mind
about something I think we're doing terribly
wrong and as a result is hurting the project.

Basicly we have the following situation (and please correct me if I'm wrong). Hartmut
spends some time examining the registry and
object manager problems. Alex has partially rewritten the object manager to fix the
problems, but can't get get further because he
needs to wait for Thomas to finish his handle table implementation. Meanwhile, Thomas
seems to be doing everything but finishing his
handle table implementation. Alex keeps an increasing number of bugfixes (not only
bugfixes for the object manager) on his private
miscelanea branch and thus keeping these bugfixes away from 25 developers and whatever
number of testers we have. The 25 developers
and testers will have to live with these bugs until the branch is merged to trunk which
appears to be several weeks away. Hartmut
seems to be willing to fix the object manager and registry manager bugs on trunk, but if
he now did, he would be duplicating work.

If this is what has happened, then we should make sure it doesn't happen again so we
don't waste our (very) limited resources. If
this is not what happened then I apologise for wasting bandwidth on this mail.

Casper

>   -----Original Message-----
>  From: ros-dev-bounces(a)reactos.com 
>  [mailto:[e-mail]] On Behalf Of Alex Ionescu
>  Sent: 8. marts 2005 23:58
>  To: ReactOS Development List
>  Subject: Re: [ros-dev] Registry and ObjectManager (was 
>  FreeLdr Part II)
>  
>  Of course, it is interesting to note that one of the reasons 
>  I have made an Ob rewrite is because of the way we 
>  incorrectly manage handles and references. Not only are our 
>  ObCreate/Insert routines badly implemented (which can create 
>  problems with drivers) but we also sometimes create handles 
>  out of thin air. There are other problems as well which 
>  influence the registry, like the fact that we don't send a 
>  parse context to the parse routine, and many more 
>  implementation issues. I have been waiting on Thomas to get 
>  the handle table working, because that will directly plug in 
>  with my Ob. Expect a branch in around 2 weeks (which won't 
>  compile or boot), and hopefully my design will have worked 
>  around this problem; it's entirely based on Windows'.
>  
>  Best regards,
>  Alex Ionescu
>  _______________________________________________
>  Ros-dev mailing list
>  Ros-dev(a)reactos.com
>  http://reactos.com:8080/mailman/listinfo/ros-dev

[James Tabor]

Hi!
Casper Hornstrup wrote:
>   I hope the parties involved don't take this
> personally, but I have to speak my mind
>  about something I think we're doing terribly wrong and as a result is hurting the
> project.
>  
>  Basicly we have the following situation (and please correct me if I'm wrong).
>  Hartmut spends some time examining the registry and object manager problems.
>  Alex has partially rewritten the object manager to fix the problems, but can't
>  get get further because he needs to wait for Thomas to finish his handle table
>  implementation. Meanwhile, Thomas seems to be doing everything but finishing his
>  handle table implementation. Alex keeps an increasing number of bugfixes (not only
>  bugfixes for the object manager) on his private miscelanea branch and thus keeping
>  these bugfixes away from 25 developers and whatever number of testers we have. The
>  25 developers and testers will have to live with these bugs until the branch is
>  merged to trunk which appears to be several weeks away. Hartmut seems to be willing
>  to fix the object manager and registry manager bugs on trunk, but if he now did, he
>  would be duplicating work.
>  
>  If this is what has happened, then we should make sure it doesn't happen again so we
>  don't waste our (very) limited resources. If this is not what happened then I
> apologise
>  for wasting bandwidth on this mail.
>  
>  Casper
>  
>   
I do not think this is a wast of time. *Finally* getting good debug information from
problems I once had (remember the little lockup reports I've posted). Yeah, no one
seemed
to understand why too. So, if this delays 0.2.6, I do not care. I just know when it does
make *tha cut* I will be very happy to have Reactos stable enough to soon become 0.3.0
alpha.


Oh, don't get me started on emulators, I found these little problems on real
hardware.

8^|

James

[Alex Ionescu]

Casper Hornstrup wrote:

>  I hope the parties involved don't take this
> personally, but I have to speak my mind about something I think we're doing terribly
> wrong and as a result is hurting the project.
>   
>  
And don't take my replies personally either...

>  Basicly we have the following situation (and please
> correct me if I'm wrong). 
>  
>  Hartmut spends some time examining the registry and
> object manager problems.
>  
I think Thomas deserves credit for this as well.

>   Alex has partially rewritten the object manager to fix
> the problems, but can't get get further because he
> needs to wait for Thomas to finish his handle table implementation. 
>  
Not quite true... it is blocking me, but so are at least hundreds of 
lines that I have to fix and review and re-edit in my rewrite, which is 
at LEAST more then a month away, just like the Cc rewrite has/will be in 
progress for a long time.

>  Meanwhile, Thomas seems to be doing everything but
> finishing his
> handle table implementation. 
>  
Thomas has been testing this for over a week and he almost gets the GUI 
to boot...it's unfair to accuse him of slacking off.

>  Alex keeps an increasing number of bugfixes (not only
> bugfixes for the object manager) on his private
> miscelanea branch and thus keeping these bugfixes away from 25 developers and whatever
> number of testers we have. 
>  
"Private"? there's no password on it, and anyone is free to test it,
and 
this is exactly what they are doing:
1) Thomas - Added many features, fixed some bugs and alerted me to some 
issues
2) Steven - Tested the branch and found out his laptop now works, helped 
me with some debug issues
3) Jim - Tested the branch extensively almost every day for HOURS.
4) Art - Did lots of testing with network applications and the stack, 
noted speedups and improvements.

These are only some of the people that have helped and tried out and 
shared code and hints. You keep telling people to use a branch, and now 
you accuse me of keeping it "private". The reason I've done it in a 
branch is because I was getting sick of people saying how I broke HEAD. 
And if this branch would be in head since the start, I would've broken 
it a large number of times. The existence of my branch has done nothing 
to hinder development on HEAD. The only person who needed its features 
was Thomas, and he has been working with it.

>  The 25 developers
> and testers will have to live with these bugs until the branch is merged to trunk which
> appears to be several weeks away.
>  
Just like we all have to live without Hartmut's fixes to Cc, Filip's DMA 
Patch, Eric's RPC stuff, Art's Ext2Fs support, and the numerous other 
local fixes the developers have made and keep for themselves until they 
are ready to test. Unlike them however, I have made my stuff PUBLIC so 
that others could help out (and they have). I am the last person you 
should blame for "keeping stuff private" and making devs live with bugs. 
My branch is not weeks away...it has been ready since Monday, and I've 
been waiting on Steven to prepare for 0.2.6, which he told me he was 
going to do Wednesday "OUT OF LACK OF TIME". I find it extremly 
disappointing that Steven says that 0.2.6 was delayed to today because 
of my branch... It was delayed a week, yes, but that was the result of 
people REQUESTING it after I asked on the ML.

>   Hartmut
> seems to be willing to fix the object manager and registry manager bugs on trunk, but if
> he now did, he would be duplicating work.
>   
>  
No he wouldn't, he would do everyone a good favor. My object manager 
works completely different from ros's, so it wouldnt' be code duplication.

>  If this is what has happened, then we should make sure
> it doesn't happen again so we don't waste our (very) limited resources. If
> this is not what happened then I apologise for wasting bandwidth on this mail.
>
> Casper
>   
>  
Once again, I hope nobody takes this personally, but I wanted to clear 
up the situation.

Best regards,
Alex Ionescu

[Casper Hornstrup]

>   -----Original Message-----
>  From: ros-dev-bounces(a)reactos.com 
>  [mailto:[e-mail]] On Behalf Of Alex Ionescu
>  Sent: 9. marts 2005 22:17
>  To: ReactOS Development List
>  Subject: Re: [ros-dev] Registry and ObjectManager (was 
>  FreeLdr Part II) 
<snip>Talk about all the work that went into the branch</snip>

I'll try to be more clear since you seem to be confused about my points.
I'm NOT arguing that the product is bad. I haven't looked at the branch
yet, but I'm sure a lot of great work went into it. ReactOS is still
a volunteer effort and everyone can (and should) be able to decide for
him/herself what they want to contribute. This is a great motivation
factor, but it's also a disadvantage so we should make sure that our
processes minimize the damages that can happen from that.

What I AM arguing for is that THE PROCESS could be better. Traditionally
there are two types of branches.

1) Release branch. Usually created some time before a release to have a
more stable product at the time of a release. Only bugfixes go to this
branch.

2) Feature branch. Usually created to achieve a well-defined purpose
(e.g. the implementation of a new feature). Only changes required to
achieve the well-defined purpose are committed to this branch.

Alex_devel_branch is a combination of the two and it has some disadvantages.

* Bugs that that could have been fixed on trunk are annoying to trunk
developers. The bugfixes that go only to the branch are not in trunk (until
merged there from the branch) where most developers work and is thus
annoying to these developers.

* Risk of duplicating work. We'll have more branches to track bugfixes on
so it's harder to know which bugs has been fixed and which hasn't.

Casper

[Alex Ionescu]

Casper Hornstrup wrote:

>   
>
>   
>
> >  -----Original Message-----
> > From: ros-dev-bounces(a)reactos.com 
> > [mailto:[e-mail]] On Behalf Of Alex Ionescu
> > Sent: 9. marts 2005 22:17
> > To: ReactOS Development List
> > Subject: Re: [ros-dev] Registry and ObjectManager (was 
> > FreeLdr Part II)
> >     
> >  
> <snip>Talk about all the work that went into the branch</snip>
>
> I'll try to be more clear since you seem to be confused about my points.
> I'm NOT arguing that the product is bad. I haven't looked at the branch
> yet, but I'm sure a lot of great work went into it. ReactOS is still
> a volunteer effort and everyone can (and should) be able to decide for
> him/herself what they want to contribute. This is a great motivation
> factor, but it's also a disadvantage so we should make sure that our
> processes minimize the damages that can happen from that.
>
> What I AM arguing for is that THE PROCESS could be better. Traditionally
> there are two types of branches.
>
> 1) Release branch. Usually created some time before a release to have a
> more stable product at the time of a release. Only bugfixes go to this
> branch.
>
> 2) Feature branch. Usually created to achieve a well-defined purpose
> (e.g. the implementation of a new feature). Only changes required to
> achieve the well-defined purpose are committed to this branch.
>
> Alex_devel_branch is a combination of the two and it has some disadvantages.
>   
>  
I agree, but I just want to clear up something.... this was never 
supposed to happen. I never planned for my branch to be ready for 0.2.6 
release. However, both Steven and James wanted 0.2.6 delayed until the 
merge, so that some stuff they considered blockers could be fixed. This 
is why it became an ugly combination, and I know it's been frustrating.

>  * Bugs that that could have been fixed on trunk are
> annoying to trunk
> developers. The bugfixes that go only to the branch are not in trunk (until
> merged there from the branch) where most developers work and is thus
> annoying to these developers.
>   
>  
This comment continues to seriously piss me off, but I will try to be as 
diplomatic as possible. The bugfixes that went in my branch are my own 
work, my own code, and wholy owned and copyrighted by me (just like any 
other code anyone writes). It was my decision to write that code, and it 
is my decision if I want to share it. There is nothing stopping me from 
deleting the whole branch tomorrow morning (except for Thomas's patch), 
if I feel like it. Of course, I will not do that, because I believe in 
the effort that everyone brings to this project, and I want to help out.

Those bugs have been in trunk *forever*. This, the developpers have been 
annoyed *forever*. If this were truly the case, then either:

1) All the developers would have nervous breakdowns by now, having been 
"annoyed" for the last 7 years.
2) They would've found and fixed the bugs, being higly "annoyed" by them.

This did not happen. Why? Because those bugs were 1) unknown to the 
developers, and also 2) not affecting them at this stage. Those who knew 
about it simply told themselves "it doesn't break anything for now, so 
someone will fix it later". The exception to this is Steven's bug.

So please don't try to tell me that developers are being "annoyed". 
Developers should be *thankful* that I took the time to review the whole 
Executive/Kernel (ex/ke) and fixed dozens of bugs and implemented new 
APIs, and that Thomas added more SEH to the functions. I have not once 
heard any developer speak of this "annoyance", so maybe it would be wise 
not to use "the developers" in your sentence.

I am really not trying to be an asshole here, but I consider the work 
that ANY developer does a gift. I consider your work on rbuild a gift to 
ReactOS and to me. I am not annoyed by the fact rbuild is not in trunk 
now, but that you've been using it for months. Instead, I thank you for 
your effort and dedication, and I anxiously await the result. I would 
give and expect the same attitude towards and from any other developer. 
You are not annoying me for having worked on your branch for months now, 
why am I annoying you for having worked on mine for two weeks? One could 
argue that the deficencies of "make" are much more annoying then some 
kernel bugs which won't come into play until later when we get serious 
about stability and drivers. I appreciate the fact you've decided to 
attack them, no matter how long it takes (btw, this also goes to Royce 
and everyone else working on it, as well as to Hartmut for Cc).

>  * Risk of duplicating work. We'll have more
> branches to track bugfixes on
> so it's harder to know which bugs has been fixed and which hasn't.
>   
>  
I have tracked and merged all bug fixes, because I'm doing a good job at 
it. Instead, you should be happy that the bugs which plagued my branch 
did not affect trunk until they were fixed, which saved a lot more grief.

>  Casper
>   
>  
Best regards,
Alex Ionescu

[Steven Edwards]

--- Alex Ionescu <ionucu(a)videotron.ca> wrote:
>   I agree, but I just want to clear up something....
> this was never 
>  supposed to happen. I never planned for my branch to be ready for 0.2.6 
>  release. However, both Steven and James wanted 0.2.6 delayed until the 
>  merge, so that some stuff they considered blockers could be fixed. This 
>  is why it became an ugly combination, and I know it's been frustrating. 
I wanted 0.2.6 to be delayed until those regressions were fixed. Once they were I wanted
you to
merge your branch to the trunk. I don't care what you do in your own branch but I
don't want to
see this project do releases that contain regressions so of course I am going to vote for
a delay
if I know someone has a fix. But once that fix is found I hope that it makes it to the
trunk as
quickly as possible.

Thanks
Steven



		
__________________________________ 
Do you Yahoo!? 
Yahoo! Small Business - Try our new resources site!
http://smallbusiness.yahoo.com/resources/

[Casper Hornstrup]

> >  * Bugs that
> > that could have been fixed on trunk are annoying  
>  to trunk 
> >  developers. The bugfixes that go only to the
> > branch are not in trunk 
> > (until merged there from the branch) where most developers  
>  work and is 
> >  thus annoying to these developers.
> >   
> >  
>  This comment continues to seriously piss me off, but I will 
>  try to be as diplomatic as possible. The bugfixes that went 
>  in my branch are my own work, my own code, and wholy owned 
>  and copyrighted by me (just like any other code anyone 
>  writes). It was my decision to write that code, and it is my 
>  decision if I want to share it. There is nothing stopping me 
>  from deleting the whole branch tomorrow morning (except for 
>  Thomas's patch), if I feel like it. Of course, I will not do 
>  that, because I believe in the effort that everyone brings to 
>  this project, and I want to help out. 
What I'm saying is, that if we allow developers to keep bugfixes
on miscellanea branches then we don't have an as effective
organisation as we could have. I will compare it to using empty
commit log messages. It's good for the committer since he's not
going to have to spend time describing his changes. He already
knows his change as he wrote them. It's bad for all other project
members since they don't know about what was changed.

If the general consensus is that it is alright to have this
kind of branches, then I will respect that decision.

Casper

[Hartmut Birr]

Thomas Weidenmueller schrieb:

>  
> >   They look for
> >  an existing sub key object. If one is found, the parse routine reference
> >  this object and return it. But this is not correct. The parse routine
> >  should only use and reference this existing object, if the reference
> >  count is greater than zero.
> >  
>  This doesn't sound right, the reference count should never be zero
>  because then the object should be deleted, which means accessing it
>  would be dangerous. The object in any case needs to be removed from
>  the list before it gets deleted.
>  
This doesn't sound right, but the current implemention of key objects
uses this way.

> >   My idea is to
> > split the data for the key object and the key object
> >  itself. The parsing routine creates the data only once, but it creates
> >  for each request a seperate key object.
> >  
>  I don't really understand the advantage of this. For file systems it's
>  obvious because these objects represent data streams with different
>  offsets, ... But i don't really understand how it could be useful for
>  registry key objects.
>  
I see two ways to solve the wrong referencing problem. The first one is,
we create the key object and reference it again. One reference is for
holding the key object in the parent key structure valid. The second
reference is for the caller (ObFindObject..) and he could do what he
want with it. The problem is, the first reference is never removed and
it is not possible to dereference (delete) the parent. The other way is
a special data structur between the parent key and the child keys. This
structure has an internal reference count, which is incremented when a
child key is created and decremented when a key is deleted. The create
or delete operation occurs only if the hive is looked. The creating or
deleting of this data structur has nothing to do with the real reference
count of the object. It is always triggered by the create (parse) or
delete operation.

- Hartmut

[Thomas Weidenmueller]

Hartmut Birr wrote:

>  I see two ways to solve the wrong referencing problem.
> The first one is,
> we create the key object and reference it again. One reference is for
> holding the key object in the parent key structure valid. The second
> reference is for the caller (ObFindObject..) and he could do what he
> want with it.
>  
it should be this way

>  The problem is, the first reference is never removed
> and
> it is not possible to dereference (delete) the parent.
>  
imo a key should always hold a reference to it's parent and dereference 
it when deleted (in the deletion callback), that would automatically 
delete all no longer used keys since they'd always dereference their 
parent, which might dereference even more keys nearer to the root key. 
when a key get's deleted it should be sufficient to mark it as 
deleted+unlink it from the parent key (while holding the registry lock) 
and remove it's keep-alive reference. as soon as sub-keys get freed, 
they'd just dereference their parents and if they get to dereference the 
parent key that was deleted it'd be directly freed.

Best Regards

[Hartmut Birr]

Thomas Weidenmueller schrieb:

> >   The problem
> > is, the first reference is never removed and
> >  it is not possible to dereference (delete) the parent.
> >  
>  imo a key should always hold a reference to it's parent and
>  dereference it when deleted (in the deletion callback), that would
>  automatically delete all no longer used keys since they'd always
>  dereference their parent, which might dereference even more keys
>  nearer to the root key.  
In this case a key object is never removed, because nobody can remove
the keep-alive reference. We can use a worker thread which scans
periodically the registry and dereference all key objects which have
only the keep-alive reference.

- Hartmut

[Gunnar Dalsnes]

Windows use a read/write lock (ERESOURCE) per OBJECT_TYPE. BTW: windows 
has an internal function BOOL ObReferenceObjectSafe, which tells if the 
referencing succeded or not, while the ObReferenceObject is a VOID. Its 
probably unrelated thou.

Hartmut Birr wrote:
>   Hi,
>  
>  I can fix my problem by protecting the access to some entries of the
>  object header with a global spinnlock, but I doesn't like this. Has
>  anyone a better idea?
>  
>  - Hartmut
>  
>  
>

[Alex Ionescu]

Gunnar Dalsnes wrote:

>   Windows use a read/write lock (ERESOURCE) per
> OBJECT_TYPE. BTW: 
>  windows has an internal function BOOL ObReferenceObjectSafe, which 
>  tells if the referencing succeded or not, while the ObReferenceObject 
>  is a VOID. Its probably unrelated thou.
>  
Plus it reparses completely different from us and also uses locks for 
directory entries.

Best regards,
Alex Ionescu