From 118028674c77ec4ba5725a097dfbc9fd99b181b8 Mon Sep 17 00:00:00 2001 From: Ingo Weinhold Date: Sat, 1 Jun 2013 00:31:37 +0200 Subject: [PATCH] Fix reading attributes of directories When reading an attribute of a directory there was no guarantee that the underlying package would be open. When it wasn't reading an attribute would fail, unless the attribute data were already cached. The reasons for this are: * UnpackingDirectory didn't forward the {Init,Uninit}VFS() calls to the underlying PackageDirectory. * Only PackageFile was actually opening the package in InitVFS(). Now we forward the {Init,Uninit}VFS() calls in all cases -- even in {Add,Remove}PackageNode(), when the active package node changes -- and opening/closing the package is now done in PackageNode::{Init,Uninit}VFS(). --- .../file_systems/packagefs/nodes/Node.h | 17 ++++-- .../packagefs/nodes/UnpackingDirectory.cpp | 37 ++++++++++-- .../packagefs/nodes/UnpackingDirectory.h | 11 +++- .../packagefs/nodes/UnpackingLeafNode.cpp | 32 +++++++---- .../packagefs/nodes/UnpackingLeafNode.h | 8 ++- .../packagefs/nodes/UnpackingNode.cpp | 57 ++++++++++++++++++- .../packagefs/nodes/UnpackingNode.h | 18 +++++- .../packagefs/package/PackageFile.cpp | 7 +-- .../packagefs/package/PackageNode.cpp | 8 ++- .../file_systems/packagefs/volume/Volume.cpp | 8 +-- 10 files changed, 165 insertions(+), 38 deletions(-) diff --git a/src/add-ons/kernel/file_systems/packagefs/nodes/Node.h b/src/add-ons/kernel/file_systems/packagefs/nodes/Node.h index 3924fb0048..a1e72718b7 100644 --- a/src/add-ons/kernel/file_systems/packagefs/nodes/Node.h +++ b/src/add-ons/kernel/file_systems/packagefs/nodes/Node.h @@ -1,5 +1,5 @@ /* - * Copyright 2009-2011, Ingo Weinhold, ingo_weinhold@gmx.de. + * Copyright 2009-2013, Ingo Weinhold, ingo_weinhold@gmx.de. * Distributed under the terms of the MIT License. */ #ifndef NODE_H @@ -27,10 +27,11 @@ class Directory; class PackageNode; -// node flags +// internal node flags enum { - NODE_FLAG_KNOWN_TO_VFS = 0x01 - // internal flag + NODE_FLAG_KNOWN_TO_VFS = 0x01, + NODE_FLAG_VFS_INIT_ERROR = 0x02, + // used by subclasses }; @@ -61,6 +62,7 @@ public: virtual void VFSUninit(); // base class version must be called inline bool IsKnownToVFS() const; + inline bool HasVFSInitError() const; void SetID(ino_t id); void SetParent(Directory* parent); @@ -133,6 +135,13 @@ Node::IsKnownToVFS() const } +bool +Node::HasVFSInitError() const +{ + return (fFlags & NODE_FLAG_VFS_INIT_ERROR) != 0; +} + + // #pragma mark - diff --git a/src/add-ons/kernel/file_systems/packagefs/nodes/UnpackingDirectory.cpp b/src/add-ons/kernel/file_systems/packagefs/nodes/UnpackingDirectory.cpp index 22410b9d16..355c3f9b19 100644 --- a/src/add-ons/kernel/file_systems/packagefs/nodes/UnpackingDirectory.cpp +++ b/src/add-ons/kernel/file_systems/packagefs/nodes/UnpackingDirectory.cpp @@ -1,5 +1,5 @@ /* - * Copyright 2009-2011, Ingo Weinhold, ingo_weinhold@gmx.de. + * Copyright 2009-2013, Ingo Weinhold, ingo_weinhold@gmx.de. * Distributed under the terms of the MIT License. */ @@ -27,6 +27,25 @@ UnpackingDirectory::~UnpackingDirectory() } +status_t +UnpackingDirectory::VFSInit(dev_t deviceID) +{ + status_t error = NodeInitVFS(deviceID, fID, fPackageDirectories.Head()); + if (error == B_OK) + Directory::VFSInit(deviceID); + + return error; +} + + +void +UnpackingDirectory::VFSUninit() +{ + NodeUninitVFS(fPackageDirectories.Head(), fFlags); + Directory::VFSUninit(); +} + + mode_t UnpackingDirectory::Mode() const { @@ -80,7 +99,7 @@ UnpackingDirectory::GetNode() status_t -UnpackingDirectory::AddPackageNode(PackageNode* packageNode) +UnpackingDirectory::AddPackageNode(PackageNode* packageNode, dev_t deviceID) { if (!S_ISDIR(packageNode->Mode())) return B_BAD_VALUE; @@ -92,9 +111,10 @@ UnpackingDirectory::AddPackageNode(PackageNode* packageNode) bool isNewest = other == NULL || packageDirectory->ModifiedTime() > other->ModifiedTime(); - if (isNewest) + if (isNewest) { fPackageDirectories.Insert(other, packageDirectory); - else + NodeReinitVFS(deviceID, fID, packageDirectory, other, fFlags); + } else fPackageDirectories.Add(packageDirectory); return B_OK; @@ -102,7 +122,7 @@ UnpackingDirectory::AddPackageNode(PackageNode* packageNode) void -UnpackingDirectory::RemovePackageNode(PackageNode* packageNode) +UnpackingDirectory::RemovePackageNode(PackageNode* packageNode, dev_t deviceID) { bool isNewest = packageNode == fPackageDirectories.Head(); fPackageDirectories.Remove(dynamic_cast(packageNode)); @@ -121,6 +141,7 @@ UnpackingDirectory::RemovePackageNode(PackageNode* packageNode) fPackageDirectories.Remove(newestNode); fPackageDirectories.Insert(fPackageDirectories.Head(), newestNode); + NodeReinitVFS(deviceID, fID, newestNode, packageNode, fFlags); } } @@ -164,6 +185,9 @@ UnpackingDirectory::PrepareForRemoval() status_t UnpackingDirectory::OpenAttributeDirectory(AttributeDirectoryCookie*& _cookie) { + if (HasVFSInitError()) + return B_ERROR; + return UnpackingAttributeDirectoryCookie::Open(fPackageDirectories.Head(), _cookie); } @@ -173,6 +197,9 @@ status_t UnpackingDirectory::OpenAttribute(const StringKey& name, int openMode, AttributeCookie*& _cookie) { + if (HasVFSInitError()) + return B_ERROR; + return UnpackingAttributeCookie::Open(fPackageDirectories.Head(), name, openMode, _cookie); } diff --git a/src/add-ons/kernel/file_systems/packagefs/nodes/UnpackingDirectory.h b/src/add-ons/kernel/file_systems/packagefs/nodes/UnpackingDirectory.h index 4cebf6da8c..6d09c27aec 100644 --- a/src/add-ons/kernel/file_systems/packagefs/nodes/UnpackingDirectory.h +++ b/src/add-ons/kernel/file_systems/packagefs/nodes/UnpackingDirectory.h @@ -1,5 +1,5 @@ /* - * Copyright 2009-2011, Ingo Weinhold, ingo_weinhold@gmx.de. + * Copyright 2009-2013, Ingo Weinhold, ingo_weinhold@gmx.de. * Distributed under the terms of the MIT License. */ #ifndef UNPACKING_DIRECTORY_H @@ -16,6 +16,9 @@ public: UnpackingDirectory(ino_t id); virtual ~UnpackingDirectory(); + virtual status_t VFSInit(dev_t deviceID); + virtual void VFSUninit(); + virtual mode_t Mode() const; virtual uid_t UserID() const; virtual gid_t GroupID() const; @@ -24,8 +27,10 @@ public: virtual Node* GetNode(); - virtual status_t AddPackageNode(PackageNode* packageNode); - virtual void RemovePackageNode(PackageNode* packageNode); + virtual status_t AddPackageNode(PackageNode* packageNode, + dev_t deviceID); + virtual void RemovePackageNode(PackageNode* packageNode, + dev_t deviceID); virtual PackageNode* GetPackageNode(); virtual bool IsOnlyPackageNode(PackageNode* node) const; diff --git a/src/add-ons/kernel/file_systems/packagefs/nodes/UnpackingLeafNode.cpp b/src/add-ons/kernel/file_systems/packagefs/nodes/UnpackingLeafNode.cpp index 51f80bb784..bfaac35142 100644 --- a/src/add-ons/kernel/file_systems/packagefs/nodes/UnpackingLeafNode.cpp +++ b/src/add-ons/kernel/file_systems/packagefs/nodes/UnpackingLeafNode.cpp @@ -1,5 +1,5 @@ /* - * Copyright 2009-2011, Ingo Weinhold, ingo_weinhold@gmx.de. + * Copyright 2009-2013, Ingo Weinhold, ingo_weinhold@gmx.de. * Distributed under the terms of the MIT License. */ @@ -34,10 +34,7 @@ UnpackingLeafNode::~UnpackingLeafNode() status_t UnpackingLeafNode::VFSInit(dev_t deviceID) { - status_t error = B_OK; - if (PackageLeafNode* packageNode = _ActivePackageNode()) - error = packageNode->VFSInit(deviceID, fID); - + status_t error = NodeInitVFS(deviceID, fID, _ActivePackageNode()); if (error == B_OK) Node::VFSInit(deviceID); @@ -48,9 +45,7 @@ UnpackingLeafNode::VFSInit(dev_t deviceID) void UnpackingLeafNode::VFSUninit() { - if (PackageLeafNode* packageNode = _ActivePackageNode()) - packageNode->VFSUninit(); - + NodeUninitVFS(_ActivePackageNode(), fFlags); Node::VFSUninit(); } @@ -110,7 +105,7 @@ UnpackingLeafNode::GetNode() status_t -UnpackingLeafNode::AddPackageNode(PackageNode* packageNode) +UnpackingLeafNode::AddPackageNode(PackageNode* packageNode, dev_t deviceID) { ASSERT(fFinalPackageNode == NULL); @@ -126,6 +121,7 @@ UnpackingLeafNode::AddPackageNode(PackageNode* packageNode) if (isNewest) { fPackageNodes.Add(packageLeafNode); + NodeReinitVFS(deviceID, fID, packageLeafNode, headNode, fFlags); } else { // add after the head fPackageNodes.RemoveHead(); @@ -138,7 +134,7 @@ UnpackingLeafNode::AddPackageNode(PackageNode* packageNode) void -UnpackingLeafNode::RemovePackageNode(PackageNode* packageNode) +UnpackingLeafNode::RemovePackageNode(PackageNode* packageNode, dev_t deviceID) { ASSERT(fFinalPackageNode == NULL); @@ -160,6 +156,7 @@ UnpackingLeafNode::RemovePackageNode(PackageNode* packageNode) // re-add the newest node to the head fPackageNodes.Remove(newestNode); fPackageNodes.Add(newestNode); + NodeReinitVFS(deviceID, fID, newestNode, packageNode, fFlags); } } @@ -238,6 +235,9 @@ UnpackingLeafNode::CloneTransferPackageNodes(ino_t id, UnpackingNode*& _newNode) status_t UnpackingLeafNode::Read(off_t offset, void* buffer, size_t* bufferSize) { + if (HasVFSInitError()) + return B_ERROR; + if (PackageLeafNode* packageNode = _ActivePackageNode()) return packageNode->Read(offset, buffer, bufferSize); return B_ERROR; @@ -247,6 +247,9 @@ UnpackingLeafNode::Read(off_t offset, void* buffer, size_t* bufferSize) status_t UnpackingLeafNode::Read(io_request* request) { + if (HasVFSInitError()) + return B_ERROR; + if (PackageLeafNode* packageNode = _ActivePackageNode()) return packageNode->Read(request); return EBADF; @@ -256,6 +259,9 @@ UnpackingLeafNode::Read(io_request* request) status_t UnpackingLeafNode::ReadSymlink(void* buffer, size_t* bufferSize) { + if (HasVFSInitError()) + return B_ERROR; + PackageLeafNode* packageNode = _ActivePackageNode(); if (packageNode == NULL) return B_BAD_VALUE; @@ -277,6 +283,9 @@ UnpackingLeafNode::ReadSymlink(void* buffer, size_t* bufferSize) status_t UnpackingLeafNode::OpenAttributeDirectory(AttributeDirectoryCookie*& _cookie) { + if (HasVFSInitError()) + return B_ERROR; + return UnpackingAttributeDirectoryCookie::Open(_ActivePackageNode(), _cookie); } @@ -286,6 +295,9 @@ status_t UnpackingLeafNode::OpenAttribute(const StringKey& name, int openMode, AttributeCookie*& _cookie) { + if (HasVFSInitError()) + return B_ERROR; + return UnpackingAttributeCookie::Open(_ActivePackageNode(), name, openMode, _cookie); } diff --git a/src/add-ons/kernel/file_systems/packagefs/nodes/UnpackingLeafNode.h b/src/add-ons/kernel/file_systems/packagefs/nodes/UnpackingLeafNode.h index 914e9a4903..e70ad2a371 100644 --- a/src/add-ons/kernel/file_systems/packagefs/nodes/UnpackingLeafNode.h +++ b/src/add-ons/kernel/file_systems/packagefs/nodes/UnpackingLeafNode.h @@ -1,5 +1,5 @@ /* - * Copyright 2009-2011, Ingo Weinhold, ingo_weinhold@gmx.de. + * Copyright 2009-2013, Ingo Weinhold, ingo_weinhold@gmx.de. * Distributed under the terms of the MIT License. */ #ifndef LEAF_NODE_H @@ -27,8 +27,10 @@ public: virtual Node* GetNode(); - virtual status_t AddPackageNode(PackageNode* packageNode); - virtual void RemovePackageNode(PackageNode* packageNode); + virtual status_t AddPackageNode(PackageNode* packageNode, + dev_t deviceID); + virtual void RemovePackageNode(PackageNode* packageNode, + dev_t deviceID); virtual PackageNode* GetPackageNode(); virtual bool IsOnlyPackageNode(PackageNode* node) const; diff --git a/src/add-ons/kernel/file_systems/packagefs/nodes/UnpackingNode.cpp b/src/add-ons/kernel/file_systems/packagefs/nodes/UnpackingNode.cpp index f9700ab7ef..63800159e9 100644 --- a/src/add-ons/kernel/file_systems/packagefs/nodes/UnpackingNode.cpp +++ b/src/add-ons/kernel/file_systems/packagefs/nodes/UnpackingNode.cpp @@ -1,11 +1,15 @@ /* - * Copyright 2011, Ingo Weinhold, ingo_weinhold@gmx.de. + * Copyright 2011-2013, Ingo Weinhold, ingo_weinhold@gmx.de. * Distributed under the terms of the MIT License. */ #include "UnpackingNode.h" +#include "DebugSupport.h" +#include "Node.h" +#include "PackageNode.h" + UnpackingNode::~UnpackingNode() { @@ -17,3 +21,54 @@ UnpackingNode::CloneTransferPackageNodes(ino_t id, UnpackingNode*& _newNode) { return B_BAD_VALUE; } + + +status_t +UnpackingNode::NodeInitVFS(dev_t deviceID, ino_t nodeID, + PackageNode* packageNode) +{ + status_t error = B_OK; + if (packageNode != NULL) + error = packageNode->VFSInit(deviceID, nodeID); + + return error; +} + + +void +UnpackingNode::NodeUninitVFS(PackageNode* packageNode, uint32& nodeFlags) +{ + if (packageNode != NULL) { + if ((nodeFlags & NODE_FLAG_VFS_INIT_ERROR) == 0) + packageNode->VFSUninit(); + else + nodeFlags &= ~(uint32)NODE_FLAG_VFS_INIT_ERROR; + } +} + + +void +UnpackingNode::NodeReinitVFS(dev_t deviceID, ino_t nodeID, + PackageNode* packageNode, PackageNode* previousPackageNode, + uint32& nodeFlags) +{ + if ((nodeFlags & NODE_FLAG_KNOWN_TO_VFS) == 0) + return; + + if (packageNode != previousPackageNode) { + bool hadInitError = (nodeFlags & NODE_FLAG_VFS_INIT_ERROR) != 0; + nodeFlags &= ~(uint32)NODE_FLAG_VFS_INIT_ERROR; + + if (packageNode != NULL) { + status_t error = packageNode->VFSInit(deviceID, nodeID); + if (error != B_OK) { + ERROR("UnpackingNode::NodeReinitVFS(): VFSInit() failed for " + "(%" B_PRIdDEV ", %" B_PRIdINO ")\n", deviceID, nodeID); + nodeFlags |= NODE_FLAG_VFS_INIT_ERROR; + } + } + + if (previousPackageNode != NULL && !hadInitError) + previousPackageNode->VFSUninit(); + } +} diff --git a/src/add-ons/kernel/file_systems/packagefs/nodes/UnpackingNode.h b/src/add-ons/kernel/file_systems/packagefs/nodes/UnpackingNode.h index 28130ca661..bfb15cfed7 100644 --- a/src/add-ons/kernel/file_systems/packagefs/nodes/UnpackingNode.h +++ b/src/add-ons/kernel/file_systems/packagefs/nodes/UnpackingNode.h @@ -1,5 +1,5 @@ /* - * Copyright 2011, Ingo Weinhold, ingo_weinhold@gmx.de. + * Copyright 2011-2013, Ingo Weinhold, ingo_weinhold@gmx.de. * Distributed under the terms of the MIT License. */ #ifndef UNPACKING_NODE_H @@ -19,8 +19,10 @@ public: virtual Node* GetNode() = 0; - virtual status_t AddPackageNode(PackageNode* packageNode) = 0; - virtual void RemovePackageNode(PackageNode* packageNode) = 0; + virtual status_t AddPackageNode(PackageNode* packageNode, + dev_t deviceID) = 0; + virtual void RemovePackageNode(PackageNode* packageNode, + dev_t deviceID) = 0; virtual PackageNode* GetPackageNode() = 0; virtual bool IsOnlyPackageNode(PackageNode* node) const = 0; @@ -30,6 +32,16 @@ public: virtual void PrepareForRemoval() = 0; virtual status_t CloneTransferPackageNodes(ino_t id, UnpackingNode*& _newNode); + +protected: + status_t NodeInitVFS(dev_t deviceID, ino_t nodeID, + PackageNode* packageNode); + void NodeUninitVFS(PackageNode* packageNode, + uint32& nodeFlags); + void NodeReinitVFS(dev_t deviceID, ino_t nodeID, + PackageNode* packageNode, + PackageNode* previousPackageNode, + uint32& nodeFlags); }; diff --git a/src/add-ons/kernel/file_systems/packagefs/package/PackageFile.cpp b/src/add-ons/kernel/file_systems/packagefs/package/PackageFile.cpp index 2fa558cced..469251a3af 100644 --- a/src/add-ons/kernel/file_systems/packagefs/package/PackageFile.cpp +++ b/src/add-ons/kernel/file_systems/packagefs/package/PackageFile.cpp @@ -1,5 +1,5 @@ /* - * Copyright 2009, Ingo Weinhold, ingo_weinhold@gmx.de. + * Copyright 2009-2013, Ingo Weinhold, ingo_weinhold@gmx.de. * Distributed under the terms of the MIT License. */ @@ -150,7 +150,8 @@ PackageFile::VFSInit(dev_t deviceID, ino_t nodeID) MethodDeleter baseClassUninit(this, &PackageNode::NonVirtualVFSUninit); - // open the package + // open the package -- that's already done by PackageNode::VFSInit(), so it + // shouldn't fail here. We only need to do it again, since we need the FD. int fd = fPackage->Open(); if (fd < 0) RETURN_ERROR(fd); @@ -168,7 +169,6 @@ PackageFile::VFSInit(dev_t deviceID, ino_t nodeID) return error; } - packageCloser.Detach(); baseClassUninit.Detach(); return B_OK; } @@ -178,7 +178,6 @@ void PackageFile::VFSUninit() { if (fDataAccessor != NULL) { - fPackage->Close(); delete fDataAccessor; fDataAccessor = NULL; } diff --git a/src/add-ons/kernel/file_systems/packagefs/package/PackageNode.cpp b/src/add-ons/kernel/file_systems/packagefs/package/PackageNode.cpp index 1888da39d8..689b7cd732 100644 --- a/src/add-ons/kernel/file_systems/packagefs/package/PackageNode.cpp +++ b/src/add-ons/kernel/file_systems/packagefs/package/PackageNode.cpp @@ -1,5 +1,5 @@ /* - * Copyright 2009, Ingo Weinhold, ingo_weinhold@gmx.de. + * Copyright 2009-2013, Ingo Weinhold, ingo_weinhold@gmx.de. * Distributed under the terms of the MIT License. */ @@ -44,6 +44,11 @@ PackageNode::Init(PackageDirectory* parent, const String& name) status_t PackageNode::VFSInit(dev_t deviceID, ino_t nodeID) { + // open the package + int fd = fPackage->Open(); + if (fd < 0) + RETURN_ERROR(fd); + fPackage->AcquireReference(); return B_OK; } @@ -52,6 +57,7 @@ PackageNode::VFSInit(dev_t deviceID, ino_t nodeID) void PackageNode::VFSUninit() { + fPackage->Close(); fPackage->ReleaseReference(); } diff --git a/src/add-ons/kernel/file_systems/packagefs/volume/Volume.cpp b/src/add-ons/kernel/file_systems/packagefs/volume/Volume.cpp index fc24afe94a..8dda2c69ad 100644 --- a/src/add-ons/kernel/file_systems/packagefs/volume/Volume.cpp +++ b/src/add-ons/kernel/file_systems/packagefs/volume/Volume.cpp @@ -1192,7 +1192,7 @@ Volume::_AddPackageNode(Directory* directory, PackageNode* packageNode, newNode = true; } - status_t error = unpackingNode->AddPackageNode(packageNode); + status_t error = unpackingNode->AddPackageNode(packageNode, ID()); if (error != B_OK) { // Remove the node, if created before. If the node was created to // replace the previous node, send out notifications instead. @@ -1274,7 +1274,7 @@ Volume::_RemovePackageNode(Directory* directory, PackageNode* packageNode, // 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); + unpackingNode->RemovePackageNode(packageNode, ID()); _NotifyNodeChanged(node, kAllStatFields, OldUnpackingNodeAttributes(headPackageNode)); } else { @@ -1284,7 +1284,7 @@ Volume::_RemovePackageNode(Directory* directory, PackageNode* packageNode, fNextNodeID++, newUnpackingNode); if (error == B_OK) { // remove the package node - newUnpackingNode->RemovePackageNode(packageNode); + newUnpackingNode->RemovePackageNode(packageNode, ID()); // remove the old node _NotifyNodeRemoved(node); @@ -1311,7 +1311,7 @@ Volume::_RemovePackageNode(Directory* directory, PackageNode* packageNode, } else { // The package node to remove is not the head of the node. This change // doesn't have any visible effect. - unpackingNode->RemovePackageNode(packageNode); + unpackingNode->RemovePackageNode(packageNode, ID()); } if (!notify)