From 45647df34929e440e512979c8901d28889e66f30 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?L=C3=A9o=20Lam?= Date: Sat, 14 Apr 2018 00:19:44 +0200 Subject: [PATCH 1/2] UnitTests: Add tests for the Wii filesystem This adds unit tests for the Wii filesystem now that the filesystem interface is neatly separated from the IPC code. Basic FS functionality is tested, in addition to problematic usages and edge cases that Dolphin used to handle incorrectly (which of course broke emulated software). These tests should make it quite a bit harder to introduce regressions. Issues that are covered by the tests in particular: * Metadata: issue 10234 (though tests are commented out for now because Dolphin doesn't support NAND images yet so it can't track metadata); * EOF seeks/reads: https://github.com/dolphin-emu/dolphin/pull/4942 * Read/write operations from multiple handles: see issue 2917, 5232 and 8702 and https://github.com/dolphin-emu/dolphin/pull/2649 --- Source/UnitTests/Core/CMakeLists.txt | 2 + .../UnitTests/Core/IOS/FS/FileSystemTest.cpp | 291 ++++++++++++++++++ 2 files changed, 293 insertions(+) create mode 100644 Source/UnitTests/Core/IOS/FS/FileSystemTest.cpp diff --git a/Source/UnitTests/Core/CMakeLists.txt b/Source/UnitTests/Core/CMakeLists.txt index ba8c228e88..621cfd3f61 100644 --- a/Source/UnitTests/Core/CMakeLists.txt +++ b/Source/UnitTests/Core/CMakeLists.txt @@ -11,3 +11,5 @@ add_dolphin_test(DSPAssemblyTest ) add_dolphin_test(ESFormatsTest IOS/ES/FormatsTest.cpp IOS/ES/TestBinaryData.cpp) + +add_dolphin_test(FileSystemTest IOS/FS/FileSystemTest.cpp) diff --git a/Source/UnitTests/Core/IOS/FS/FileSystemTest.cpp b/Source/UnitTests/Core/IOS/FS/FileSystemTest.cpp new file mode 100644 index 0000000000..cc6dd25b91 --- /dev/null +++ b/Source/UnitTests/Core/IOS/FS/FileSystemTest.cpp @@ -0,0 +1,291 @@ +// Copyright 2018 Dolphin Emulator Project +// Licensed under GPLv2+ +// Refer to the license.txt file included. + +#include +#include +#include + +#include + +#include "Common/CommonTypes.h" +#include "Common/FileUtil.h" +#include "Core/IOS/FS/FileSystem.h" +#include "Core/IOS/IOS.h" +#include "UICommon/UICommon.h" + +using namespace IOS::HLE::FS; + +class FileSystemTest : public testing::Test +{ +protected: + FileSystemTest() : m_profile_path{File::CreateTempDir()} + { + UICommon::SetUserDirectory(m_profile_path); + m_fs = IOS::HLE::Kernel{}.GetFS(); + } + + virtual ~FileSystemTest() + { + m_fs.reset(); + File::DeleteDirRecursively(m_profile_path); + } + + std::shared_ptr m_fs; + +private: + std::string m_profile_path; +}; + +TEST_F(FileSystemTest, EssentialDirectories) +{ + for (const std::string& path : + {"/sys", "/ticket", "/title", "/shared1", "/shared2", "/tmp", "/import", "/meta"}) + { + EXPECT_TRUE(m_fs->ReadDirectory(Uid{0}, Gid{0}, path).Succeeded()) << path; + } +} + +TEST_F(FileSystemTest, CreateFile) +{ + const std::string PATH = "/tmp/f"; + + ASSERT_EQ(m_fs->CreateFile(Uid{0}, Gid{0}, PATH, 0, Mode::Read, Mode::None, Mode::None), + ResultCode::Success); + + const Result stats = m_fs->GetMetadata(Uid{0}, Gid{0}, PATH); + ASSERT_TRUE(stats.Succeeded()); + EXPECT_TRUE(stats->is_file); + EXPECT_EQ(stats->size, 0u); + // TODO: After we start saving metadata correctly, check the UID, GID, permissions + // as well (issue 10234). + + ASSERT_EQ(m_fs->CreateFile(Uid{0}, Gid{0}, PATH, 0, Mode::Read, Mode::None, Mode::None), + ResultCode::AlreadyExists); + + const Result> tmp_files = m_fs->ReadDirectory(Uid{0}, Gid{0}, "/tmp"); + ASSERT_TRUE(tmp_files.Succeeded()); + EXPECT_EQ(std::count(tmp_files->begin(), tmp_files->end(), "f"), 1u); +} + +TEST_F(FileSystemTest, CreateDirectory) +{ + const std::string PATH = "/tmp/d"; + + ASSERT_EQ(m_fs->CreateDirectory(Uid{0}, Gid{0}, PATH, 0, Mode::Read, Mode::None, Mode::None), + ResultCode::Success); + + const Result stats = m_fs->GetMetadata(Uid{0}, Gid{0}, PATH); + ASSERT_TRUE(stats.Succeeded()); + EXPECT_FALSE(stats->is_file); + // TODO: After we start saving metadata correctly, check the UID, GID, permissions + // as well (issue 10234). + + const Result> children = m_fs->ReadDirectory(Uid{0}, Gid{0}, PATH); + ASSERT_TRUE(children.Succeeded()); + EXPECT_TRUE(children->empty()); + + // TODO: uncomment this after the FS code is fixed to return AlreadyExists. + // EXPECT_EQ(m_fs->CreateDirectory(Uid{0}, Gid{0}, PATH, 0, Mode::Read, Mode::None, Mode::None), + // ResultCode::AlreadyExists); +} + +TEST_F(FileSystemTest, Delete) +{ + EXPECT_TRUE(m_fs->ReadDirectory(Uid{0}, Gid{0}, "/tmp").Succeeded()); + EXPECT_EQ(m_fs->Delete(Uid{0}, Gid{0}, "/tmp"), ResultCode::Success); + EXPECT_EQ(m_fs->ReadDirectory(Uid{0}, Gid{0}, "/tmp").Error(), ResultCode::NotFound); +} + +TEST_F(FileSystemTest, Rename) +{ + EXPECT_TRUE(m_fs->ReadDirectory(Uid{0}, Gid{0}, "/tmp").Succeeded()); + + EXPECT_EQ(m_fs->Rename(Uid{0}, Gid{0}, "/tmp", "/test"), ResultCode::Success); + + EXPECT_EQ(m_fs->ReadDirectory(Uid{0}, Gid{0}, "/tmp").Error(), ResultCode::NotFound); + EXPECT_TRUE(m_fs->ReadDirectory(Uid{0}, Gid{0}, "/test").Succeeded()); +} + +TEST_F(FileSystemTest, GetDirectoryStats) +{ + auto check_stats = [this](u32 clusters, u32 inodes) { + const Result stats = m_fs->GetDirectoryStats("/tmp"); + ASSERT_TRUE(stats.Succeeded()); + EXPECT_EQ(stats->used_clusters, clusters); + EXPECT_EQ(stats->used_inodes, inodes); + }; + + check_stats(0u, 1u); + + EXPECT_EQ(m_fs->CreateFile(Uid{0}, Gid{0}, "/tmp/file", 0, Mode::Read, Mode::None, Mode::None), + ResultCode::Success); + // Still no clusters (because the file is empty), but 2 inodes now. + check_stats(0u, 2u); + + { + const Result file = m_fs->OpenFile(Uid{0}, Gid{0}, "/tmp/file", Mode::Write); + file->Write(std::vector(20).data(), 20); + } + // The file should now take up one cluster. + // TODO: uncomment after the FS code is fixed. + // check_stats(1u, 2u); +} + +// Files need to be explicitly created using CreateFile or CreateDirectory. +// Automatically creating them on first use would be a bug. +TEST_F(FileSystemTest, NonExistingFiles) +{ + const Result metadata = m_fs->GetMetadata(Uid{0}, Gid{0}, "/tmp/foo"); + ASSERT_FALSE(metadata.Succeeded()); + EXPECT_EQ(metadata.Error(), ResultCode::NotFound); + + const Result file = m_fs->OpenFile(Uid{0}, Gid{0}, "/tmp/foo", Mode::Read); + ASSERT_FALSE(file.Succeeded()); + EXPECT_EQ(file.Error(), ResultCode::NotFound); + + const Result> children = m_fs->ReadDirectory(Uid{0}, Gid{0}, "/foo"); + ASSERT_FALSE(children.Succeeded()); + EXPECT_EQ(children.Error(), ResultCode::NotFound); +} + +TEST_F(FileSystemTest, Seek) +{ + const std::vector TEST_DATA(10); + + ASSERT_EQ(m_fs->CreateFile(Uid{0}, Gid{0}, "/tmp/f", 0, Mode::ReadWrite, Mode::None, Mode::None), + ResultCode::Success); + + const Result file = m_fs->OpenFile(Uid{0}, Gid{0}, "/tmp/f", Mode::ReadWrite); + ASSERT_TRUE(file.Succeeded()); + + // An empty file should have a size of exactly 0 bytes. + EXPECT_EQ(file->GetStatus()->size, 0u); + // The file position should be set to the start right after an open. + EXPECT_EQ(file->GetStatus()->offset, 0u); + + // Write some dummy data. + ASSERT_TRUE(file->Write(TEST_DATA.data(), TEST_DATA.size()).Succeeded()); + EXPECT_EQ(file->GetStatus()->size, TEST_DATA.size()); + EXPECT_EQ(file->GetStatus()->offset, TEST_DATA.size()); + + auto seek_and_check = [&file](u32 offset, SeekMode mode, u32 expected_position) { + const Result new_offset = file->Seek(offset, mode); + ASSERT_TRUE(new_offset.Succeeded()); + EXPECT_EQ(*new_offset, expected_position); + EXPECT_EQ(file->GetStatus()->offset, expected_position); + }; + + seek_and_check(0, SeekMode::Set, 0); + seek_and_check(5, SeekMode::Set, 5); + seek_and_check(0, SeekMode::Current, 5); + seek_and_check(2, SeekMode::Current, 7); + seek_and_check(0, SeekMode::End, 10); + + // Test past-EOF seeks. + const Result new_position = file->Seek(11, SeekMode::Set); + ASSERT_FALSE(new_position.Succeeded()); + EXPECT_EQ(new_position.Error(), ResultCode::Invalid); +} + +TEST_F(FileSystemTest, WriteAndSimpleReadback) +{ + const std::vector TEST_DATA{{0, 1, 2, 3, 4, 5, 6, 7, 8, 9}}; + std::vector read_buffer(TEST_DATA.size()); + + ASSERT_EQ(m_fs->CreateFile(Uid{0}, Gid{0}, "/tmp/f", 0, Mode::ReadWrite, Mode::None, Mode::None), + ResultCode::Success); + + const Result file = m_fs->OpenFile(Uid{0}, Gid{0}, "/tmp/f", Mode::ReadWrite); + ASSERT_TRUE(file.Succeeded()); + + // Write some test data. + ASSERT_TRUE(file->Write(TEST_DATA.data(), TEST_DATA.size()).Succeeded()); + + // Now read it back and make sure it is identical. + ASSERT_TRUE(file->Seek(0, SeekMode::Set).Succeeded()); + ASSERT_TRUE(file->Read(read_buffer.data(), read_buffer.size()).Succeeded()); + EXPECT_EQ(TEST_DATA, read_buffer); +} + +TEST_F(FileSystemTest, WriteAndRead) +{ + const std::vector TEST_DATA{{0xf, 1, 2, 3, 4, 5, 6, 7, 8, 9}}; + std::vector buffer(TEST_DATA.size()); + + ASSERT_EQ(m_fs->CreateFile(Uid{0}, Gid{0}, "/tmp/f", 0, Mode::ReadWrite, Mode::None, Mode::None), + ResultCode::Success); + + Result tmp_handle = m_fs->OpenFile(Uid{0}, Gid{0}, "/tmp/f", Mode::ReadWrite); + ASSERT_TRUE(tmp_handle.Succeeded()); + const Fd fd = tmp_handle->Release(); + + // Try to read from an empty file. This should do nothing. + // See https://github.com/dolphin-emu/dolphin/pull/4942 + Result read_result = m_fs->ReadBytesFromFile(fd, buffer.data(), buffer.size()); + EXPECT_TRUE(read_result.Succeeded()); + EXPECT_EQ(*read_result, 0u); + EXPECT_EQ(m_fs->GetFileStatus(fd)->offset, 0u); + + ASSERT_TRUE(m_fs->WriteBytesToFile(fd, TEST_DATA.data(), TEST_DATA.size()).Succeeded()); + EXPECT_EQ(m_fs->GetFileStatus(fd)->offset, TEST_DATA.size()); + + // Try to read past EOF while we are at the end of the file. This should do nothing too. + read_result = m_fs->ReadBytesFromFile(fd, buffer.data(), buffer.size()); + EXPECT_TRUE(read_result.Succeeded()); + EXPECT_EQ(*read_result, 0u); + EXPECT_EQ(m_fs->GetFileStatus(fd)->offset, TEST_DATA.size()); + + // Go back to the start and try to read past EOF. This should read the entire file until EOF. + ASSERT_TRUE(m_fs->SeekFile(fd, 0, SeekMode::Set).Succeeded()); + std::vector larger_buffer(TEST_DATA.size() + 10); + read_result = m_fs->ReadBytesFromFile(fd, larger_buffer.data(), larger_buffer.size()); + EXPECT_TRUE(read_result.Succeeded()); + EXPECT_EQ(*read_result, TEST_DATA.size()); + EXPECT_EQ(m_fs->GetFileStatus(fd)->offset, TEST_DATA.size()); +} + +TEST_F(FileSystemTest, MultipleHandles) +{ + ASSERT_EQ(m_fs->CreateFile(Uid{0}, Gid{0}, "/tmp/f", 0, Mode::ReadWrite, Mode::None, Mode::None), + ResultCode::Success); + + { + const Result file = m_fs->OpenFile(Uid{0}, Gid{0}, "/tmp/f", Mode::ReadWrite); + ASSERT_TRUE(file.Succeeded()); + // Fill it with 10 zeroes. + ASSERT_TRUE(file->Write(std::vector(10).data(), 10).Succeeded()); + } + + const Result file1 = m_fs->OpenFile(Uid{0}, Gid{0}, "/tmp/f", Mode::ReadWrite); + const Result file2 = m_fs->OpenFile(Uid{0}, Gid{0}, "/tmp/f", Mode::ReadWrite); + ASSERT_TRUE(file1.Succeeded()); + ASSERT_TRUE(file2.Succeeded()); + + // Write some test data using one handle and make sure the data is seen by the other handle + // (see issue 2917, 5232 and 8702 and https://github.com/dolphin-emu/dolphin/pull/2649). + // Also make sure the file offsets are independent for each handle. + + const std::vector TEST_DATA{{0, 1, 2, 3, 4, 5, 6, 7, 8, 9}}; + EXPECT_EQ(file1->GetStatus()->offset, 0u); + ASSERT_TRUE(file1->Write(TEST_DATA.data(), TEST_DATA.size()).Succeeded()); + EXPECT_EQ(file1->GetStatus()->offset, 10u); + + std::vector read_buffer(TEST_DATA.size()); + EXPECT_EQ(file2->GetStatus()->offset, 0u); + ASSERT_TRUE(file2->Read(read_buffer.data(), read_buffer.size()).Succeeded()); + EXPECT_EQ(file2->GetStatus()->offset, 10u); + EXPECT_EQ(TEST_DATA, read_buffer); +} + +// ReadDirectory is used by official titles to determine whether a path is a file. +// If it is not a file, ResultCode::Invalid must be returned. +TEST_F(FileSystemTest, ReadDirectoryOnFile) +{ + ASSERT_EQ(m_fs->CreateFile(Uid{0}, Gid{0}, "/tmp/f", 0, Mode::Read, Mode::None, Mode::None), + ResultCode::Success); + + const Result> result = m_fs->ReadDirectory(Uid{0}, Gid{0}, "/tmp/f"); + ASSERT_FALSE(result.Succeeded()); + EXPECT_EQ(result.Error(), ResultCode::Invalid); +} From 3355ddcfb34bb634797f06be2d4193050c60d55a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?L=C3=A9o=20Lam?= Date: Sun, 15 Apr 2018 11:31:12 +0200 Subject: [PATCH 2/2] FileSystem: Fix member destruction order --- Source/Core/Core/IOS/FS/HostBackend/FS.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Source/Core/Core/IOS/FS/HostBackend/FS.h b/Source/Core/Core/IOS/FS/HostBackend/FS.h index b30e1742f6..162a0f8320 100644 --- a/Source/Core/Core/IOS/FS/HostBackend/FS.h +++ b/Source/Core/Core/IOS/FS/HostBackend/FS.h @@ -77,8 +77,8 @@ private: std::shared_ptr OpenHostFile(const std::string& host_path); std::string m_root_path; - std::array m_handles{}; std::map> m_open_files; + std::array m_handles{}; }; } // namespace IOS::HLE::FS