Fix node handling on package addition/removal

* UnpackingLeafNode: Add a fFinalPackageNode attribute. It is set when
  the node is about to be removed and will point to the node's previous
  head package node and be used in its stead. From the perspective of
  the FS hooks this leaves the node in an unchanged state.
* Unpacking[Leaf,Directory]Node:
  - Add WillBeFirstPackageNode(), returning whether the given package
    node would become the head package node when added.
  - Add PrepareForRemoval() which removes all package nodes. In case of
    UnpackingLeafNode it also sets fFinalPackageNode.
  - Add CloneTransferPackageNodes(). It is only implemented for
    UnpackingLeafNode. It clones the node, transfers all package nodes
    to the clone and sets fFinalPackageNode on this node.
* Volume::_{Add,Remove}PackageNode(): Solved the following TODO: When a
  package is added or removed and a file present in both the
  added/removed package and another package with the version in the
  former having precedence, we have to remove the node (leaving it
  unchanged) and replace it by a new node. This prevents clients having
  the node opened or mapped from suddenly seeing different data. It also
  fixes unbalanced calls to PackageNode::VFSInit()/VFSUninit() which
  would result in file descriptors to package files being leaked.

src/add-ons/kernel/file_systems/packagefs/UnpackingDirectory.cpp

@@ -140,6 +140,27 @@ UnpackingDirectory::IsOnlyPackageNode(PackageNode* node) const
 }
 
 
