Skip to content

Commit

Permalink
[io] Harmonize Query.GetInfo and QueryFilesystem.
Browse files Browse the repository at this point in the history
Previously there were two different FilesystemInfo structs, one for
fuchsia.fs.Query.GetInfo() and one for
fuchsia.io.admin.DirectoryAdmin.QueryFilesystem(). These were similar
but not identical.

This patch unifies on the fuchsia.io struct version and updates all
callers. A high-level version of this struct is implemented in the C++
VFS for the convenience of the filesystems.

The fuchsia.fs version uses resource tables which are more forward-
compatible. But there are two reasons to use the struct version:

 - Currently the C++ bindings for resource tables in LLCPP is difficult
   to use and error-prone. All implementations and consumers of this
   function currently use LLCPP. This will hopefully be improved in the
   future with some planned FIDL bindings changes.

 - The vast majority of implementations and callers are for the
   fuchsia.io version. This version also has some binary compatibility
   guarantees that require it be changed in multiple steps over a period
   of time. Therefore, we could not do the opposite patch to unify on
   the resource table version without more steps in between.

As a result, this patch currently tries only to solve the problem of
there being more than one FilesystemInfo implementation, and we can
address any limitations of this existing struct as they come up.

Bug: 84558
Change-Id: I16dcbb25c614c4921073dba46d39111c52c64510
Reviewed-on: https://fuchsia-review.googlesource.com/c/fuchsia/+/606665
Reviewed-by: Adam Barth <abarth@google.com>
Reviewed-by: Chris Suter <csuter@google.com>
API-Review: Adam Barth <abarth@google.com>
Commit-Queue: Brett Wilson <brettw@google.com>
  • Loading branch information
brettw authored and CQ Bot committed Nov 17, 2021
1 parent 4b12aeb commit 6764513
Show file tree
Hide file tree
Showing 4 changed files with 68 additions and 52 deletions.
36 changes: 33 additions & 3 deletions src/lib/storage/vfs/cpp/fuchsia_vfs.cc
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,37 @@ uint32_t ToStreamOptions(const VnodeConnectionOptions& options) {

} // namespace

void FilesystemInfo::SetFsId(const zx::event& event) {
zx_info_handle_basic_t handle_info;
if (zx_status_t status =
event.get_info(ZX_INFO_HANDLE_BASIC, &handle_info, sizeof(handle_info), nullptr, nullptr);
status == ZX_OK) {
fs_id = handle_info.koid;
} else {
fs_id = ZX_KOID_INVALID;
}
}

fuchsia_io_admin::wire::FilesystemInfo FilesystemInfo::ToFidl() const {
fuchsia_io_admin::wire::FilesystemInfo out = {};

out.total_bytes = total_bytes;
out.used_bytes = used_bytes;
out.total_nodes = total_nodes;
out.used_nodes = used_nodes;
out.free_shared_pool_bytes = free_shared_pool_bytes;
out.fs_id = fs_id;
out.block_size = block_size;
out.max_filename_size = max_filename_size;
out.fs_type = fs_type;

ZX_DEBUG_ASSERT(name.size() < fuchsia_io_admin::wire::kMaxFsNameBuffer);
out.name[name.copy(reinterpret_cast<char*>(out.name.data()),
fuchsia_io_admin::wire::kMaxFsNameBuffer - 1)] = '\0';

return out;
}

constexpr FuchsiaVfs::MountNode::MountNode() = default;
FuchsiaVfs::MountNode::~MountNode() { ZX_DEBUG_ASSERT(vn_ == nullptr); }

Expand Down Expand Up @@ -208,9 +239,8 @@ zx_status_t FuchsiaVfs::Rename(zx::event token, fbl::RefPtr<Vnode> oldparent,
return ZX_OK;
}

