RAMFS: Lock attribute iterators separate from entry iterators.

And clean up the code style while at it.

src/add-ons/kernel/file_systems/ramfs/AttributeIterator.cpp

@@ -2,11 +2,15 @@
  * Copyright 2007, Ingo Weinhold, ingo_weinhold@gmx.de.
  * All rights reserved. Distributed under the terms of the MIT license.
  */
+
 #include "AttributeIterator.h"
+
+#include <util/AutoLock.h>
+
 #include "Node.h"
 #include "Volume.h"
 
-// constructor
+
 AttributeIterator::AttributeIterator(Node *node)
 	: fNode(node),
 	  fAttribute(NULL),
@@ -16,34 +20,36 @@ AttributeIterator::AttributeIterator(Node *node)
 {
 }
 
-// destructor
+
 AttributeIterator::~AttributeIterator()
 {
 	Unset();
 }
 
-// SetTo
+
 status_t
 AttributeIterator::SetTo(Node *node)
 {
 	Unset();
-	status_t error = (node ? B_OK : B_BAD_VALUE);
-	if (error == B_OK) {
-		fNode = node;
-		fAttribute = NULL;
-		fSuspended = false;
-		fIsNext = false;
-		fDone = false;
-	}
-	return error;
+
+	if (node == NULL)
+		return B_BAD_VALUE;
+
+	fNode = node;
+	fAttribute = NULL;
+	fSuspended = false;
+	fIsNext = false;
+	fDone = false;
+	return B_OK;
 }
 
-// Unset
+
 void
 AttributeIterator::Unset()
 {
 	if (fNode && fSuspended)
 		Resume();
+
 	fNode = NULL;
 	fAttribute = NULL;
 	fSuspended = false;
@@ -51,51 +57,57 @@ AttributeIterator::Unset()
 	fDone = false;
 }
 
-// Suspend
+
 status_t
 AttributeIterator::Suspend()
 {
-	status_t error = (fNode ? B_OK : B_ERROR);
-	if (error == B_OK) {
-		if (fNode->GetVolume()->IteratorLock()) {
-			if (!fSuspended) {
-				if (fAttribute)
-					fAttribute->AttachAttributeIterator(this);
-				fNode->GetVolume()->IteratorUnlock();
-				fSuspended = true;
-			} else
-				error = B_ERROR;
-		} else
-			error = B_ERROR;
-	}
-	return error;
+	if (fNode == NULL)
+		return B_ERROR;
+
+	RecursiveLocker locker(fNode->GetVolume()->AttributeIteratorLocker());
+	if (!locker.IsLocked())
+		return B_ERROR;
+
+	if (fSuspended)
+		return B_ERROR;
+
+	if (fAttribute)
+		fAttribute->AttachAttributeIterator(this);
+	fSuspended = true;
+
+	return B_OK;
 }
 
-// Resume
+
 status_t
 AttributeIterator::Resume()
 {
-	status_t error = (fNode ? B_OK : B_ERROR);
-	if (error == B_OK) {
-		if (fNode->GetVolume()->IteratorLock()) {
-			if (fSuspended) {
-				if (fAttribute)
-					fAttribute->DetachAttributeIterator(this);
-				fSuspended = false;
-			}
-			fNode->GetVolume()->IteratorUnlock();
-		} else
-			error = B_ERROR;
-	}
-	return error;
+	if (fNode == NULL)
+		return B_ERROR;
+
+	RecursiveLocker locker(fNode->GetVolume()->AttributeIteratorLocker());
+	if (!locker.IsLocked())
+		return B_ERROR;
+
+	if (!fSuspended)
+		return B_ERROR;
+
+	if (fAttribute)
+		fAttribute->DetachAttributeIterator(this);
+	fSuspended = false;
+
+	return B_OK;
 }
 
