From 0639a32d3a27b9a89b82d915f58cfd59a634bb44 Mon Sep 17 00:00:00 2001 From: Ben Boeckel Date: Fri, 1 Dec 2023 22:36:00 -0500 Subject: cmFileTimes: return status codes from APIs This avoids accidentally overwriting the global error state before fetching the intended error code. --- Source/cmFileTimes.cxx | 42 ++++++++++++++++++++++++++++-------------- Source/cmFileTimes.h | 9 ++++++--- 2 files changed, 34 insertions(+), 17 deletions(-) diff --git a/Source/cmFileTimes.cxx b/Source/cmFileTimes.cxx index 0aaab1d..6835e46 100644 --- a/Source/cmFileTimes.cxx +++ b/Source/cmFileTimes.cxx @@ -6,6 +6,8 @@ #include +#include "cmsys/Status.hxx" + #include "cm_sys_stat.h" #if defined(_WIN32) @@ -13,6 +15,8 @@ # include "cmSystemTools.h" #else +# include + # include #endif @@ -66,7 +70,7 @@ cmFileTimes::cmFileTimes(std::string const& fileName) } cmFileTimes::~cmFileTimes() = default; -bool cmFileTimes::Load(std::string const& fileName) +cmsys::Status cmFileTimes::Load(std::string const& fileName) { std::unique_ptr ptr; if (this->IsValid()) { @@ -82,29 +86,29 @@ bool cmFileTimes::Load(std::string const& fileName) GENERIC_READ, FILE_SHARE_READ, 0, OPEN_EXISTING, FILE_FLAG_BACKUP_SEMANTICS, 0); if (!handle) { - return false; + return cmsys::Status::Windows_GetLastError(); } if (!GetFileTime(handle, &ptr->timeCreation, &ptr->timeLastAccess, &ptr->timeLastWrite)) { - return false; + return cmsys::Status::Windows_GetLastError(); } #else struct stat st; if (stat(fileName.c_str(), &st) < 0) { - return false; + return cmsys::Status::POSIX_errno(); } ptr->timeBuf.actime = st.st_atime; ptr->timeBuf.modtime = st.st_mtime; #endif // Accept times this->times = std::move(ptr); - return true; + return cmsys::Status::Success(); } -bool cmFileTimes::Store(std::string const& fileName) const +cmsys::Status cmFileTimes::Store(std::string const& fileName) const { if (!this->IsValid()) { - return false; + return cmsys::Status::POSIX(EINVAL); } #if defined(_WIN32) && !defined(__CYGWIN__) @@ -112,18 +116,28 @@ bool cmFileTimes::Store(std::string const& fileName) const cmSystemTools::ConvertToWindowsExtendedPath(fileName).c_str(), FILE_WRITE_ATTRIBUTES, 0, 0, OPEN_EXISTING, FILE_FLAG_BACKUP_SEMANTICS, 0); if (!handle) { - return false; + return cmsys::Status::Windows_GetLastError(); + } + if (SetFileTime(handle, &this->times->timeCreation, + &this->times->timeLastAccess, + &this->times->timeLastWrite) == 0) { + return cmsys::Status::Windows_GetLastError(); } - return SetFileTime(handle, &this->times->timeCreation, - &this->times->timeLastAccess, - &this->times->timeLastWrite) != 0; #else - return utime(fileName.c_str(), &this->times->timeBuf) >= 0; + if (utime(fileName.c_str(), &this->times->timeBuf) < 0) { + return cmsys::Status::POSIX_errno(); + } #endif + return cmsys::Status::Success(); } -bool cmFileTimes::Copy(std::string const& fromFile, std::string const& toFile) +cmsys::Status cmFileTimes::Copy(std::string const& fromFile, + std::string const& toFile) { cmFileTimes fileTimes; - return (fileTimes.Load(fromFile) && fileTimes.Store(toFile)); + cmsys::Status load_status = fileTimes.Load(fromFile); + if (!load_status) { + return load_status; + } + return fileTimes.Store(toFile); } diff --git a/Source/cmFileTimes.h b/Source/cmFileTimes.h index 50d64fd..58d24bc 100644 --- a/Source/cmFileTimes.h +++ b/Source/cmFileTimes.h @@ -7,6 +7,8 @@ #include #include +#include "cmsys/Status.hxx" + /** \class cmFileTimes * \brief Loads and stores file times. */ @@ -21,12 +23,13 @@ public: //! @return true, if file times were loaded successfully bool IsValid() const { return (this->times != nullptr); } //! Try to load the file times from @a fileName and @return IsValid() - bool Load(std::string const& fileName); + cmsys::Status Load(std::string const& fileName); //! Stores the file times at @a fileName (if IsValid()) - bool Store(std::string const& fileName) const; + cmsys::Status Store(std::string const& fileName) const; //! Copies the file times of @a fromFile to @a toFile - static bool Copy(std::string const& fromFile, std::string const& toFile); + static cmsys::Status Copy(std::string const& fromFile, + std::string const& toFile); private: #ifdef _WIN32 -- cgit v0.12 From 5cf7018af6a5f5f0c84c9b7d5b31d9f849503886 Mon Sep 17 00:00:00 2001 From: Ben Boeckel Date: Fri, 1 Dec 2023 22:18:20 -0500 Subject: cmFileCopier: remember error statuses and get their strings The last error may have changed between the original call and the `GetLastSystemError()` call. Remember the status explicitly and ask it for its error string. Reported on Discourse: https://discourse.cmake.org/t/9539 --- Source/cmFileCopier.cxx | 37 +++++++++++++++++++++++-------------- 1 file changed, 23 insertions(+), 14 deletions(-) diff --git a/Source/cmFileCopier.cxx b/Source/cmFileCopier.cxx index 663bddc..d6e91fa 100644 --- a/Source/cmFileCopier.cxx +++ b/Source/cmFileCopier.cxx @@ -86,10 +86,11 @@ bool cmFileCopier::SetPermissions(const std::string& toFile, } #endif - if (!cmSystemTools::SetPermissions(toFile, permissions)) { + auto perm_status = cmSystemTools::SetPermissions(toFile, permissions); + if (!perm_status) { std::ostringstream e; e << this->Name << " cannot set permissions on \"" << toFile - << "\": " << cmSystemTools::GetLastSystemError() << "."; + << "\": " << perm_status.GetString() << "."; this->Status.SetError(e.str()); return false; } @@ -530,11 +531,13 @@ bool cmFileCopier::InstallSymlink(const std::string& fromFile, { // Read the original symlink. std::string symlinkTarget; - if (!cmSystemTools::ReadSymlink(fromFile, symlinkTarget)) { + auto read_symlink_status = + cmSystemTools::ReadSymlink(fromFile, symlinkTarget); + if (!read_symlink_status) { std::ostringstream e; e << this->Name << " cannot read symlink \"" << fromFile << "\" to duplicate at \"" << toFile - << "\": " << cmSystemTools::GetLastSystemError() << "."; + << "\": " << read_symlink_status.GetString() << "."; this->Status.SetError(e.str()); return false; } @@ -604,12 +607,15 @@ bool cmFileCopier::InstallFile(const std::string& fromFile, this->ReportCopy(toFile, TypeFile, copy); // Copy the file. - if (copy && !cmSystemTools::CopyAFile(fromFile, toFile, true)) { - std::ostringstream e; - e << this->Name << " cannot copy file \"" << fromFile << "\" to \"" - << toFile << "\": " << cmSystemTools::GetLastSystemError() << "."; - this->Status.SetError(e.str()); - return false; + if (copy) { + auto copy_status = cmSystemTools::CopyAFile(fromFile, toFile, true); + if (!copy_status) { + std::ostringstream e; + e << this->Name << " cannot copy file \"" << fromFile << "\" to \"" + << toFile << "\": " << copy_status.GetString() << "."; + this->Status.SetError(e.str()); + return false; + } } // Set the file modification time of the destination file. @@ -620,10 +626,11 @@ bool cmFileCopier::InstallFile(const std::string& fromFile, if (cmSystemTools::GetPermissions(toFile, perm)) { cmSystemTools::SetPermissions(toFile, perm | mode_owner_write); } - if (!cmFileTimes::Copy(fromFile, toFile)) { + auto copy_status = cmFileTimes::Copy(fromFile, toFile); + if (!copy_status) { std::ostringstream e; e << this->Name << " cannot set modification time on \"" << toFile - << "\": " << cmSystemTools::GetLastSystemError() << "."; + << "\": " << copy_status.GetString() << "."; this->Status.SetError(e.str()); return false; } @@ -660,10 +667,12 @@ bool cmFileCopier::InstallDirectory(const std::string& source, } // Make sure the destination directory exists. - if (!cmSystemTools::MakeDirectory(destination, default_dir_mode)) { + auto makedir_status = + cmSystemTools::MakeDirectory(destination, default_dir_mode); + if (!makedir_status) { std::ostringstream e; e << this->Name << " cannot make directory \"" << destination - << "\": " << cmSystemTools::GetLastSystemError() << "."; + << "\": " << makedir_status.GetString() << "."; this->Status.SetError(e.str()); return false; } -- cgit v0.12 From a820877d033069062f2cac83d9e611d5af905d0a Mon Sep 17 00:00:00 2001 From: Ben Boeckel Date: Fri, 1 Dec 2023 22:45:18 -0500 Subject: errors: avoid constructing a stream before getting the last error Constructing a stream may involve operations that change the global error state. Avoid the streams by using `cmStrCat` instead. --- Source/cmCoreTryCompile.cxx | 12 +++++------- Source/cmExportCommand.cxx | 12 +++++------- Source/cmFileCommand.cxx | 11 +++++------ Source/cmFileCopier.cxx | 7 +++---- Source/cmake.cxx | 6 ++---- 5 files changed, 20 insertions(+), 28 deletions(-) diff --git a/Source/cmCoreTryCompile.cxx b/Source/cmCoreTryCompile.cxx index 7e19812..8c84ace 100644 --- a/Source/cmCoreTryCompile.cxx +++ b/Source/cmCoreTryCompile.cxx @@ -626,13 +626,11 @@ cm::optional cmCoreTryCompile::TryCompileCode( // now create a CMakeLists.txt file in that directory FILE* fout = cmsys::SystemTools::Fopen(outFileName, "w"); if (!fout) { - std::ostringstream e; - /* clang-format off */ - e << "Failed to open\n" - " " << outFileName << "\n" - << cmSystemTools::GetLastSystemError(); - /* clang-format on */ - this->Makefile->IssueMessage(MessageType::FATAL_ERROR, e.str()); + this->Makefile->IssueMessage( + MessageType::FATAL_ERROR, + cmStrCat("Failed to open\n" + " ", + outFileName, '\n', cmSystemTools::GetLastSystemError())); return cm::nullopt; } diff --git a/Source/cmExportCommand.cxx b/Source/cmExportCommand.cxx index e78b869..7e44210 100644 --- a/Source/cmExportCommand.cxx +++ b/Source/cmExportCommand.cxx @@ -385,13 +385,11 @@ static void StorePackageRegistry(cmMakefile& mf, std::string const& package, if (entry) { entry << content << "\n"; } else { - std::ostringstream e; - /* clang-format off */ - e << "Cannot create package registry file:\n" - << " " << fname << "\n" - << cmSystemTools::GetLastSystemError() << "\n"; - /* clang-format on */ - mf.IssueMessage(MessageType::WARNING, e.str()); + mf.IssueMessage(MessageType::WARNING, + cmStrCat("Cannot create package registry file:\n" + " ", + fname, '\n', + cmSystemTools::GetLastSystemError(), '\n')); } } } diff --git a/Source/cmFileCommand.cxx b/Source/cmFileCommand.cxx index 93bed9a..c65136c 100644 --- a/Source/cmFileCommand.cxx +++ b/Source/cmFileCommand.cxx @@ -3022,16 +3022,15 @@ bool HandleCreateLinkCommand(std::vector const& args, // Check if the new file already exists and remove it. if (cmSystemTools::PathExists(newFileName) && !cmSystemTools::RemoveFile(newFileName)) { - std::ostringstream e; - e << "Failed to create link '" << newFileName - << "' because existing path cannot be removed: " - << cmSystemTools::GetLastSystemError() << "\n"; + auto err = cmStrCat("Failed to create link '", newFileName, + "' because existing path cannot be removed: ", + cmSystemTools::GetLastSystemError(), '\n'); if (!arguments.Result.empty()) { - status.GetMakefile().AddDefinition(arguments.Result, e.str()); + status.GetMakefile().AddDefinition(arguments.Result, err); return true; } - status.SetError(e.str()); + status.SetError(err); return false; } diff --git a/Source/cmFileCopier.cxx b/Source/cmFileCopier.cxx index d6e91fa..686162b 100644 --- a/Source/cmFileCopier.cxx +++ b/Source/cmFileCopier.cxx @@ -119,10 +119,9 @@ std::string const& cmFileCopier::ToName(std::string const& fromName) bool cmFileCopier::ReportMissing(const std::string& fromFile) { // The input file does not exist and installation is not optional. - std::ostringstream e; - e << this->Name << " cannot find \"" << fromFile - << "\": " << cmSystemTools::GetLastSystemError() << "."; - this->Status.SetError(e.str()); + this->Status.SetError(cmStrCat(this->Name, " cannot find \"", fromFile, + "\": ", cmSystemTools::GetLastSystemError(), + '.')); return false; } diff --git a/Source/cmake.cxx b/Source/cmake.cxx index 942c59b..f54196b 100644 --- a/Source/cmake.cxx +++ b/Source/cmake.cxx @@ -1707,10 +1707,8 @@ void cmake::SetTraceFile(const std::string& file) this->TraceFile.close(); this->TraceFile.open(file.c_str()); if (!this->TraceFile) { - std::stringstream ss; - ss << "Error opening trace file " << file << ": " - << cmSystemTools::GetLastSystemError(); - cmSystemTools::Error(ss.str()); + cmSystemTools::Error(cmStrCat("Error opening trace file ", file, ": ", + cmSystemTools::GetLastSystemError())); return; } std::cout << "Trace will be written to " << file << '\n'; -- cgit v0.12