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.
BugCheck Alert!
ion@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
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
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;
}
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@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); + } }
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").
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
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@reactos.com http://reactos.com:8080/mailman/listinfo/ros-dev
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
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
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@reactos.com [mailto:ros-dev-bounces@reactos.com] 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@reactos.com http://reactos.com:8080/mailman/listinfo/ros-dev
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
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
-----Original Message----- From: ros-dev-bounces@reactos.com [mailto:ros-dev-bounces@reactos.com] 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
Casper Hornstrup wrote:
-----Original Message----- From: ros-dev-bounces@reactos.com [mailto:ros-dev-bounces@reactos.com] 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.
- 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.
- 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
--- Alex Ionescu ionucu@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/
- 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
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
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
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
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
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
-
Alex Ionescu -
Casper Hornstrup -
Gunnar Dalsnes -
Hartmut Birr -
James Tabor -
Mike Nordell -
Steven Edwards -
Thomas Weidenmueller