-// GetNext
+
 status_t
 AttributeIterator::GetNext(Attribute **attribute)
 {
+	if (attribute == NULL)
+		return B_BAD_VALUE;
+
 	status_t error = B_ENTRY_NOT_FOUND;
-	if (!fDone && fNode && attribute) {
+	if (!fDone && fNode != NULL) {
 		if (fIsNext) {
 			fIsNext = false;
 			if (fAttribute)
@@ -108,26 +120,26 @@ AttributeIterator::GetNext(Attribute **attribute)
 	return error;
 }
 
-// Rewind
+
 status_t
 AttributeIterator::Rewind()
 {
-	status_t error = (fNode ? B_OK : B_ERROR);
-	if (error == B_OK) {
-		if (fNode->GetVolume()->IteratorLock()) {
-			if (fSuspended && fAttribute)
-				fAttribute->DetachAttributeIterator(this);
-			fAttribute = NULL;
-			fIsNext = false;
-			fDone = false;
-			fNode->GetVolume()->IteratorUnlock();
-		} else
-			error = B_ERROR;
-	}
-	return error;
+	if (fNode == NULL)
+		return B_ERROR;
+
+	RecursiveLocker locker(fNode->GetVolume()->AttributeIteratorLocker());
+	if (!locker.IsLocked())
+		return B_ERROR;
+
+	if (fSuspended && fAttribute)
+		fAttribute->DetachAttributeIterator(this);
+	fAttribute = NULL;
+	fIsNext = false;
+	fDone = false;
+	return B_OK;
 }
 
-// SetCurrent
+
 void
 AttributeIterator::SetCurrent(Attribute *attribute, bool isNext)
 {
@@ -135,4 +147,3 @@ AttributeIterator::SetCurrent(Attribute *attribute, bool isNext)
 	fAttribute = attribute;
 	fDone = !fAttribute;
 }
-

src/add-ons/kernel/file_systems/ramfs/Node.cpp

@@ -3,11 +3,14 @@
  * All rights reserved. Distributed under the terms of the MIT license.
  */
 
+#include "Node.h"
+
+#include <util/AutoLock.h>
+
 #include "AllocationInfo.h"
 #include "DebugSupport.h"
 #include "EntryIterator.h"
 #include "LastModifiedIndex.h"
-#include "Node.h"
 #include "Volume.h"
 
 
@@ -191,40 +194,39 @@ Node::AddAttribute(Attribute *attribute)
 status_t
 Node::RemoveAttribute(Attribute *attribute)
 {
-	status_t error = (attribute && attribute->GetNode() == this
-					  ? B_OK : B_BAD_VALUE);
+	if (attribute == NULL || attribute->GetNode() != this)
+		return B_BAD_VALUE;
+
+	RecursiveLocker locker(GetVolume()->AttributeIteratorLocker());
+	if (!locker.IsLocked())
+		return B_ERROR;
+
+	// move all iterators pointing to the attribute to the next attribute
+	Attribute *nextAttr = fAttributes.GetNext(attribute);
+	DoublyLinkedList<AttributeIterator> *iterators
+		= attribute->GetAttributeIteratorList();
+	for (AttributeIterator *iterator = iterators->First();
+			iterator != NULL; iterator = iterators->GetNext(iterator)) {
+		iterator->SetCurrent(nextAttr, true);
+	}
+
+	// Move the iterators from one list to the other, or just remove
+	// them, if there is no next attribute.
+	if (nextAttr != NULL) {
+		DoublyLinkedList<AttributeIterator> *nextIterators
+			= nextAttr->GetAttributeIteratorList();
+		nextIterators->MoveFrom(iterators);
+	} else
+		iterators->RemoveAll();
+
+	locker.Unlock();
+
+	// remove the attribute
+	status_t error = GetVolume()->NodeAttributeRemoved(GetID(), attribute);
 	if (error == B_OK) {
-		// move all iterators pointing to the attribute to the next attribute
-		if (GetVolume()->IteratorLock()) {
-			// set the iterators' current entry
-			Attribute *nextAttr = fAttributes.GetNext(attribute);
-			DoublyLinkedList<AttributeIterator> *iterators
-				= attribute->GetAttributeIteratorList();
-			for (AttributeIterator *iterator = iterators->First();
-				 iterator;
-				 iterator = iterators->GetNext(iterator)) {
-				iterator->SetCurrent(nextAttr, true);
-			}
-			// Move the iterators from one list to the other, or just remove
-			// them, if there is no next attribute.
-			if (nextAttr) {
-				DoublyLinkedList<AttributeIterator> *nextIterators
-					= nextAttr->GetAttributeIteratorList();
-				nextIterators->MoveFrom(iterators);
-			} else
-				iterators->RemoveAll();
-			GetVolume()->IteratorUnlock();
-		} else
-			error = B_ERROR;
-		// remove the attribute
-		if (error == B_OK) {
-			error = GetVolume()->NodeAttributeRemoved(GetID(), attribute);
-			if (error == B_OK) {
-				fAttributes.Remove(attribute);
-				attribute->SetNode(NULL);
-				MarkModified(B_STAT_MODIFICATION_TIME);
-			}
-		}
+		fAttributes.Remove(attribute);
+		attribute->SetNode(NULL);
+		MarkModified(B_STAT_MODIFICATION_TIME);
 	}
 	return error;
 }

src/add-ons/kernel/file_systems/ramfs/Volume.cpp

@@ -141,6 +141,7 @@ Volume::Volume(fs_volume* volume)
 {
 	rw_lock_init(&fLocker, "ramfs volume");
 	recursive_lock_init(&fIteratorLocker, "ramfs iterators");
+	recursive_lock_init(&fAttributeIteratorLocker, "ramfs attribute iterators");
 	recursive_lock_init(&fQueryLocker, "ramfs queries");
 }
 
@@ -149,6 +150,7 @@ Volume::~Volume()
 {
 	Unmount();
 
+	recursive_lock_destroy(&fAttributeIteratorLocker);
 	recursive_lock_destroy(&fIteratorLocker);
 	recursive_lock_destroy(&fQueryLocker);
 	rw_lock_destroy(&fLocker);
@@ -777,4 +779,3 @@ Volume::IteratorUnlock()
 {
 	recursive_lock_unlock(&fIteratorLocker);
 }
-

src/add-ons/kernel/file_systems/ramfs/Volume.h

@@ -167,6 +167,8 @@ public:
 	bool IteratorLock();
 	void IteratorUnlock();
 
+	recursive_lock&	AttributeIteratorLocker() { return fAttributeIteratorLocker; }
+
 protected:
 	fs_volume*				fVolume;
 
@@ -186,6 +188,7 @@ private:
 	EntryListenerList		fAnyEntryListeners;
 
 	recursive_lock			fIteratorLocker;
+	recursive_lock			fAttributeIteratorLocker;
 
 	recursive_lock			fQueryLocker;
 	QueryList				fQueries;