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.

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::intmax_t>(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));