From df74e77075f6a13c1f8c48794ce09f63145622d5 Mon Sep 17 00:00:00 2001
From: Manali Bhutiyani <manalib@google.com>
Date: Mon, 13 May 2019 20:55:52 +0000
Subject: [PATCH] [minfs] Make fsck unfriend minfs class.
In order to make the code more encapsulated , this change removes the "friend"
relation between fsck and minfs class as well as inode-manager class. We use the interfaces
added as part of inspector changes and instead of fsck directly accessing the private members,
we use these Interfaces instead.
Testing: No behavior change. Merely converted private members to be accessed via public interfaces instead of directly via friend relation.
Change-Id: I8fae7acf6d59b47ad2c3376a91d602a127e34de7
---
.../ulib/minfs/allocator/inode-manager.cpp | 11 ++++++-
.../ulib/minfs/allocator/inode-manager.h | 10 ++++--
zircon/system/ulib/minfs/fsck.cpp | 33 ++++++++++---------
zircon/system/ulib/minfs/minfs-private.h | 20 +++++++++--
.../system/ulib/minfs/test/inspector-test.cpp | 9 +++++
zircon/system/ulib/minfs/vnode.h | 3 --
6 files changed, 62 insertions(+), 24 deletions(-)
diff --git a/zircon/system/ulib/minfs/allocator/inode-manager.cpp b/zircon/system/ulib/minfs/allocator/inode-manager.cpp
index 9d4fa2cfbf3..2bcdb03e963 100644
--- a/zircon/system/ulib/minfs/allocator/inode-manager.cpp
+++ b/zircon/system/ulib/minfs/allocator/inode-manager.cpp
@@ -12,7 +12,12 @@
namespace minfs {
InodeManager::InodeManager(Bcache* bc, blk_t start_block) :
- bc_(bc), start_block_(start_block) {}
+ start_block_(start_block) {
+#ifndef __Fuchsia__
+ bc_ = bc;
+#endif
+}
+
InodeManager::~InodeManager() = default;
zx_status_t InodeManager::Create(Bcache* bc, SuperblockManager* sb, fs::ReadTxn* txn,
@@ -74,6 +79,10 @@ void InodeManager::Update(WriteTxn* txn, ino_t ino, const Inode* inode) {
#endif
}
+const Allocator* InodeManager::GetInodeAllocator() const {
+ return inode_allocator_.get();
+}
+
void InodeManager::Load(ino_t ino, Inode* out) const {
// obtain the block of the inode table we need
uint32_t off_of_ino = (ino % kMinfsInodesPerBlock) * kMinfsInodeSize;
diff --git a/zircon/system/ulib/minfs/allocator/inode-manager.h b/zircon/system/ulib/minfs/allocator/inode-manager.h
index 3b9f87ef93f..9dfbcb7d905 100644
--- a/zircon/system/ulib/minfs/allocator/inode-manager.h
+++ b/zircon/system/ulib/minfs/allocator/inode-manager.h
@@ -26,6 +26,9 @@ class InspectableInodeManager {
public:
virtual ~InspectableInodeManager() {}
+ // Gets immutable reference to the inode allocator.
+ virtual const Allocator* GetInodeAllocator() const = 0;
+
// Loads the inode from storage.
virtual void Load(ino_t inode_num, Inode* out) const = 0;
};
@@ -59,6 +62,8 @@ public:
void Update(WriteTxn* txn, ino_t ino, const Inode* inode);
// InspectableInodeManager interface:
+ const Allocator* GetInodeAllocator() const final;
+
void Load(ino_t ino, Inode* out) const final;
// Extend the number of inodes managed.
@@ -68,11 +73,10 @@ public:
zx_status_t Grow(size_t inodes);
private:
- friend class MinfsChecker;
-
InodeManager(Bcache* bc, blk_t start_block);
-
+#ifndef __Fuchsia__
Bcache* bc_;
+#endif
blk_t start_block_;
fbl::unique_ptr<Allocator> inode_allocator_;
#ifdef __Fuchsia__
diff --git a/zircon/system/ulib/minfs/fsck.cpp b/zircon/system/ulib/minfs/fsck.cpp
index 01168db8c4f..b88377f5d77 100644
--- a/zircon/system/ulib/minfs/fsck.cpp
+++ b/zircon/system/ulib/minfs/fsck.cpp
@@ -70,7 +70,7 @@ zx_status_t MinfsChecker::GetInode(Inode* inode, ino_t ino) {
return ZX_ERR_OUT_OF_RANGE;
}
- fs_->inodes_->Load(ino, inode);
+ fs_->GetInodeManager()->Load(ino, inode);
if ((inode->magic != kMinfsMagicFile) && (inode->magic != kMinfsMagicDir)) {
FS_TRACE_ERROR("check: ino %u has bad magic %#x\n", ino, inode->magic);
return ZX_ERR_IO_DATA_INTEGRITY;
@@ -217,7 +217,8 @@ zx_status_t MinfsChecker::CheckDirectory(Inode* inode, ino_t ino,
bool dot_or_dotdot = false;
if ((de->namelen == 0) || (de->namelen > (rlen - MINFS_DIRENT_SIZE))) {
- FS_TRACE_ERROR("check: ino#%u: de[%u]: invalid namelen %u\n", ino, eno, de->namelen);
+ FS_TRACE_ERROR("check: ino#%u: de[%u]: invalid namelen %u\n", ino, eno,
+ de->namelen);
return ZX_ERR_IO_DATA_INTEGRITY;
}
if ((de->namelen == 1) && (de->name[0] == '.')) {
@@ -227,7 +228,8 @@ zx_status_t MinfsChecker::CheckDirectory(Inode* inode, ino_t ino,
dot_or_dotdot = true;
dot = true;
if (de->ino != ino) {
- FS_TRACE_ERROR("check: ino#%u: de[%u]: '.' ino=%u (not self!)\n", ino, eno, de->ino);
+ FS_TRACE_ERROR("check: ino#%u: de[%u]: '.' ino=%u (not self!)\n", ino, eno,
+ de->ino);
}
}
if ((de->namelen == 2) && (de->name[0] == '.') && (de->name[1] == '.')) {
@@ -237,13 +239,14 @@ zx_status_t MinfsChecker::CheckDirectory(Inode* inode, ino_t ino,
dot_or_dotdot = true;
dotdot = true;
if (de->ino != parent) {
- FS_TRACE_ERROR("check: ino#%u: de[%u]: '..' ino=%u (not parent!)\n", ino, eno, de->ino);
+ FS_TRACE_ERROR("check: ino#%u: de[%u]: '..' ino=%u (not parent!)\n", ino, eno,
+ de->ino);
}
}
//TODO: check for cycles (non-dot/dotdot dir ref already in checked bitmap)
if (flags & CD_DUMP) {
- FS_TRACE_DEBUG("ino#%u: de[%u]: ino=%u type=%u '%.*s' %s\n", ino, eno, de->ino, de->type,
- de->namelen, de->name, is_last ? "[last]" : "");
+ FS_TRACE_DEBUG("ino#%u: de[%u]: ino=%u type=%u '%.*s' %s\n", ino, eno, de->ino,
+ de->type, de->namelen, de->name, is_last ? "[last]" : "");
}
if (flags & CD_RECURSE) {
@@ -280,7 +283,7 @@ const char* MinfsChecker::CheckDataBlock(blk_t bno) {
if (bno >= fs_->Info().block_count) {
return "out of range";
}
- if (!fs_->block_allocator_->CheckAllocated(bno)) {
+ if (!fs_->GetBlockAllocator()->CheckAllocated(bno)) {
return "not allocated";
}
if (checked_blocks_.Get(bno, bno + 1)) {
@@ -393,7 +396,7 @@ zx_status_t MinfsChecker::CheckFile(Inode* inode, ino_t ino) {
void MinfsChecker::CheckReserved() {
// Check reserved inode '0'.
- if (fs_->inodes_->inode_allocator_->CheckAllocated(0)) {
+ if (fs_->GetInodeManager()->GetInodeAllocator()->CheckAllocated(0)) {
checked_inodes_.Set(0, 1);
alloc_inodes_++;
} else {
@@ -402,7 +405,7 @@ void MinfsChecker::CheckReserved() {
}
// Check reserved data block '0'.
- if (fs_->block_allocator_->CheckAllocated(0)) {
+ if (fs_->GetBlockAllocator()->CheckAllocated(0)) {
checked_blocks_.Set(0, 1);
alloc_blocks_++;
} else {
@@ -438,7 +441,7 @@ zx_status_t MinfsChecker::CheckInode(ino_t ino, ino_t parent, bool dot_or_dotdot
checked_inodes_.Set(ino, ino + 1);
alloc_inodes_++;
- if (!fs_->inodes_->inode_allocator_->CheckAllocated(ino)) {
+ if (!fs_->GetInodeManager()->GetInodeAllocator()->CheckAllocated(ino)) {
FS_TRACE_WARN("check: ino#%u: not marked in-use\n", ino);
conforming_ = false;
}
@@ -455,8 +458,8 @@ zx_status_t MinfsChecker::CheckInode(ino_t ino, ino_t parent, bool dot_or_dotdot
return status;
}
} else {
- FS_TRACE_DEBUG("ino#%u: FILE blks=%u links=%u size=%u\n", ino, inode.block_count, inode.link_count,
- inode.size);
+ FS_TRACE_DEBUG("ino#%u: FILE blks=%u links=%u size=%u\n", ino, inode.block_count,
+ inode.link_count, inode.size);
if ((status = CheckFile(&inode, ino)) < 0) {
return status;
}
@@ -516,7 +519,7 @@ zx_status_t MinfsChecker::CheckForUnusedBlocks() const {
unsigned missing = 0;
for (unsigned n = 0; n < fs_->Info().block_count; n++) {
- if (fs_->block_allocator_->CheckAllocated(n)) {
+ if (fs_->GetBlockAllocator()->CheckAllocated(n)) {
if (!checked_blocks_.Get(n, n + 1)) {
missing++;
}
@@ -533,7 +536,7 @@ zx_status_t MinfsChecker::CheckForUnusedBlocks() const {
zx_status_t MinfsChecker::CheckForUnusedInodes() const {
unsigned missing = 0;
for (unsigned n = 0; n < fs_->Info().inode_count; n++) {
- if (fs_->inodes_->inode_allocator_->CheckAllocated(n)) {
+ if (fs_->GetInodeManager()->GetInodeAllocator()->CheckAllocated(n)) {
if (!checked_inodes_.Get(n, n + 1)) {
missing++;
}
@@ -587,7 +590,7 @@ zx_status_t MinfsChecker::CheckJournal() const {
#ifdef __Fuchsia__
journal_block = fs_->Info().journal_start_block;
#else
- journal_block = fs_->offsets_.JournalStartBlock();
+ journal_block = fs_->GetBlockOffsets().JournalStartBlock();
#endif
if (fs_->bc_->Readblk(journal_block, data) < 0) {
diff --git a/zircon/system/ulib/minfs/minfs-private.h b/zircon/system/ulib/minfs/minfs-private.h
index 49c0d4d116c..f3572d1ed7c 100644
--- a/zircon/system/ulib/minfs/minfs-private.h
+++ b/zircon/system/ulib/minfs/minfs-private.h
@@ -166,8 +166,16 @@ public:
// Gets an immutable reference to the InodeManager.
virtual const InspectableInodeManager* GetInodeManager() const = 0;
+ // Gets an immutable reference to the block_allocator.
+ virtual const Allocator* GetBlockAllocator() const = 0;
+
// Reads a block at the |start_block_num| location.
virtual zx_status_t ReadBlock(blk_t start_block_num, void* out_data) const = 0;
+
+#ifndef __Fuchsia__
+ // Gets an immutable copy of offsets_.
+ virtual const BlockOffsets GetBlockOffsets() const = 0;
+#endif
};
class Minfs :
@@ -347,6 +355,16 @@ public:
return inodes_.get();
}
+ const Allocator* GetBlockAllocator() const final {
+ return block_allocator_.get();
+ }
+
+#ifndef __Fuchsia__
+ const BlockOffsets GetBlockOffsets() const final {
+ return offsets_;
+ }
+#endif
+
zx_status_t ReadBlock(blk_t start_block_num, void* data) const final;
const TransactionLimits& Limits() const {
@@ -363,8 +381,6 @@ public:
fbl::unique_ptr<Bcache> bc_;
private:
- // Fsck can introspect Minfs
- friend class MinfsChecker;
using HashTable = fbl::HashTable<ino_t, VnodeMinfs*>;
#ifdef __Fuchsia__
diff --git a/zircon/system/ulib/minfs/test/inspector-test.cpp b/zircon/system/ulib/minfs/test/inspector-test.cpp
index 6d818eee667..660dc0df091 100644
--- a/zircon/system/ulib/minfs/test/inspector-test.cpp
+++ b/zircon/system/ulib/minfs/test/inspector-test.cpp
@@ -23,12 +23,17 @@ public:
MockInodeManager& operator=(MockInodeManager&&) = delete;
void Load(ino_t inode_num, Inode* out) const final;
+ const Allocator* GetInodeAllocator() const final;
};
MockInodeManager::MockInodeManager() {}
void MockInodeManager::Load(ino_t inode_num, Inode* out) const {}
+const Allocator* MockInodeManager::GetInodeAllocator() const {
+ return nullptr;
+}
+
constexpr Superblock superblock = {};
// Mock Minfs class to be used in inspector tests.
@@ -48,6 +53,10 @@ public:
return nullptr;
}
+ const Allocator* GetBlockAllocator() const final {
+ return nullptr;
+ }
+
zx_status_t ReadBlock(blk_t start_block_num, void* out_data) const final {
return ZX_OK;
}
diff --git a/zircon/system/ulib/minfs/vnode.h b/zircon/system/ulib/minfs/vnode.h
index 79044a05ea9..72b0de9c24a 100644
--- a/zircon/system/ulib/minfs/vnode.h
+++ b/zircon/system/ulib/minfs/vnode.h
@@ -199,9 +199,6 @@ public:
// of "File + Directory + Vnode" being a single class. They should be transitioned to
// private.
protected:
- // Fsck can introspect Minfs
- friend class MinfsChecker;
-
VnodeMinfs(Minfs* fs);
// fs::Vnode interface.
--
GitLab