From 2438854ea33d0a2f8d9fb13d2e3ef6f7d9bda235 Mon Sep 17 00:00:00 2001 From: Frederick Martian Date: Thu, 4 Dec 2025 00:10:42 +0100 Subject: Incorporate some of the comment improvements suggested by Copilot and make LLFile::size() return -1 on error and adjust the callers to account for that. --- indra/llaudio/llaudiodecodemgr.cpp | 2 +- indra/llcommon/llfile.cpp | 58 ++++++++++++++++++---------------- indra/llcommon/llfile.h | 10 +++--- indra/llfilesystem/llfilesystem.cpp | 19 +++++++---- indra/llfilesystem/llfilesystem.h | 4 +-- indra/llmessage/llassetstorage.cpp | 2 +- indra/newview/lltexturecache.cpp | 5 +-- indra/newview/llviewerassetstorage.cpp | 2 +- indra/test/llfile_tut.cpp | 1 - 9 files changed, 53 insertions(+), 50 deletions(-) diff --git a/indra/llaudio/llaudiodecodemgr.cpp b/indra/llaudio/llaudiodecodemgr.cpp index d8a6fffea6..232b429130 100755 --- a/indra/llaudio/llaudiodecodemgr.cpp +++ b/indra/llaudio/llaudiodecodemgr.cpp @@ -199,7 +199,7 @@ bool LLVorbisDecodeState::initDecode() LL_DEBUGS("AudioEngine") << "Initing decode from vfile: " << mUUID << LL_ENDL; mInFilep = new LLFileSystem(mUUID, LLAssetType::AT_SOUND); - if (!mInFilep || !mInFilep->getSize()) + if (!mInFilep || mInFilep->getSize() <= 0) { LL_WARNS("AudioEngine") << "unable to open vorbis source vfile for reading" << LL_ENDL; delete mInFilep; diff --git a/indra/llcommon/llfile.cpp b/indra/llcommon/llfile.cpp index 0751bde07c..3e469111fa 100755 --- a/indra/llcommon/llfile.cpp +++ b/indra/llcommon/llfile.cpp @@ -226,7 +226,7 @@ static void find_locking_process(const std::string& filename) } #endif // LL_WINDOWS hack to identify processes holding file open -static int warnif(const std::string& desc, const std::string& filename, int rc, int accept = 0) +static int warnif(const std::string& desc, const std::string& filename, int rc, int suppress_warning = 0) { if (rc < 0) { @@ -236,7 +236,7 @@ static int warnif(const std::string& desc, const std::string& filename, int rc, // For certain operations, a particular errno value might be // acceptable -- e.g. stat() could permit ENOENT, mkdir() could permit // EEXIST. Don't log a warning if caller explicitly says this errno is okay. - if (errn != accept) + if (errn != suppress_warning) { LL_WARNS("LLFile") << "Couldn't " << desc << " '" << filename << "' (errno " << errn << "): " << strerr(errn) << LL_ENDL; } @@ -252,12 +252,12 @@ static int warnif(const std::string& desc, const std::string& filename, int rc, return rc; } -static int warnif(const std::string& desc, const std::string& filename, std::error_code& ec, int accept = 0) +static int warnif(const std::string& desc, const std::string& filename, const std::error_code& ec, int suppress_warning = 0) { if (ec) { - // get Posix errno from the std::error_code so we can compare it to the accept parameter to see - // when a caller wants us to not generate a warning for a particular error code + // get Posix errno from the std::error_code so we can compare it to the suppress_warning parameter + // to see when a caller wants us to not generate a warning for a particular error code #if LL_WINDOWS int errn = get_errno_from_oserror(ec.value()); #else @@ -265,7 +265,7 @@ static int warnif(const std::string& desc, const std::string& filename, std::err #endif // For certain operations, a particular errno value might be acceptable // Don't warn if caller explicitly says this errno is okay. - if (errn != accept) + if (errn != suppress_warning) { LL_WARNS("LLFile") << "Couldn't " << desc << " '" << filename << "' (errno " << errn << "): " << ec.message() << LL_ENDL; } @@ -511,12 +511,13 @@ int LLFile::open(const std::string& filename, std::ios_base::openmode omode, std std::wstring file_path = utf8StringToWstring(filename); mHandle = CreateFileW(file_path.c_str(), access, share, nullptr, create, attributes, nullptr); + // The dwShareMode = share parameter takes care of locking the file for other processes if indicated, + // no need to do anything else for file locking here #else int oflags = decode_open_mode(omode); - int lmode = LLFile::noblock | (omode & LLFile::lock_mask); + int lmode = omode & LLFile::lock_mask; mHandle = ::open(filename.c_str(), oflags, perm); - if (mHandle != InvalidHandle && omode & LLFile::lock_mask && - lock(omode | LLFile::noblock, ec) != 0) + if (mHandle != InvalidHandle && lmode && lock(lmode | LLFile::noblock, ec) != 0) { close(); return -1; @@ -611,8 +612,7 @@ S64 LLFile::read(void* buffer, S64 nbytes, std::error_code& ec) if (nbytes == 0) { // Nothing to do - clear_error(ec); - return 0; + return clear_error(ec); } else if (!buffer || nbytes < 0) { @@ -652,8 +652,7 @@ S64 LLFile::write(const void* buffer, S64 nbytes, std::error_code& ec) if (nbytes == 0) { // Nothing to do here - clear_error(ec); - return 0; + return clear_error(ec); } else if (!buffer || nbytes < 0) { @@ -699,7 +698,7 @@ S64 LLFile::printf(const char* fmt, ...) va_start(args1, fmt); va_list args2; va_copy(args2, args1); - int length = vsnprintf(NULL, 0, fmt, args1); + int length = vsnprintf(nullptr, 0, fmt, args1); va_end(args1); if (length < 0) { @@ -806,7 +805,7 @@ LLFILE* LLFile::fopen(const std::string& filename, const char* mode, int lmode) // Rather fail on a sharing conflict than block if (flock(fileno(file), decode_lock_mode(lmode | LLFile::noblock))) { - LLFile::close(file); + ::fclose(file); file = nullptr; } } @@ -820,13 +819,7 @@ int LLFile::close(LLFILE* file) int ret_value = 0; if (file) { - // Read the current errno and restore it if it was not 0 - int errn = errno; ret_value = ::fclose(file); - if (errn) - { - errno = errn; - } } return ret_value; } @@ -874,22 +867,22 @@ int LLFile::mkdir(const std::string& dirname) } // static -int LLFile::remove(const std::string& filename, int suppress_error) +int LLFile::remove(const std::string& filename, int suppress_warning) { std::error_code ec; std::filesystem::path file_path = utf8StringToPath(filename); std::filesystem::remove(file_path, ec); - return warnif("remove", filename, ec, suppress_error); + return warnif("remove", filename, ec, suppress_warning); } // static -int LLFile::rename(const std::string& filename, const std::string& newname, int suppress_error) +int LLFile::rename(const std::string& filename, const std::string& newname, int suppress_warning) { std::error_code ec; std::filesystem::path file_path = utf8StringToPath(filename); std::filesystem::path new_path = utf8StringToPath(newname); std::filesystem::rename(file_path, new_path, ec); - return warnif(STRINGIZE("rename to '" << newname << "' from"), filename, ec, suppress_error); + return warnif(STRINGIZE("rename to '" << newname << "' from"), filename, ec, suppress_warning); } // static @@ -1064,8 +1057,12 @@ S64 LLFile::size(const std::string& filename, int suppress_warning) { std::error_code ec; std::filesystem::path file_path = utf8StringToPath(filename); - std::intmax_t size = (std::intmax_t)std::filesystem::file_size(file_path, ec); - return warnif("size", filename, ec, suppress_warning) ? 0 : size; + std::intmax_t size = static_cast(std::filesystem::file_size(file_path, ec)); + if (ec) + { + return warnif("size", filename, ec, suppress_warning); + } + return size; } // static @@ -1152,7 +1149,12 @@ std::wstring LLFile::utf8StringToWstring(const std::string& pathname) utf16string.assign(L"\\\\?\\").append(utf16path); /* remove trailing spaces and dots (yes, Windows really does that) */ - return utf16string.substr(0, utf16string.find_last_not_of(L" \t.")); + size_t last_valid = utf16string.find_last_not_of(L" \t."); + if (last_valid == std::wstring::npos) + { + return std::wstring(); + } + return utf16string.substr(0, last_valid + 1); } return utf16string; } diff --git a/indra/llcommon/llfile.h b/indra/llcommon/llfile.h index f48b0f5cad..87c8b80321 100755 --- a/indra/llcommon/llfile.h +++ b/indra/llcommon/llfile.h @@ -191,7 +191,7 @@ public: /// @name class member methods /// /// These methods provide read and write support as well as additional functionality to query the size of - /// the file, change the position of the the current file pointer or query it. + /// the file, change the position of the current file pointer or query it. /// /// Most of these functions take as one of their parameters a std::error_code object which can be used to /// determine in more detail what error occurred if required @@ -370,11 +370,11 @@ public: static std::time_t getModificationTime(const std::string& filename, int suppress_warning = 0); ///< @returns the modification time of the file or 0 on error - /// get the file or directory attributes for filename + /// get the std::filesystem::file_status for filename static std::filesystem::file_status getStatus(const std::string& filename, bool dontFollowSymLink = false, int suppress_warning = ENOENT); - ///< dontFollowSymLinks set to true returns the attributes of the symlink if it is one, rather than resolving it - /// we pass by default ENOENT in the optional 'suppress_warning' parameter to not spam the log with - /// warnings when the file or directory does not exist + ///< dontFollowSymLinks set to true returns the std::filesystem::file_status of the symlink if it + /// is one, rather than resolving it. We pass by default ENOENT in the optional 'suppress_warning' + /// parameter to not spam the log with warnings when the file or directory does not exist /// @returns a std::filesystem::file_status value that can be passed to the appropriate std::filesystem::exists() /// and other APIs accepting a file_status. diff --git a/indra/llfilesystem/llfilesystem.cpp b/indra/llfilesystem/llfilesystem.cpp index 087daf78be..df8b8de880 100755 --- a/indra/llfilesystem/llfilesystem.cpp +++ b/indra/llfilesystem/llfilesystem.cpp @@ -78,15 +78,21 @@ bool LLFileSystem::getExists(const LLUUID& file_id, const LLAssetType::EType fil const std::string filename = LLDiskCache::metaDataToFilepath(file_id, file_type); // not only test for existence but for the file to be not empty - return LLFile::size(filename) > 0; + S64 size = LLFile::size(filename); + if (size < 0) + { + LL_WARNS() << "Failed to get size for file '" << filename << "': " << strerror(errno) << LL_ENDL; + return false; + } + return size > 0; } // static -bool LLFileSystem::removeFile(const LLUUID& file_id, const LLAssetType::EType file_type, int suppress_error /*= 0*/) +bool LLFileSystem::removeFile(const LLUUID& file_id, const LLAssetType::EType file_type, int suppress_warning /*= 0*/) { const std::string filename = LLDiskCache::metaDataToFilepath(file_id, file_type); - LLFile::remove(filename.c_str(), suppress_error); + LLFile::remove(filename.c_str(), suppress_warning); return true; } @@ -111,11 +117,10 @@ bool LLFileSystem::renameFile(const LLUUID& old_file_id, const LLAssetType::ETyp } // static -S32 LLFileSystem::getFileSize(const LLUUID& file_id, const LLAssetType::EType file_type) +S64 LLFileSystem::getFileSize(const LLUUID& file_id, const LLAssetType::EType file_type) { const std::string filename = LLDiskCache::metaDataToFilepath(file_id, file_type); - S64 fileSize = LLFile::size(filename); - return (fileSize > 0) ? (S32)fileSize : 0; + return LLFile::size(filename); } bool LLFileSystem::read(U8* buffer, S32 bytes) @@ -256,7 +261,7 @@ S32 LLFileSystem::tell() const S32 LLFileSystem::getSize() const { - return LLFileSystem::getFileSize(mFileID, mFileType); + return (S32)LLFileSystem::getFileSize(mFileID, mFileType); } S32 LLFileSystem::getMaxSize() const diff --git a/indra/llfilesystem/llfilesystem.h b/indra/llfilesystem/llfilesystem.h index 10649b6920..7188683e7f 100644 --- a/indra/llfilesystem/llfilesystem.h +++ b/indra/llfilesystem/llfilesystem.h @@ -61,10 +61,10 @@ class LLFileSystem void updateFileAccessTime(const std::string& file_path); static bool getExists(const LLUUID& file_id, const LLAssetType::EType file_type); - static bool removeFile(const LLUUID& file_id, const LLAssetType::EType file_type, int suppress_error = 0); + static bool removeFile(const LLUUID& file_id, const LLAssetType::EType file_type, int suppress_warning = 0); static bool renameFile(const LLUUID& old_file_id, const LLAssetType::EType old_file_type, const LLUUID& new_file_id, const LLAssetType::EType new_file_type); - static S32 getFileSize(const LLUUID& file_id, const LLAssetType::EType file_type); + static S64 getFileSize(const LLUUID& file_id, const LLAssetType::EType file_type); public: static const S32 READ; diff --git a/indra/llmessage/llassetstorage.cpp b/indra/llmessage/llassetstorage.cpp index 10fd56a68e..34eeacb273 100644 --- a/indra/llmessage/llassetstorage.cpp +++ b/indra/llmessage/llassetstorage.cpp @@ -467,7 +467,7 @@ bool LLAssetStorage::findInCacheAndInvokeCallback(const LLUUID& uuid, LLAssetTyp else { LL_WARNS("AssetStorage") << "Asset vfile " << uuid << ":" << type - << " found in static cache with bad size " << file.getSize() << ", ignoring" << LL_ENDL; + << " found in static cache with bad size " << size << ", ignoring" << LL_ENDL; } } return false; diff --git a/indra/newview/lltexturecache.cpp b/indra/newview/lltexturecache.cpp index ad222f229f..a6d81816ce 100755 --- a/indra/newview/lltexturecache.cpp +++ b/indra/newview/lltexturecache.cpp @@ -181,7 +181,6 @@ bool LLTextureCacheLocalFileWorker::doRead() { LL_PROFILE_ZONE_SCOPED_CATEGORY_TEXTURE; S32 local_size = (S32)LLFile::size(mFileName); - if (local_size > 0 && mFileName.size() > 4) { mDataSize = local_size; // Only a complete file is valid @@ -444,7 +443,7 @@ bool LLTextureCacheRemoteWorker::doRead() std::string filename = mCache->getTextureFileName(mID); S32 filesize = (S32)LLFile::size(filename); - if (filesize && (filesize + TEXTURE_CACHE_ENTRY_SIZE) > mOffset) + if (filesize > 0 && (filesize + TEXTURE_CACHE_ENTRY_SIZE) > mOffset) { S32 max_datasize = TEXTURE_CACHE_ENTRY_SIZE + filesize - mOffset; mDataSize = llmin(max_datasize, mDataSize); @@ -934,8 +933,6 @@ const char* fast_cache_filename = "FastCache.cache"; void LLTextureCache::setDirNames(ELLPath location) { -// std::string delem = gDirUtilp->getDirDelimiter(); - mHeaderEntriesFileName = gDirUtilp->getExpandedFilename(location, textures_dirname, entries_filename); mHeaderDataFileName = gDirUtilp->getExpandedFilename(location, textures_dirname, cache_filename); mTexturesDirName = gDirUtilp->getExpandedFilename(location, textures_dirname); diff --git a/indra/newview/llviewerassetstorage.cpp b/indra/newview/llviewerassetstorage.cpp index 141f370ecb..e76d340eda 100644 --- a/indra/newview/llviewerassetstorage.cpp +++ b/indra/newview/llviewerassetstorage.cpp @@ -182,7 +182,7 @@ void LLViewerAssetStorage::storeAssetData( else { // LLAssetStorage metric: Successful Request - S32 size = LLFileSystem::getFileSize(asset_id, asset_type); + S32 size = (S32)LLFileSystem::getFileSize(asset_id, asset_type); const char *message = "Added to upload queue"; reportMetric( asset_id, asset_type, LLStringUtil::null, LLUUID::null, size, MR_OKAY, __FILE__, __LINE__, message ); diff --git a/indra/test/llfile_tut.cpp b/indra/test/llfile_tut.cpp index 9cd7fd335f..c3676cd8b3 100755 --- a/indra/test/llfile_tut.cpp +++ b/indra/test/llfile_tut.cpp @@ -84,7 +84,6 @@ namespace tut ensure("llfile_test1.dat should not yet exist", !LLFile::exists(testfile1.string())); const char* testdata = "testdata"; - std::time_t current = time(nullptr); S64 bytes = LLFile::write(testfile1.string(), testdata, 0, sizeof(testdata)); ensure("LLFile::write() did not write correctly", bytes == sizeof(testdata)); -- cgit v1.3