zx_status_t FuchsiaVfs::GetFilesystemInfo(fidl::AnyArena& allocator,
fuchsia_fs::wire::FilesystemInfo& out) {
return ZX_ERR_NOT_SUPPORTED;
zx::status<FilesystemInfo> FuchsiaVfs::GetFilesystemInfo() {
return zx::error(ZX_ERR_NOT_SUPPORTED);
}

zx_status_t FuchsiaVfs::Link(zx::event token, fbl::RefPtr<Vnode> oldparent, std::string_view oldStr,
Expand Down
33 changes: 27 additions & 6 deletions src/lib/storage/vfs/cpp/fuchsia_vfs.h
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,29 @@ namespace internal {
class Connection;
} // namespace internal

// An internal version of fuchsia_io_admin::wire::FilesystemInfo with a simpler API and default
// initializers. See that FIDL struct for documentation.
struct FilesystemInfo {
uint64_t total_bytes = 0;
uint64_t used_bytes = 0;
uint64_t total_nodes = 0;
uint64_t used_nodes = 0;
uint64_t free_shared_pool_bytes = 0;
uint64_t fs_id = 0; // One of VFS_TYPE_*
uint32_t block_size = 0;
uint32_t max_filename_size = 0;
uint32_t fs_type = 0;
std::string name; // Length must be less than MAX_FS_NAME_BUFFER.

// To ensure global uniqueness, filesystems should create and maintain an event object. The koid
// of this object is guaranteed unique in the system and is used for the filesystem ID. This
// function extracts the koid of the given event object and sets it as the filesystem ID.
void SetFsId(const zx::event& event);

// Writes this object's values to the given FIDL object.
fuchsia_io_admin::wire::FilesystemInfo ToFidl() const;
};

// Vfs specialization that adds Fuchsia-specific
class FuchsiaVfs : public Vfs {
public:
Expand Down Expand Up @@ -61,12 +84,10 @@ class FuchsiaVfs : public Vfs {
zx_status_t Rename(zx::event token, fbl::RefPtr<Vnode> oldparent, std::string_view oldStr,
std::string_view newStr) __TA_EXCLUDES(vfs_lock_);

// Provides the implementation for fuchsia.fs.Query. This default implementation returns
// ZX_ERR_NOT_SUPPORTED. The implementation should use the given allocator when setting the
// values in the |out| struct.
virtual zx_status_t GetFilesystemInfo(fidl::AnyArena& allocator,
fuchsia_fs::wire::FilesystemInfo& out)
__TA_EXCLUDES(vfs_lock_);
// Provides the implementation for fuchsia.fs.Query.GetInfo() and
// fuchsia.io.admin.DirectoryAdmin.QueryFilesystem(). This default implementation returns
// ZX_ERR_NOT_SUPPORTED.
virtual zx::status<FilesystemInfo> GetFilesystemInfo() __TA_EXCLUDES(vfs_lock_);

async_dispatcher_t* dispatcher() const { return dispatcher_; }
void SetDispatcher(async_dispatcher_t* dispatcher);
Expand Down
9 changes: 4 additions & 5 deletions src/lib/storage/vfs/cpp/query_service.cc
Original file line number Diff line number Diff line change
Expand Up @@ -19,12 +19,11 @@ QueryService::QueryService(FuchsiaVfs* vfs)
vfs_(vfs) {}

void QueryService::GetInfo(GetInfoRequestView request, GetInfoCompleter::Sync& completer) {
fidl::Arena allocator;
fuchsia_fs::wire::FilesystemInfo out(allocator);
if (zx_status_t status = vfs_->GetFilesystemInfo(allocator, out); status != ZX_OK) {
completer.ReplyError(status);
zx::status<FilesystemInfo> info_or = vfs_->GetFilesystemInfo();
if (info_or.is_ok()) {
completer.ReplySuccess(info_or->ToFidl());
} else {
completer.ReplySuccess(std::move(out));
completer.ReplyError(info_or.error_value());
}
}

Expand Down
42 changes: 4 additions & 38 deletions src/lib/storage/vfs/cpp/vnode.cc
Original file line number Diff line number Diff line change
Expand Up @@ -305,45 +305,11 @@ zx_status_t Vnode::QueryFilesystem(fuchsia_io_admin::wire::FilesystemInfo* out)
if (!vfs_)
return ZX_ERR_NOT_SUPPORTED;

// TODO(fxbug.dev/84558) This should be unified with the fs.Query.FilesystemInfo to avoid
// this transformation. For now, allow implementation via the fs.Query version and convert to
// io.admin.
fidl::Arena allocator;
fuchsia_fs::wire::FilesystemInfo input(allocator);
if (zx_status_t status = vfs_->GetFilesystemInfo(allocator, input); status != ZX_OK)
return status;

if (input.has_block_size())
out->block_size = input.block_size();
if (input.has_max_node_name_size())
out->max_filename_size = input.max_node_name_size();
if (input.has_fs_type())
out->fs_type = static_cast<uint32_t>(input.fs_type());
if (input.has_total_bytes())
out->total_bytes = input.total_bytes();
if (input.has_used_bytes())
out->used_bytes = input.used_bytes();
if (input.has_total_nodes())
out->total_nodes = input.total_nodes();
if (input.has_used_nodes())
out->used_nodes = input.used_nodes();
if (input.has_free_shared_pool_bytes())
out->free_shared_pool_bytes = input.free_shared_pool_bytes();

if (input.has_fs_id()) {
zx_info_handle_basic_t handle_info;
if (zx_status_t status = input.fs_id().get_info(ZX_INFO_HANDLE_BASIC, &handle_info,
sizeof(handle_info), nullptr, nullptr);
status == ZX_OK) {
out->fs_id = handle_info.koid;
}
}

if (input.has_name()) {
out->name[input.name().get().copy(reinterpret_cast<char*>(out->name.data()),
fuchsia_io_admin::wire::kMaxFsNameBuffer - 1)] = '\0';
}
zx::status<FilesystemInfo> info_or = vfs_->GetFilesystemInfo();
if (info_or.is_error())
return info_or.error_value();

*out = info_or->ToFidl();
return ZX_OK;
}

Expand Down

0 comments on commit 6764513

Please sign in to comment.