+bool
+UnpackingDirectory::WillBeFirstPackageNode(PackageNode* packageNode) const
+{
+	PackageDirectory* packageDirectory
+		= dynamic_cast<PackageDirectory*>(packageNode);
+	if (packageDirectory == NULL)
+		return false;
+
+	PackageDirectory* other = fPackageDirectories.Head();
+	return other == NULL
+		|| packageDirectory->ModifiedTime() > other->ModifiedTime();
+}
+
+
+void
+UnpackingDirectory::PrepareForRemoval()
+{
+	fPackageDirectories.MakeEmpty();
+}
+
+
 status_t
 UnpackingDirectory::OpenAttributeDirectory(AttributeDirectoryCookie*& _cookie)
 {

src/add-ons/kernel/file_systems/packagefs/UnpackingDirectory.h

@@ -29,6 +29,10 @@ public:
 
 	virtual	PackageNode*		GetPackageNode();
 	virtual	bool				IsOnlyPackageNode(PackageNode* node) const;
+	virtual	bool				WillBeFirstPackageNode(
+									PackageNode* packageNode) const;
+
+	virtual	void				PrepareForRemoval();
 
 	virtual	status_t			OpenAttributeDirectory(
 									AttributeDirectoryCookie*& _cookie);

src/add-ons/kernel/file_systems/packagefs/UnpackingLeafNode.cpp

@@ -9,6 +9,7 @@
 #include <string.h>
 
 #include <algorithm>
+#include <new>
 
 #include "UnpackingAttributeCookie.h"
 #include "UnpackingAttributeDirectoryCookie.h"
@@ -17,20 +18,23 @@
 
 UnpackingLeafNode::UnpackingLeafNode(ino_t id)
 	:
-	Node(id)
+	Node(id),
+	fFinalPackageNode(NULL)
 {
 }
 
 
 UnpackingLeafNode::~UnpackingLeafNode()
 {
+	if (fFinalPackageNode != NULL)
+		fFinalPackageNode->ReleaseReference();
 }
 
 
 status_t
 UnpackingLeafNode::VFSInit(dev_t deviceID)
 {
-	if (PackageLeafNode* packageNode = fPackageNodes.Head())
+	if (PackageLeafNode* packageNode = _ActivePackageNode())
 		return packageNode->VFSInit(deviceID, fID);
 	return B_OK;
 }
@@ -39,7 +43,7 @@ UnpackingLeafNode::VFSInit(dev_t deviceID)
 void
 UnpackingLeafNode::VFSUninit()
 {
-	if (PackageLeafNode* packageNode = fPackageNodes.Head())
+	if (PackageLeafNode* packageNode = _ActivePackageNode())
 		packageNode->VFSUninit();
 }
 
@@ -47,7 +51,7 @@ UnpackingLeafNode::VFSUninit()
 mode_t
 UnpackingLeafNode::Mode() const
 {
-	if (PackageLeafNode* packageNode = fPackageNodes.Head())
+	if (PackageLeafNode* packageNode = _ActivePackageNode())
 		return packageNode->Mode();
 	return S_IFREG | S_IRUSR | S_IRGRP | S_IROTH;
 }
@@ -56,7 +60,7 @@ UnpackingLeafNode::Mode() const
 uid_t
 UnpackingLeafNode::UserID() const
 {
-	if (PackageLeafNode* packageNode = fPackageNodes.Head())
+	if (PackageLeafNode* packageNode = _ActivePackageNode())
 		return packageNode->UserID();
 	return 0;
 }
@@ -65,7 +69,7 @@ UnpackingLeafNode::UserID() const
 gid_t
 UnpackingLeafNode::GroupID() const
 {
-	if (PackageLeafNode* packageNode = fPackageNodes.Head())
+	if (PackageLeafNode* packageNode = _ActivePackageNode())
 		return packageNode->GroupID();
 	return 0;
 }
@@ -74,7 +78,7 @@ UnpackingLeafNode::GroupID() const
 timespec
 UnpackingLeafNode::ModifiedTime() const
 {
-	if (PackageLeafNode* packageNode = fPackageNodes.Head())
+	if (PackageLeafNode* packageNode = _ActivePackageNode())
 		return packageNode->ModifiedTime();
 
 	timespec time = { 0, 0 };
@@ -85,7 +89,7 @@ UnpackingLeafNode::ModifiedTime() const
 off_t
 UnpackingLeafNode::FileSize() const
 {
-	if (PackageLeafNode* packageNode = fPackageNodes.Head())
+	if (PackageLeafNode* packageNode = _ActivePackageNode())
 		return packageNode->FileSize();
 	return 0;
 }
@@ -101,6 +105,8 @@ UnpackingLeafNode::GetNode()
 status_t
 UnpackingLeafNode::AddPackageNode(PackageNode* packageNode)
 {
+	ASSERT(fFinalPackageNode == NULL);
+
 	if (S_ISDIR(packageNode->Mode()))
 		return B_BAD_VALUE;
 
@@ -127,6 +133,8 @@ UnpackingLeafNode::AddPackageNode(PackageNode* packageNode)
 void
 UnpackingLeafNode::RemovePackageNode(PackageNode* packageNode)
 {
+	ASSERT(fFinalPackageNode == NULL);
+
 	bool isNewest = packageNode == fPackageNodes.Head();
 	fPackageNodes.Remove(dynamic_cast<PackageLeafNode*>(packageNode));
 
@@ -152,22 +160,78 @@ UnpackingLeafNode::RemovePackageNode(PackageNode* packageNode)
 PackageNode*
 UnpackingLeafNode::GetPackageNode()
 {
-	return fPackageNodes.Head();
+	return _ActivePackageNode();
 }
 
 
 bool
 UnpackingLeafNode::IsOnlyPackageNode(PackageNode* node) const
 {
+	ASSERT(fFinalPackageNode == NULL);
+
 	PackageLeafNode* head = fPackageNodes.Head();
 	return node == head && fPackageNodes.GetNext(head) == NULL;
 }
 
 
+bool
+UnpackingLeafNode::WillBeFirstPackageNode(PackageNode* packageNode) const
+{
+	PackageLeafNode* packageLeafNode
+		= dynamic_cast<PackageLeafNode*>(packageNode);
+	if (packageLeafNode == NULL)
+		return false;
+
+	PackageLeafNode* headNode = fPackageNodes.Head();
+	return headNode == NULL
+		|| packageLeafNode->ModifiedTime() > headNode->ModifiedTime();
+}
+
+void
+UnpackingLeafNode::PrepareForRemoval()
+{
+	ASSERT(fFinalPackageNode == NULL);
+
+	fFinalPackageNode = fPackageNodes.Head();
+	if (fFinalPackageNode != NULL) {
+		fFinalPackageNode->AcquireReference();
+		fPackageNodes.MakeEmpty();
+	}
+}
+
+
+status_t
+UnpackingLeafNode::CloneTransferPackageNodes(ino_t id, UnpackingNode*& _newNode)
+{
+	ASSERT(fFinalPackageNode == NULL);
+
+	UnpackingLeafNode* clone = new(std::nothrow) UnpackingLeafNode(id);
+	if (clone == NULL)
+		return B_NO_MEMORY;
+
+	status_t error = clone->Init(Parent(), Name(), 0);
+	if (error != B_OK) {
+		delete clone;
+		return error;
+	}
+
+	// We keep the old head as fFinalPackageNode, which will make us to behave
+	// exactly as before with respect to FS operations.
+	fFinalPackageNode = fPackageNodes.Head();
+	if (fFinalPackageNode != NULL) {
+		fFinalPackageNode->AcquireReference();
+		clone->fPackageNodes.MoveFrom(&fPackageNodes);
+	}
+
+	_newNode = clone;
+	return B_OK;
+}
+
+
 status_t
 UnpackingLeafNode::Read(off_t offset, void* buffer, size_t* bufferSize)
 {
-	if (PackageLeafNode* packageNode = fPackageNodes.Head())
+	if (PackageLeafNode* packageNode = _ActivePackageNode())
 		return packageNode->Read(offset, buffer, bufferSize);
 	return B_ERROR;
 }
@@ -176,7 +240,7 @@ UnpackingLeafNode::Read(off_t offset, void* buffer, size_t* bufferSize)
 status_t
 UnpackingLeafNode::Read(io_request* request)
 {
-	if (PackageLeafNode* packageNode = fPackageNodes.Head())
+	if (PackageLeafNode* packageNode = _ActivePackageNode())
 		return packageNode->Read(request);
 	return EBADF;
 }
@@ -185,7 +249,7 @@ UnpackingLeafNode::Read(io_request* request)
 status_t
 UnpackingLeafNode::ReadSymlink(void* buffer, size_t* bufferSize)
 {
-	PackageLeafNode* packageNode = fPackageNodes.Head();
+	PackageLeafNode* packageNode = _ActivePackageNode();
 	if (packageNode == NULL)
 		return B_BAD_VALUE;
 
@@ -206,7 +270,7 @@ UnpackingLeafNode::ReadSymlink(void* buffer, size_t* bufferSize)
 status_t
 UnpackingLeafNode::OpenAttributeDirectory(AttributeDirectoryCookie*& _cookie)
 {
-	return UnpackingAttributeDirectoryCookie::Open(fPackageNodes.Head(),
+	return UnpackingAttributeDirectoryCookie::Open(_ActivePackageNode(),
 		_cookie);
 }
 
@@ -215,7 +279,7 @@ status_t
 UnpackingLeafNode::OpenAttribute(const char* name, int openMode,
 	AttributeCookie*& _cookie)
 {
-	return UnpackingAttributeCookie::Open(fPackageNodes.Head(), name, openMode,
+	return UnpackingAttributeCookie::Open(_ActivePackageNode(), name, openMode,
 		_cookie);
 }
 
@@ -223,7 +287,7 @@ UnpackingLeafNode::OpenAttribute(const char* name, int openMode,
 status_t
 UnpackingLeafNode::IndexAttribute(AttributeIndexer* indexer)
 {
-	return UnpackingAttributeCookie::IndexAttribute(fPackageNodes.Head(),
+	return UnpackingAttributeCookie::IndexAttribute(_ActivePackageNode(),
 		indexer);
 }
 
@@ -231,7 +295,16 @@ UnpackingLeafNode::IndexAttribute(AttributeIndexer* indexer)
 void*
 UnpackingLeafNode::IndexCookieForAttribute(const char* name) const
 {
-	if (PackageLeafNode* packageNode = fPackageNodes.Head())
+	if (PackageLeafNode* packageNode = _ActivePackageNode())
 		return packageNode->IndexCookieForAttribute(name);
 	return NULL;
 }
+
+
+PackageLeafNode*
+UnpackingLeafNode::_ActivePackageNode() const
+{
+	if (PackageLeafNode* packageNode = fPackageNodes.Head())
+		return packageNode;
+	return fFinalPackageNode;
+}

src/add-ons/kernel/file_systems/packagefs/UnpackingLeafNode.h

@@ -32,6 +32,12 @@ public:
 
 	virtual	PackageNode*		GetPackageNode();
 	virtual	bool				IsOnlyPackageNode(PackageNode* node) const;
+	virtual	bool				WillBeFirstPackageNode(
+									PackageNode* packageNode) const;
+
+	virtual	void				PrepareForRemoval();
+	virtual	status_t			CloneTransferPackageNodes(ino_t id,
+									UnpackingNode*& _newNode);
 
 	virtual	status_t			Read(off_t offset, void* buffer,
 									size_t* bufferSize);
@@ -48,8 +54,12 @@ public:
 	virtual	status_t			IndexAttribute(AttributeIndexer* indexer);
 	virtual	void*				IndexCookieForAttribute(const char* name) const;
 
+private:
+	inline	PackageLeafNode*	_ActivePackageNode() const;
+
 private:
 			PackageLeafNodeList	fPackageNodes;
+			PackageLeafNode*	fFinalPackageNode;
 };
 
 

src/add-ons/kernel/file_systems/packagefs/UnpackingNode.cpp

@@ -10,3 +10,10 @@
 UnpackingNode::~UnpackingNode()
 {
 }
+
+
+status_t
+UnpackingNode::CloneTransferPackageNodes(ino_t id, UnpackingNode*& _newNode)
+{
+	return B_BAD_VALUE;
+}

src/add-ons/kernel/file_systems/packagefs/UnpackingNode.h

@@ -24,6 +24,12 @@ public:
 
 	virtual	PackageNode*		GetPackageNode() = 0;
 	virtual	bool				IsOnlyPackageNode(PackageNode* node) const = 0;
+	virtual	bool				WillBeFirstPackageNode(
+									PackageNode* packageNode) const = 0;
+
+	virtual	void				PrepareForRemoval() = 0;
+	virtual	status_t			CloneTransferPackageNodes(ino_t id,
+									UnpackingNode*& _newNode);
 };
 
 

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

@@ -1243,21 +1243,59 @@ Volume::_AddPackageNode(Directory* directory, PackageNode* packageNode,
 		newNode = true;
 	}
 
-	// TODO: The non-new part is broken for files. If a node is already known to
-	// the VFS, we can't just change the file content. The file might be an
-	// executable or library that is currently in use (i.e. mapped) and when
-	// just changing the file content we break things horribly. In fact we don't
-	// even do that correctly in UnpackingLeafNode::AddPackageNode() -- neither
-	// the former nor the new head package node is notified.
-
 	BReference<Node> nodeReference(node);
 	NodeWriteLocker nodeWriteLocker(node);
 
+	BReference<Node> newNodeReference;
+	NodeWriteLocker newNodeWriteLocker;
+	Node* oldNode = NULL;
+
+	if (!newNode && !S_ISDIR(node->Mode()) && oldPackageNode != NULL
+		&& unpackingNode->WillBeFirstPackageNode(packageNode)) {
+		// The package node we're going to add will represent the node,
+		// replacing the current head package node. Since the node isn't a
+		// directory, we must make sure that clients having opened or mapped the
+		// node won't be surprised. So we create a new node and remove the
+		// current one.
+		// create a new node and transfer the package nodes to it
+		UnpackingNode* newUnpackingNode;
+		status_t error = unpackingNode->CloneTransferPackageNodes(
+			fNextNodeID++, newUnpackingNode);
+		if (error != B_OK)
+			RETURN_ERROR(error);
+
+		// remove the old node
+		_NotifyNodeRemoved(node);
+		_RemoveNodeAndVNode(node);
+		oldNode = node;
+
+		// add the new node
+		unpackingNode = newUnpackingNode;
+		node = unpackingNode->GetNode();
+		newNodeReference.SetTo(node);
+		newNodeWriteLocker.SetTo(node, false);
+
+		directory->AddChild(node);
+		fNodes.Insert(node);
+		newNode = true;
+	}
+
 	status_t error = unpackingNode->AddPackageNode(packageNode);
 	if (error != B_OK) {
-		// remove the node, if created before
-		if (newNode)
-			_RemoveNode(node);
+		// Remove the node, if created before. If the node was created to
+		// replace the previous node, send out notifications instead.
+		if (newNode) {
+			if (oldNode != NULL) {
+				_NotifyNodeAdded(node);
+				if (notify) {
+					notify_entry_removed(ID(), directory->ID(), oldNode->Name(),
+						oldNode->ID());
+					notify_entry_created(ID(), directory->ID(), node->Name(),
+						node->ID());
+				}
+			} else
+				_RemoveNode(node);
+		}
 		RETURN_ERROR(error);
 	}
 
@@ -1270,21 +1308,18 @@ Volume::_AddPackageNode(Directory* directory, PackageNode* packageNode,
 
 	if (notify) {
 		if (newNode) {
+			if (oldNode != NULL) {
+				notify_entry_removed(ID(), directory->ID(), oldNode->Name(),
+					oldNode->ID());
+			}
 			notify_entry_created(ID(), directory->ID(), node->Name(),
 				node->ID());
 		} else if (packageNode == unpackingNode->GetPackageNode()) {
 			// The new package node has become the one representing the node.
 			// Send stat changed notification for directories and entry
 			// removed + created notifications for files and symlinks.
-			if (S_ISDIR(packageNode->Mode())) {
-				notify_stat_changed(ID(), node->ID(), kAllStatFields);
-				// TODO: Actually the attributes might change, too!
-			} else {
-				notify_entry_removed(ID(), directory->ID(), node->Name(),
-					node->ID());
-				notify_entry_created(ID(), directory->ID(), node->Name(),
-					node->ID());
-			}
+			notify_stat_changed(ID(), node->ID(), kAllStatFields);
+			// TODO: Actually the attributes might change, too!
 		}
 	}
 
@@ -1304,11 +1339,12 @@ Volume::_RemovePackageNode(Directory* directory, PackageNode* packageNode,
 	BReference<Node> nodeReference(node);
 	NodeWriteLocker nodeWriteLocker(node);
 
-	// TODO: This is broken for files that are in use. Cf. _AddPackageNode() for
-	// details.
-
 	PackageNode* headPackageNode = unpackingNode->GetPackageNode();
 	bool nodeRemoved = false;
+	Node* newNode = NULL;
+
+	BReference<Node> newNodeReference;
+	NodeWriteLocker newNodeWriteLocker;
 
 	// If this is the last package node of the node, remove it completely.
 	if (unpackingNode->IsOnlyPackageNode(packageNode)) {
@@ -1316,29 +1352,54 @@ Volume::_RemovePackageNode(Directory* directory, PackageNode* packageNode,
 		// find the node anymore.
 		_NotifyNodeRemoved(node);
 
-		unpackingNode->RemovePackageNode(packageNode);
+		unpackingNode->PrepareForRemoval();
 
-		// we get and put the vnode to notify the VFS
-		// TODO: We should probably only do that, if the node is known to the
-		// VFS in the first place.
-		Node* dummyNode;
-		bool gotVNode = GetVNode(node->ID(), dummyNode) == B_OK;
-
-		_RemoveNode(node);
+		_RemoveNodeAndVNode(node);
 		nodeRemoved = true;
-
-		if (gotVNode) {
-			RemoveVNode(node->ID());
-			PutVNode(node->ID());
-		}
-	} else {
-		// The node does at least have one more package node.
-		unpackingNode->RemovePackageNode(packageNode);
-
-		if (packageNode == headPackageNode) {
+	} else if (packageNode == headPackageNode) {
+		// The node does at least have one more package node, but the one to be
+		// removed is the head. Unless it's a directory, we replace the node
+		// with a completely new one and let the old one die. This is necessary
+		// to avoid surprises for clients that have opened/mapped the node.
+		if (S_ISDIR(packageNode->Mode())) {
+			unpackingNode->RemovePackageNode(packageNode);
 			_NotifyNodeChanged(node, kAllStatFields,
 				OldUnpackingNodeAttributes(headPackageNode));
+		} else {
+			// create a new node and transfer the package nodes to it
+			UnpackingNode* newUnpackingNode;
+			status_t error = unpackingNode->CloneTransferPackageNodes(
+				fNextNodeID++, newUnpackingNode);
+			if (error == B_OK) {
+				// remove the package node
+				newUnpackingNode->RemovePackageNode(packageNode);
+
+				// remove the old node
+				_NotifyNodeRemoved(node);
+				_RemoveNodeAndVNode(node);
+
+				// add the new node
+				newNode = newUnpackingNode->GetNode();
+				newNodeReference.SetTo(newNode);
+				newNodeWriteLocker.SetTo(newNode, false);
+
+				directory->AddChild(newNode);
+				fNodes.Insert(newNode);
+				_NotifyNodeAdded(newNode);
+			} else {
+				// There's nothing we can do. Remove the node completely.
+				_NotifyNodeRemoved(node);
+
+				unpackingNode->PrepareForRemoval();
+
+				_RemoveNodeAndVNode(node);
+				nodeRemoved = true;
+			}
 		}
+	} else {
+		// The package node to remove is not the head of the node. This change
+		// doesn't have any visible effect.
+		unpackingNode->RemovePackageNode(packageNode);
 	}
 
 	if (!notify)
@@ -1348,7 +1409,7 @@ Volume::_RemovePackageNode(Directory* directory, PackageNode* packageNode,
 	if (nodeRemoved) {
 		notify_entry_removed(ID(), directory->ID(), node->Name(), node->ID());
 	} else if (packageNode == headPackageNode) {
-		// The new package node was the one representing the node.
+		// The removed package node was the one representing the node.
 		// Send stat changed notification for directories and entry
 		// removed + created notifications for files and symlinks.
 		if (S_ISDIR(packageNode->Mode())) {
@@ -1357,8 +1418,8 @@ Volume::_RemovePackageNode(Directory* directory, PackageNode* packageNode,
 		} else {
 			notify_entry_removed(ID(), directory->ID(), node->Name(),
 				node->ID());
-			notify_entry_created(ID(), directory->ID(), node->Name(),
-				node->ID());
+			notify_entry_created(ID(), directory->ID(), newNode->Name(),
+				newNode->ID());
 		}
 	}
 }
@@ -1410,6 +1471,24 @@ Volume::_RemoveNode(Node* node)
 }
 
 
+void
+Volume::_RemoveNodeAndVNode(Node* node)
+{
+	// we get and put the vnode to notify the VFS
+	// TODO: We should probably only do that, if the node is known to the
+	// VFS in the first place.
+	Node* dummyNode;
+	bool gotVNode = GetVNode(node->ID(), dummyNode) == B_OK;
+
+	_RemoveNode(node);
+
+	if (gotVNode) {
+		RemoveVNode(node->ID());
+		PutVNode(node->ID());
+	}
+}
+
+
 void
 Volume::_DomainListenerEventOccurred(PackageDomain* domain,
 	const KMessage* event)

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

@@ -1,5 +1,5 @@
 /*
- * Copyright 2009, Ingo Weinhold, ingo_weinhold@gmx.de.
+ * Copyright 2009-2011, Ingo Weinhold, ingo_weinhold@gmx.de.
  * Distributed under the terms of the MIT License.
  */
 #ifndef VOLUME_H
@@ -157,6 +157,8 @@ private:
 									UnpackingNode*& _node);
 									// does *not* return a reference
 			void				_RemoveNode(Node* node);
+			void				_RemoveNodeAndVNode(Node* node);
+									// caller must hold a reference
 
 			void				_DomainListenerEventOccurred(
 									PackageDomain* domain,