From 564f56b15075b0d15349ebfcc24f07b33ce109a0 Mon Sep 17 00:00:00 2001 From: Ingo Weinhold Date: Thu, 21 Jul 2011 03:56:23 +0200 Subject: [PATCH] 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. --- .../packagefs/UnpackingDirectory.cpp | 21 +++ .../packagefs/UnpackingDirectory.h | 4 + .../packagefs/UnpackingLeafNode.cpp | 105 +++++++++-- .../packagefs/UnpackingLeafNode.h | 10 ++ .../file_systems/packagefs/UnpackingNode.cpp | 7 + .../file_systems/packagefs/UnpackingNode.h | 6 + .../kernel/file_systems/packagefs/Volume.cpp | 165 +++++++++++++----- .../kernel/file_systems/packagefs/Volume.h | 4 +- 8 files changed, 262 insertions(+), 60 deletions(-) diff --git a/src/add-ons/kernel/file_systems/packagefs/UnpackingDirectory.cpp b/src/add-ons/kernel/file_systems/packagefs/UnpackingDirectory.cpp index 66606aeb04..ee67354819 100644 --- a/src/add-ons/kernel/file_systems/packagefs/UnpackingDirectory.cpp +++ b/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(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) { diff --git a/src/add-ons/kernel/file_systems/packagefs/UnpackingDirectory.h b/src/add-ons/kernel/file_systems/packagefs/UnpackingDirectory.h index 61d73034fc..baad89b1d5 100644 --- a/src/add-ons/kernel/file_systems/packagefs/UnpackingDirectory.h +++ b/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); diff --git a/src/add-ons/kernel/file_systems/packagefs/UnpackingLeafNode.cpp b/src/add-ons/kernel/file_systems/packagefs/UnpackingLeafNode.cpp index 9d671a389b..3ed264b84c 100644 --- a/src/add-ons/kernel/file_systems/packagefs/UnpackingLeafNode.cpp +++ b/src/add-ons/kernel/file_systems/packagefs/UnpackingLeafNode.cpp @@ -9,6 +9,7 @@ #include #include +#include #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(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(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; +} diff --git a/src/add-ons/kernel/file_systems/packagefs/UnpackingLeafNode.h b/src/add-ons/kernel/file_systems/packagefs/UnpackingLeafNode.h index e1b8db4340..e3bb4f175c 100644 --- a/src/add-ons/kernel/file_systems/packagefs/UnpackingLeafNode.h +++ b/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; }; diff --git a/src/add-ons/kernel/file_systems/packagefs/UnpackingNode.cpp b/src/add-ons/kernel/file_systems/packagefs/UnpackingNode.cpp index dd99ca9dd2..f9700ab7ef 100644 --- a/src/add-ons/kernel/file_systems/packagefs/UnpackingNode.cpp +++ b/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; +} diff --git a/src/add-ons/kernel/file_systems/packagefs/UnpackingNode.h b/src/add-ons/kernel/file_systems/packagefs/UnpackingNode.h index 3ac50316c7..28130ca661 100644 --- a/src/add-ons/kernel/file_systems/packagefs/UnpackingNode.h +++ b/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); }; diff --git a/src/add-ons/kernel/file_systems/packagefs/Volume.cpp b/src/add-ons/kernel/file_systems/packagefs/Volume.cpp index 88e96cf3a0..8e7c70e576 100644 --- a/src/add-ons/kernel/file_systems/packagefs/Volume.cpp +++ b/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 nodeReference(node); NodeWriteLocker nodeWriteLocker(node); + BReference 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 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 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) diff --git a/src/add-ons/kernel/file_systems/packagefs/Volume.h b/src/add-ons/kernel/file_systems/packagefs/Volume.h index d939b66a8f..d2356180ac 100644 --- a/src/add-ons/kernel/file_systems/packagefs/Volume.h +++ b/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,