From 37f06821c8ac0819562c1f50d3f043127fc3626e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Vladim=C3=ADr=20Vondru=C5=A1?= Date: Mon, 6 Jan 2020 15:49:46 +0100 Subject: [PATCH] Utility: add better Directory::map(), rename original to mapWrite(). The original one doesn't preserve file contents, so it's practically write-only. Its original intended purpose was a "swap file" on the disk and should have been named mapWrite() right from the start. The new map() is like mapRead() but with read-write access and failing when a file doesn't exist. The original map() signature is still present, but marked as deprecated. --- doc/corrade-changelog.dox | 9 ++ src/Corrade/Utility/Directory.cpp | 107 ++++++++++++++++---- src/Corrade/Utility/Directory.h | 41 ++++++-- src/Corrade/Utility/Test/DirectoryTest.cpp | 111 ++++++++++++++++----- 4 files changed, 213 insertions(+), 55 deletions(-) diff --git a/doc/corrade-changelog.dox b/doc/corrade-changelog.dox index f3c63c4b3..568bd5354 100644 --- a/doc/corrade-changelog.dox +++ b/doc/corrade-changelog.dox @@ -77,6 +77,8 @@ namespace Corrade { - Added a @ref CORRADE_STD_IS_TRIVIALLY_TRAITS_SUPPORTED macro into @ref Corrade/Utility/TypeTraits.h denoting if @ref std::is_trivially_copyable is available in the standard library +- Added @ref Utility::Directory::map(const std::string&) that provides + read-write access to mapped files without truncating them. @subsection corrade-changelog-latest-changes Changes and improvements @@ -123,6 +125,13 @@ namespace Corrade { - Various fixes (see [mosra/corrade#80](https://github.com/mosra/corrade/pull/80)) +@subsection corrade-changelog-latest-deprecated Deprecated APIs + +- @cpp Utility::Directory::map(const std::string&, std::size_t) @ce is + deprecated and renamed to @ref Utility::Directory::mapWrite() as it doesn't + preserve original file contents and thus can't be used for read-write + access + @subsection corrade-changelog-latest-compatibility Potential compatibility breakages, removed APIs - @ref Containers::Array and @ref Containers::StaticArray was switched to diff --git a/src/Corrade/Utility/Directory.cpp b/src/Corrade/Utility/Directory.cpp index eb290bf34..3b05cdde7 100644 --- a/src/Corrade/Utility/Directory.cpp +++ b/src/Corrade/Utility/Directory.cpp @@ -759,34 +759,24 @@ void MapDeleter::operator()(const char* const data, const std::size_t size) { if(_fd) close(_fd); } -Containers::Array map(const std::string& filename, std::size_t size) { - /* Open the file for writing. Create if it doesn't exist, truncate it if it - does. */ - const int fd = open(filename.data(), O_RDWR|O_CREAT|O_TRUNC, mode_t(0600)); +Containers::Array map(const std::string& filename) { + /* Open the file for reading */ + const int fd = open(filename.data(), O_RDWR); if(fd == -1) { - Error() << "Utility::Directory::map(): can't open" << filename; - return nullptr; - } - - /* Resize the file to requested size by seeking one byte before */ - if(lseek(fd, size - 1, SEEK_SET) == -1) { - close(fd); - Error() << "Utility::Directory::map(): can't seek to resize the file"; + Error{} << "Utility::Directory::map(): can't open" << filename; return nullptr; } - /* And then writing a zero byte on that position */ - if(::write(fd, "", 1) != 1) { - close(fd); - Error() << "Utility::Directory::map(): can't write to resize the file"; - return nullptr; - } + /* Get file size */ + const off_t currentPos = lseek(fd, 0, SEEK_CUR); + const std::size_t size = lseek(fd, 0, SEEK_END); + lseek(fd, currentPos, SEEK_SET); /* Map the file */ char* data = reinterpret_cast(mmap(nullptr, size, PROT_READ|PROT_WRITE, MAP_SHARED, fd, 0)); if(data == MAP_FAILED) { close(fd); - Error() << "Utility::Directory::map(): can't map the file"; + Error{} << "Utility::Directory::map(): can't map the file"; return nullptr; } @@ -816,6 +806,40 @@ Containers::Array mapRead(const std::string& filename) { return Containers::Array{data, size, MapDeleter{fd}}; } + +Containers::Array mapWrite(const std::string& filename, std::size_t size) { + /* Open the file for writing. Create if it doesn't exist, truncate it if it + does. */ + const int fd = open(filename.data(), O_RDWR|O_CREAT|O_TRUNC, mode_t(0600)); + if(fd == -1) { + Error{} << "Utility::Directory::mapWrite(): can't open" << filename; + return nullptr; + } + + /* Resize the file to requested size by seeking one byte before */ + if(lseek(fd, size - 1, SEEK_SET) == -1) { + close(fd); + Error{} << "Utility::Directory::mapWrite(): can't seek to resize the file"; + return nullptr; + } + + /* And then writing a zero byte on that position */ + if(::write(fd, "", 1) != 1) { + close(fd); + Error{} << "Utility::Directory::mapWrite(): can't write to resize the file"; + return nullptr; + } + + /* Map the file */ + char* data = reinterpret_cast(mmap(nullptr, size, PROT_READ|PROT_WRITE, MAP_SHARED, fd, 0)); + if(data == MAP_FAILED) { + close(fd); + Error{} << "Utility::Directory::mapWrite(): can't map the file"; + return nullptr; + } + + return Containers::Array{data, size, MapDeleter{fd}}; +} #elif defined(CORRADE_TARGET_WINDOWS) && !defined(CORRADE_TARGET_WINDOWS_RT) void MapDeleter::operator()(const char* const data, const std::size_t) { if(data) UnmapViewOfFile(data); @@ -823,24 +847,27 @@ void MapDeleter::operator()(const char* const data, const std::size_t) { if(_hFile) CloseHandle(_hFile); } -Containers::Array map(const std::string& filename, std::size_t size) { +Containers::Array map(const std::string& filename) { /* Open the file for writing. Create if it doesn't exist, truncate it if it does. */ HANDLE hFile = CreateFileW(widen(filename).data(), - GENERIC_READ|GENERIC_WRITE, FILE_SHARE_READ|FILE_SHARE_WRITE, nullptr, CREATE_ALWAYS, 0, nullptr); + GENERIC_READ|GENERIC_WRITE, FILE_SHARE_READ|FILE_SHARE_WRITE, nullptr, OPEN_EXISTING, 0, nullptr); if (hFile == INVALID_HANDLE_VALUE) { Error() << "Utility::Directory::map(): can't open" << filename; return nullptr; } /* Create the file mapping */ - HANDLE hMap = CreateFileMappingW(hFile, nullptr, PAGE_READWRITE, 0, size, nullptr); + HANDLE hMap = CreateFileMappingW(hFile, nullptr, PAGE_READWRITE, 0, 0, nullptr); if (!hMap) { Error() << "Utility::Directory::map(): can't create the file mapping:" << GetLastError(); CloseHandle(hFile); return nullptr; } + /* Get file size */ + const size_t size = GetFileSize(hFile, nullptr); + /* Map the file */ char* data = reinterpret_cast(::MapViewOfFile(hMap, FILE_MAP_ALL_ACCESS, 0, 0, 0)); if(!data) { @@ -884,6 +911,42 @@ Containers::Array mapRead(const std::string& filename) { return Containers::Array{data, size, MapDeleter{hFile, hMap}}; } + +Containers::Array mapWrite(const std::string& filename, std::size_t size) { + /* Open the file for writing. Create if it doesn't exist, truncate it if it + does. */ + HANDLE hFile = CreateFileW(widen(filename).data(), + GENERIC_READ|GENERIC_WRITE, FILE_SHARE_READ|FILE_SHARE_WRITE, nullptr, CREATE_ALWAYS, 0, nullptr); + if (hFile == INVALID_HANDLE_VALUE) { + Error() << "Utility::Directory::mapWrite(): can't open" << filename; + return nullptr; + } + + /* Create the file mapping */ + HANDLE hMap = CreateFileMappingW(hFile, nullptr, PAGE_READWRITE, 0, size, nullptr); + if (!hMap) { + Error() << "Utility::Directory::mapWrite(): can't create the file mapping:" << GetLastError(); + CloseHandle(hFile); + return nullptr; + } + + /* Map the file */ + char* data = reinterpret_cast(::MapViewOfFile(hMap, FILE_MAP_ALL_ACCESS, 0, 0, 0)); + if(!data) { + Error() << "Utility::Directory::mapWrite(): can't map the file:" << GetLastError(); + CloseHandle(hMap); + CloseHandle(hFile); + return nullptr; + } + + return Containers::Array{data, size, MapDeleter{hFile, hMap}}; +} +#endif + +#if defined(CORRADE_BUILD_DEPRECATED) && (defined(CORRADE_TARGET_UNIX) || (defined(CORRADE_TARGET_WINDOWS) && !defined(CORRADE_TARGET_WINDOWS_RT))) +Containers::Array map(const std::string& filename, const std::size_t size) { + return mapWrite(filename, size); +} #endif }}} diff --git a/src/Corrade/Utility/Directory.h b/src/Corrade/Utility/Directory.h index 9cd3867e2..bcbf157f1 100644 --- a/src/Corrade/Utility/Directory.h +++ b/src/Corrade/Utility/Directory.h @@ -483,17 +483,17 @@ CORRADE_UTILITY_EXPORT bool copy(const std::string& from, const std::string& to) #if defined(DOXYGEN_GENERATING_OUTPUT) || defined(CORRADE_TARGET_UNIX) || (defined(CORRADE_TARGET_WINDOWS) && !defined(CORRADE_TARGET_WINDOWS_RT)) /** @brief Map file for reading and writing +@m_since_latest -Maps the file as read-write memory and enlarges it to @p size. If the file does -not exist yet, it is created, if it exists, it's truncated. The array deleter -takes care of unmapping, however the file is not deleted after unmapping. If an -error occurs, @cpp nullptr @ce is returned and a message is printed to -@ref Error. Expects that the filename is in UTF-8. -@see @ref mapRead(), @ref read(), @ref write() +Maps the file as read-write memory. The array deleter takes care of unmapping. +If the file doesn't exist or an error occurs while mapping, @cpp nullptr @ce is +returned and a message is printed to @ref Error. Expects that the filename is +in UTF-8. +@see @ref mapRead(), @ref mapWrite(), @ref read(), @ref write() @partialsupport Available only on @ref CORRADE_TARGET_UNIX "Unix" and non-RT @ref CORRADE_TARGET_WINDOWS "Windows" platforms. - */ -CORRADE_UTILITY_EXPORT Containers::Array map(const std::string& filename, std::size_t size); +*/ +CORRADE_UTILITY_EXPORT Containers::Array map(const std::string& filename); /** @brief Map file for reading @@ -502,11 +502,34 @@ Maps the file as read-only memory. The array deleter takes care of unmapping. If the file doesn't exist or an error occurs while mapping, @cpp nullptr @ce is returned and a message is printed to @ref Error. Expects that the filename is in UTF-8. -@see @ref map(), @ref read() +@see @ref map(), @ref mapWrite(), @ref read() @partialsupport Available only on @ref CORRADE_TARGET_UNIX "Unix" and non-RT @ref CORRADE_TARGET_WINDOWS "Windows" platforms. */ CORRADE_UTILITY_EXPORT Containers::Array mapRead(const std::string& filename); + +/** +@brief Map file for writing +@m_since_latest + +Maps the file as read-write memory and enlarges it to @p size. If the file does +not exist yet, it is created, if it exists, it's truncated --- thus no data +is preserved. The array deleter takes care of unmapping, however the file is +not deleted after unmapping. If an error occurs, @cpp nullptr @ce is returned +and a message is printed to @ref Error. Expects that the filename is in UTF-8. +@see @ref map(), @ref mapRead(), @ref read(), @ref write() +@partialsupport Available only on @ref CORRADE_TARGET_UNIX "Unix" and non-RT + @ref CORRADE_TARGET_WINDOWS "Windows" platforms. +*/ +CORRADE_UTILITY_EXPORT Containers::Array mapWrite(const std::string& filename, std::size_t size); + +#ifdef CORRADE_BUILD_DEPRECATED +/** + * @copybrief mapWrite() + * @m_deprecated_since_latest Use @ref mapWrite() instead. + */ +CORRADE_DEPRECATED("use mapWrite() instead") CORRADE_UTILITY_EXPORT Containers::Array map(const std::string& filename, std::size_t size); +#endif #endif #ifndef DOXYGEN_GENERATING_OUTPUT diff --git a/src/Corrade/Utility/Test/DirectoryTest.cpp b/src/Corrade/Utility/Test/DirectoryTest.cpp index 0cfe22b18..1d4cee648 100644 --- a/src/Corrade/Utility/Test/DirectoryTest.cpp +++ b/src/Corrade/Utility/Test/DirectoryTest.cpp @@ -143,13 +143,17 @@ struct DirectoryTest: TestSuite::Tester { #endif void map(); - void mapNoPermission(); + void mapNonexistent(); void mapUtf8(); void mapRead(); void mapReadNonexistent(); void mapReadUtf8(); + void mapWrite(); + void mapWriteNoPermission(); + void mapWriteUtf8(); + std::string _testDir, _testDirUtf8, _writeTestDir; @@ -257,12 +261,16 @@ DirectoryTest::DirectoryTest() { #endif addTests({&DirectoryTest::map, - &DirectoryTest::mapNoPermission, + &DirectoryTest::mapNonexistent, &DirectoryTest::mapUtf8, &DirectoryTest::mapRead, &DirectoryTest::mapReadNonexistent, - &DirectoryTest::mapReadUtf8}); + &DirectoryTest::mapReadUtf8, + + &DirectoryTest::mapWrite, + &DirectoryTest::mapWriteNoPermission, + &DirectoryTest::mapWriteUtf8}); #ifdef CORRADE_TARGET_APPLE if(Directory::isSandboxed() @@ -1256,31 +1264,39 @@ void DirectoryTest::copy100MMap() { void DirectoryTest::map() { #if defined(CORRADE_TARGET_UNIX) || (defined(CORRADE_TARGET_WINDOWS) && !defined(CORRADE_TARGET_WINDOWS_RT)) std::string data{"\xCA\xFE\xBA\xBE\x0D\x0A\x00\xDE\xAD\xBE\xEF", 11}; + std::string file = Directory::join(_writeTestDir, "mappedFile"); + if(Directory::exists(file)) CORRADE_VERIFY(Directory::rm(file)); + Directory::writeString(file, data); + { - auto mappedFile = Directory::map(Directory::join(_writeTestDir, "mappedFile"), data.size()); - CORRADE_VERIFY(mappedFile); - CORRADE_COMPARE(mappedFile.size(), data.size()); - std::copy(std::begin(data), std::end(data), mappedFile.begin()); + auto mappedFile = Directory::map(file); + CORRADE_COMPARE_AS(Containers::arrayView(mappedFile), + Containers::arrayView({'\xCA', '\xFE', '\xBA', '\xBE', '\x0D', '\x0A', '\x00', '\xDE', '\xAD', '\xBE', '\xEF'}), + TestSuite::Compare::Container); + + /* Write a thing there */ + mappedFile[2] = '\xCA'; + mappedFile[3] = '\xFE'; + + /* Implicit unmap */ } - CORRADE_COMPARE_AS(Directory::join(_writeTestDir, "mappedFile"), - data, + + /* The file should be changed */ + CORRADE_COMPARE_AS(file, + (std::string{"\xCA\xFE\xCA\xFE\x0D\x0A\x00\xDE\xAD\xBE\xEF", 11}), TestSuite::Compare::FileToString); #else CORRADE_SKIP("Not implemented on this platform."); #endif } -void DirectoryTest::mapNoPermission() { +void DirectoryTest::mapNonexistent() { #if defined(CORRADE_TARGET_UNIX) || (defined(CORRADE_TARGET_WINDOWS) && !defined(CORRADE_TARGET_WINDOWS_RT)) - if(Directory::home() == "/root") - CORRADE_SKIP("Running under root, can't test for permissions."); - { std::ostringstream out; Error err{&out}; - auto mappedFile = Directory::map("/root/mappedFile", 64); - CORRADE_VERIFY(!mappedFile); - CORRADE_COMPARE(out.str(), "Utility::Directory::map(): can't open /root/mappedFile\n"); + CORRADE_VERIFY(!Directory::map("nonexistent")); + CORRADE_COMPARE(out.str(), "Utility::Directory::map(): can't open nonexistent\n"); } #else CORRADE_SKIP("Not implemented on this platform."); @@ -1289,16 +1305,12 @@ void DirectoryTest::mapNoPermission() { void DirectoryTest::mapUtf8() { #if defined(CORRADE_TARGET_UNIX) || (defined(CORRADE_TARGET_WINDOWS) && !defined(CORRADE_TARGET_WINDOWS_RT)) - std::string data{"\xCA\xFE\xBA\xBE\x0D\x0A\x00\xDE\xAD\xBE\xEF", 11}; { - auto mappedFile = Directory::map(Directory::join(_writeTestDir, "hýždě chlípníka"), data.size()); - CORRADE_VERIFY(mappedFile); - CORRADE_COMPARE(mappedFile.size(), data.size()); - std::copy(std::begin(data), std::end(data), mappedFile.begin()); + const auto mappedFile = Directory::map(Directory::join(_testDirUtf8, "hýždě")); + CORRADE_COMPARE_AS(Containers::arrayView(mappedFile), + Containers::arrayView({'\xCA', '\xFE', '\xBA', '\xBE', '\x0D', '\x0A', '\x00', '\xDE', '\xAD', '\xBE', '\xEF'}), + TestSuite::Compare::Container); } - CORRADE_COMPARE_AS(Directory::join(_writeTestDir, "hýždě chlípníka"), - data, - TestSuite::Compare::FileToString); #else CORRADE_SKIP("Not implemented on this platform."); #endif @@ -1345,6 +1357,57 @@ void DirectoryTest::mapReadUtf8() { #endif } +void DirectoryTest::mapWrite() { + #if defined(CORRADE_TARGET_UNIX) || (defined(CORRADE_TARGET_WINDOWS) && !defined(CORRADE_TARGET_WINDOWS_RT)) + std::string data{"\xCA\xFE\xBA\xBE\x0D\x0A\x00\xDE\xAD\xBE\xEF", 11}; + { + auto mappedFile = Directory::mapWrite(Directory::join(_writeTestDir, "mappedWriteFile"), data.size()); + CORRADE_VERIFY(mappedFile); + CORRADE_COMPARE(mappedFile.size(), data.size()); + std::copy(std::begin(data), std::end(data), mappedFile.begin()); + } + CORRADE_COMPARE_AS(Directory::join(_writeTestDir, "mappedWriteFile"), + data, + TestSuite::Compare::FileToString); + #else + CORRADE_SKIP("Not implemented on this platform."); + #endif +} + +void DirectoryTest::mapWriteNoPermission() { + #if defined(CORRADE_TARGET_UNIX) || (defined(CORRADE_TARGET_WINDOWS) && !defined(CORRADE_TARGET_WINDOWS_RT)) + if(Directory::home() == "/root") + CORRADE_SKIP("Running under root, can't test for permissions."); + + { + std::ostringstream out; + Error err{&out}; + auto mappedFile = Directory::mapWrite("/root/mappedFile", 64); + CORRADE_VERIFY(!mappedFile); + CORRADE_COMPARE(out.str(), "Utility::Directory::mapWrite(): can't open /root/mappedFile\n"); + } + #else + CORRADE_SKIP("Not implemented on this platform."); + #endif +} + +void DirectoryTest::mapWriteUtf8() { + #if defined(CORRADE_TARGET_UNIX) || (defined(CORRADE_TARGET_WINDOWS) && !defined(CORRADE_TARGET_WINDOWS_RT)) + std::string data{"\xCA\xFE\xBA\xBE\x0D\x0A\x00\xDE\xAD\xBE\xEF", 11}; + { + auto mappedFile = Directory::mapWrite(Directory::join(_writeTestDir, "hýždě chlípníka"), data.size()); + CORRADE_VERIFY(mappedFile); + CORRADE_COMPARE(mappedFile.size(), data.size()); + std::copy(std::begin(data), std::end(data), mappedFile.begin()); + } + CORRADE_COMPARE_AS(Directory::join(_writeTestDir, "hýždě chlípníka"), + data, + TestSuite::Compare::FileToString); + #else + CORRADE_SKIP("Not implemented on this platform."); + #endif +} + }}}} CORRADE_TEST_MAIN(Corrade::Utility::Test::DirectoryTest)