From 8a351b047f34e107c4e1d35e69b107e8dbe7a7bb Mon Sep 17 00:00:00 2001
From: Rye Mutt <rye@alchemyviewer.org>
Date: Wed, 3 Mar 2021 16:33:25 -0500
Subject: [PATCH] Large cleanup of LLFileSystem to remove error-prone api and
file path usage
---
indra/llcommon/llfile.cpp | 138 +++++++++++++++++++++++++
indra/llcommon/llfile.h | 14 ++-
indra/llfilesystem/llfilesystem.cpp | 135 ++++++++++++------------
indra/llfilesystem/llfilesystem.h | 9 +-
indra/newview/llfloateruipreview.cpp | 6 +-
indra/newview/llviewerassetstorage.cpp | 2 +-
6 files changed, 226 insertions(+), 78 deletions(-)
diff --git a/indra/llcommon/llfile.cpp b/indra/llcommon/llfile.cpp
index 974a7dd6d62..ff25f1fde5e 100644
--- a/indra/llcommon/llfile.cpp
+++ b/indra/llcommon/llfile.cpp
@@ -173,6 +173,70 @@ int warnif(const std::string& desc, const std::string& filename, int rc, int acc
return rc;
}
+int warnif(const std::string& desc, const boost::filesystem::path& filename, int rc, int accept = 0)
+{
+ if (rc < 0)
+ {
+ // Capture errno before we start emitting output
+ int errn = errno;
+ // For certain operations, a particular errno value might be
+ // acceptable -- e.g. stat() could permit ENOENT, mkdir() could permit
+ // EEXIST. Don't warn if caller explicitly says this errno is okay.
+ if (errn != accept)
+ {
+#if LL_WINDOWS
+ LL_WARNS("LLFile") << "Couldn't " << desc << " '" << ll_convert_wide_to_string(filename.native())
+ << "' (errno " << errn << "): " << strerr(errn) << LL_ENDL;
+#else
+ LL_WARNS("LLFile") << "Couldn't " << desc << " '" << filename.native()
+ << "' (errno " << errn << "): " << strerr(errn) << LL_ENDL;
+#endif
+ }
+#if 0 && LL_WINDOWS // turn on to debug file-locking problems
+ // If the problem is "Permission denied," maybe it's because another
+ // process has the file open. Try to find out.
+ if (errn == EACCES) // *not* EPERM
+ {
+ // Only do any of this stuff (before LL_ENDL) if it will be logged.
+ LL_DEBUGS("LLFile") << empty;
+ // would be nice to use LLDir for this, but dependency goes the
+ // wrong way
+ const char* TEMP = LLFile::tmpdir();
+ if (!(TEMP && *TEMP))
+ {
+ LL_CONT << "No $TEMP, not running 'handle'";
+ }
+ else
+ {
+ std::string tf(TEMP);
+ tf += "\\handle.tmp";
+ // http://technet.microsoft.com/en-us/sysinternals/bb896655
+ std::string cmd(STRINGIZE("handle \"" << filename
+ // "openfiles /query /v | fgrep -i \"" << filename
+ << "\" > \"" << tf << '"'));
+ LL_CONT << cmd;
+ if (system(cmd.c_str()) != 0)
+ {
+ LL_CONT << "\nDownload 'handle.exe' from http://technet.microsoft.com/en-us/sysinternals/bb896655";
+ }
+ else
+ {
+ std::ifstream inf(tf);
+ std::string line;
+ while (std::getline(inf, line))
+ {
+ LL_CONT << '\n' << line;
+ }
+ }
+ LLFile::remove(tf);
+ }
+ LL_CONT << LL_ENDL;
+ }
+#endif // LL_WINDOWS hack to identify processes holding file open
+ }
+ return rc;
+}
+
// static
int LLFile::mkdir(const std::string& dirname, int perms)
{
@@ -220,6 +284,16 @@ LLFILE* LLFile::fopen(const std::string& filename, const char* mode) /* Flawfind
#endif
}
+// static
+LLFILE* LLFile::fopen(const boost::filesystem::path& filename, MODE_T accessmode) /* Flawfinder: ignore */
+{
+#if LL_WINDOWS
+ return _wfopen(filename.c_str(), accessmode);
+#else
+ return ::fopen(filename.c_str(), accessmode); /* Flawfinder: ignore */
+#endif
+}
+
LLFILE* LLFile::_fsopen(const std::string& filename, const char* mode, int sharingFlag)
{
#if LL_WINDOWS
@@ -254,6 +328,16 @@ int LLFile::remove(const std::string& filename, int supress_error)
return warnif("remove", filename, rc, supress_error);
}
+int LLFile::remove(const boost::filesystem::path& filename, int supress_error)
+{
+#if LL_WINDOWS
+ int rc = _wremove(filename.c_str());
+#else
+ int rc = ::remove(filename.c_str());
+#endif
+ return warnif("remove", filename, rc, supress_error);
+}
+
int LLFile::rename(const std::string& filename, const std::string& newname, int supress_error)
{
#if LL_WINDOWS
@@ -297,6 +381,48 @@ int LLFile::rename(const std::string& filename, const std::string& newname, int
return warnif(STRINGIZE("rename to '" << newname << "' from"), filename, rc, supress_error);
}
+int LLFile::rename(const boost::filesystem::path& filename, const boost::filesystem::path& newname, int supress_error)
+{
+#if LL_WINDOWS
+ int rc = _wrename(filename.c_str(), newname.c_str());
+ return warnif(STRINGIZE("rename to '" << ll_convert_wide_to_string(newname.native()) << "' from"), filename, rc, supress_error);
+#else
+ int rc = ::rename(filename.c_str(), newname.c_str());
+ // Note: This workaround generalises the solution previously applied in llxfer_file.
+ // In doing this we add more failure modes to the operation, the copy can fail, the unlink can fail, in fact the copy can fail for multiple reasons.
+ // "A common mistake that people make when trying to design something completely foolproof is to underestimate the ingenuity of complete fools." - Douglas Adams, Mostly harmless
+ if (rc)
+ {
+ S32 error_number = errno;
+ LL_INFOS("LLFile") << "Rename failure (" << error_number << ") - " << filename << " to " << newname << LL_ENDL;
+ if (EXDEV == error_number)
+ {
+ if (copy(filename, newname) == true) // sigh in their wisdom LL decided that copy returns bool true not 0 whjen no error. using == true to make that clear.
+ {
+ LL_INFOS("LLFile") << "Rename across mounts not supported; copying+unlinking the file instead." << LL_ENDL;
+ rc = LLFile::remove(filename);
+ if (rc)
+ {
+ LL_WARNS("LLFile") << "unlink failed during copy/unlink workaround. Please check for stray file: " << filename << LL_ENDL;
+ }
+ }
+ else
+ {
+ LL_WARNS("LLFile") << "Copy failure during rename workaround for rename " << filename << " to " << newname << " (check both filenames and maunally rectify)" << LL_ENDL;
+ }
+ rc = 0; // We need to reset rc here to avoid the higher level function taking corrective action on what could be bad files.
+ }
+ else
+ {
+ LL_WARNS("LLFile") << "Rename fatally failed, no workaround attempted for errno="
+ << errno << "." << LL_ENDL;
+ // rc will propogate alllowing corrective action above. Not entirely happy with this but it is what already happens so we're not making it worse.
+ }
+ }
+ return warnif(STRINGIZE("rename to '" << newname << "' from"), filename, rc, supress_error);
+#endif
+}
+
bool LLFile::copy(const std::string& from, const std::string& to)
{
bool copied = false;
@@ -341,6 +467,18 @@ int LLFile::stat(const std::string& filename, llstat* filestatus)
return warnif("stat", filename, rc, ENOENT);
}
+int LLFile::stat(const boost::filesystem::path& filename, llstat* filestatus)
+{
+#if LL_WINDOWS
+ int rc = _wstat(filename.c_str(), filestatus);
+#else
+ int rc = ::stat(filename.c_str(), filestatus);
+#endif
+ // We use stat() to determine existence (see isfile(), isdir()).
+ // Don't spam the log if the subject pathname doesn't exist.
+ return warnif("stat", filename, rc, ENOENT);
+}
+
bool LLFile::isdir(const std::string& filename)
{
llstat st;
diff --git a/indra/llcommon/llfile.h b/indra/llcommon/llfile.h
index a2f3a12aca1..7bc56247314 100644
--- a/indra/llcommon/llfile.h
+++ b/indra/llcommon/llfile.h
@@ -58,11 +58,20 @@ typedef struct stat llstat;
#include "llstring.h" // safe char* -> std::string conversion
+#include <boost/filesystem.hpp>
+
+#if LL_WINDOWS
+#define MODE_T const wchar_t*
+#else
+#define MODE_T const char*
+#endif
+
class LL_COMMON_API LLFile
{
public:
// All these functions take UTF8 path/filenames.
static LLFILE* fopen(const std::string& filename,const char* accessmode); /* Flawfinder: ignore */
+ static LLFILE* fopen(const boost::filesystem::path& filename, MODE_T accessmode); /* Flawfinder: ignore */
static LLFILE* _fsopen(const std::string& filename,const char* accessmode,int sharingFlag);
static int close(LLFILE * file);
@@ -74,10 +83,13 @@ class LL_COMMON_API LLFile
static int rmdir(const std::string& filename);
static int remove(const std::string& filename, int supress_error = 0);
+ static int remove(const boost::filesystem::path& filename, int supress_error = 0);
static int rename(const std::string& filename,const std::string& newname, int supress_error = 0);
+ static int rename(const boost::filesystem::path& filename, const boost::filesystem::path& newname, int supress_error = 0);
static bool copy(const std::string& from, const std::string& to);
- static int stat(const std::string& filename,llstat* file_status);
+ static int stat(const std::string& filename, llstat* file_status);
+ static int stat(const boost::filesystem::path& filename, llstat* file_status);
static bool isdir(const std::string& filename);
static bool isfile(const std::string& filename);
static LLFILE * _Fiopen(const std::string& filename,
diff --git a/indra/llfilesystem/llfilesystem.cpp b/indra/llfilesystem/llfilesystem.cpp
index 25b335fc1b6..4bc3785a0dd 100644
--- a/indra/llfilesystem/llfilesystem.cpp
+++ b/indra/llfilesystem/llfilesystem.cpp
@@ -36,84 +36,52 @@
#include <boost/filesystem.hpp>
-static LLTrace::BlockTimerStatHandle FTM_VFILE_WAIT("VFile Wait");
+#ifndef TEXT
+#define TEXT(quote)
+#endif
LLFileSystem::LLFileSystem(const LLUUID& file_id, const LLAssetType::EType file_type, S32 mode)
+ : mFileID(file_id),
+ mFileType(file_type),
+ mPosition(0),
+ mMode(mode),
+ mBytesRead(0)
{
- mFileType = file_type;
- mFileID = file_id;
- mPosition = 0;
- mBytesRead = 0;
- mMode = mode;
+ const std::string filename = LLDiskCache::metaDataToFilepath(file_id, file_type);
+#if LL_WINDOWS
+ mFilePath = ll_convert_string_to_wide(filename);
+#else
+ mFilePath = filename;
+#endif
}
// static
bool LLFileSystem::getExists(const LLUUID& file_id, const LLAssetType::EType file_type)
{
- const std::string filename = LLDiskCache::metaDataToFilepath(file_id, file_type);
-
- llstat stat;
- if (LLFile::stat(filename, &stat) == 0)
- {
- return S_ISREG(stat.st_mode) && stat.st_size > 0;
- }
- return false;
+ LLFileSystem file(file_id, file_type, READ);
+ return file.exists();
}
// static
bool LLFileSystem::removeFile(const LLUUID& file_id, const LLAssetType::EType file_type)
{
- const std::string filename = LLDiskCache::metaDataToFilepath(file_id, file_type);
-
- LLFile::remove(filename, ENOENT);
-
- return true;
+ LLFileSystem file(file_id, file_type, READ_WRITE);
+ return file.remove();
}
// static
bool LLFileSystem::renameFile(const LLUUID& old_file_id, const LLAssetType::EType old_file_type,
const LLUUID& new_file_id, const LLAssetType::EType new_file_type)
{
- const std::string old_filename = LLDiskCache::metaDataToFilepath(old_file_id, old_file_type);
- const std::string new_filename = LLDiskCache::metaDataToFilepath(new_file_id, new_file_type);
-
- // Rename needs the new file to not exist.
- LLFile::remove(new_filename, ENOENT);
-
- if (LLFile::rename(old_filename, new_filename) != 0)
- {
- // We would like to return FALSE here indicating the operation
- // failed but the original code does not and doing so seems to
- // break a lot of things so we go with the flow...
- //return FALSE;
- LL_WARNS() << "Failed to rename " << old_file_id << " to " << new_file_id << " reason: " << strerror(errno) << LL_ENDL;
- }
-
- return TRUE;
-}
-
-// static
-S32 LLFileSystem::getFileSize(const LLUUID& file_id, const LLAssetType::EType file_type)
-{
- const std::string filename = LLDiskCache::metaDataToFilepath(file_id, file_type);
-
- S32 file_size = 0;
- llstat stat;
- if (LLFile::stat(filename, &stat) == 0)
- {
- file_size = stat.st_size;
- }
-
- return file_size;
+ LLFileSystem file(old_file_id, old_file_type, READ_WRITE);
+ return file.rename(new_file_id, new_file_type);
}
BOOL LLFileSystem::read(U8* buffer, S32 bytes)
{
BOOL success = TRUE;
- const std::string filename = LLDiskCache::metaDataToFilepath(mFileID, mFileType);
-
- LLUniqueFile filep = LLFile::fopen(filename, "rb");
+ LLUniqueFile filep = LLFile::fopen(mFilePath, TEXT("rb"));
if (filep)
{
fseek(filep, mPosition, SEEK_SET);
@@ -158,7 +126,7 @@ BOOL LLFileSystem::read(U8* buffer, S32 bytes)
// even though we are reading and not writing because this is the
// way the cache works - it relies on a valid "last accessed time" for
// each file so it knows how to remove the oldest, unused files
- updateFileAccessTime(filename);
+ updateFileAccessTime();
return success;
}
@@ -175,13 +143,11 @@ BOOL LLFileSystem::eof()
BOOL LLFileSystem::write(const U8* buffer, S32 bytes)
{
- const std::string filename = LLDiskCache::metaDataToFilepath(mFileID, mFileType);
-
BOOL success = FALSE;
if (mMode == APPEND)
{
- LLUniqueFile filep = LLFile::fopen(filename, "ab");
+ LLUniqueFile filep = LLFile::fopen(mFilePath, TEXT("ab"));
if (filep)
{
fwrite((const void*)buffer, bytes, 1, filep);
@@ -191,7 +157,7 @@ BOOL LLFileSystem::write(const U8* buffer, S32 bytes)
}
else
{
- LLUniqueFile filep = LLFile::fopen(filename, "wb");
+ LLUniqueFile filep = LLFile::fopen(mFilePath, TEXT("wb"));
if (filep)
{
fwrite((const void*)buffer, bytes, 1, filep);
@@ -242,7 +208,14 @@ S32 LLFileSystem::tell() const
S32 LLFileSystem::getSize()
{
- return LLFileSystem::getFileSize(mFileID, mFileType);
+ S32 file_size = 0;
+ llstat stat;
+ if (LLFile::stat(mFilePath.c_str(), &stat) == 0)
+ {
+ file_size = stat.st_size;
+ }
+
+ return file_size;
}
S32 LLFileSystem::getMaxSize()
@@ -253,22 +226,50 @@ S32 LLFileSystem::getMaxSize()
BOOL LLFileSystem::rename(const LLUUID& new_id, const LLAssetType::EType new_type)
{
- LLFileSystem::renameFile(mFileID, mFileType, new_id, new_type);
+#if LL_WINDOWS
+ boost::filesystem::path new_filename = ll_convert_string_to_wide(LLDiskCache::metaDataToFilepath(new_id, new_type));
+#else
+ boost::filesystem::path new_filename = LLDiskCache::metaDataToFilepath(new_id, new_type);
+#endif
+
+ // Rename needs the new file to not exist.
+ LLFile::remove(new_filename, ENOENT);
+
+ if (LLFile::rename(mFilePath, new_filename) != 0)
+ {
+ // We would like to return FALSE here indicating the operation
+ // failed but the original code does not and doing so seems to
+ // break a lot of things so we go with the flow...
+ //return FALSE;
+ LL_WARNS() << "Failed to rename " << mFileID << " to " << new_id << " reason: " << strerror(errno) << LL_ENDL;
+ }
mFileID = new_id;
mFileType = new_type;
+ mFilePath = std::move(new_filename);
+
return TRUE;
}
BOOL LLFileSystem::remove()
{
- LLFileSystem::removeFile(mFileID, mFileType);
+ LLFile::remove(mFilePath, ENOENT);
return TRUE;
}
-void LLFileSystem::updateFileAccessTime(const std::string& file_path)
+BOOL LLFileSystem::exists()
+{
+ llstat stat;
+ if (LLFile::stat(mFilePath, &stat) == 0)
+ {
+ return S_ISREG(stat.st_mode) && stat.st_size > 0;
+ }
+ return false;
+}
+
+void LLFileSystem::updateFileAccessTime()
{
/**
* Threshold in time_t units that is used to decide if the last access time
@@ -284,14 +285,8 @@ void LLFileSystem::updateFileAccessTime(const std::string& file_path)
// current time
const std::time_t cur_time = std::time(nullptr);
-#if LL_WINDOWS
- boost::filesystem::path path(ll_convert_string_to_wide(file_path));
-#else
- boost::filesystem::path path(file_path);
-#endif
-
// file last write time
- const std::time_t last_write_time = boost::filesystem::last_write_time(path);
+ const std::time_t last_write_time = boost::filesystem::last_write_time(mFilePath);
// delta between cur time and last time the file was written
const std::time_t delta_time = cur_time - last_write_time;
@@ -300,6 +295,6 @@ void LLFileSystem::updateFileAccessTime(const std::string& file_path)
// before the last one
if (delta_time > time_threshold)
{
- boost::filesystem::last_write_time(path, cur_time);
+ boost::filesystem::last_write_time(mFilePath, cur_time);
}
}
diff --git a/indra/llfilesystem/llfilesystem.h b/indra/llfilesystem/llfilesystem.h
index d4d5fa2a58a..63b46e8e759 100644
--- a/indra/llfilesystem/llfilesystem.h
+++ b/indra/llfilesystem/llfilesystem.h
@@ -34,6 +34,8 @@
#include "llassettype.h"
#include "lldiskcache.h"
+#include <boost/filesystem.hpp>
+
class LLFileSystem
{
public:
@@ -52,19 +54,19 @@ class LLFileSystem
S32 getMaxSize();
BOOL rename(const LLUUID& new_id, const LLAssetType::EType new_type);
BOOL remove();
+ BOOL exists();
/**
* Update the "last write time" of a file to "now". This must be called whenever a
* file in the cache is read (not written) so that the last time the file was
* accessed is up to date (This is used in the mechanism for purging the cache)
*/
- void updateFileAccessTime(const std::string& file_path);
+ void updateFileAccessTime();
static bool getExists(const LLUUID& file_id, const LLAssetType::EType file_type);
static bool removeFile(const LLUUID& file_id, const LLAssetType::EType file_type);
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);
public:
enum
@@ -76,8 +78,9 @@ class LLFileSystem
};
protected:
- LLAssetType::EType mFileType;
+ boost::filesystem::path mFilePath;
LLUUID mFileID;
+ LLAssetType::EType mFileType;
S32 mPosition;
S32 mMode;
S32 mBytesRead;
diff --git a/indra/newview/llfloateruipreview.cpp b/indra/newview/llfloateruipreview.cpp
index 03968415216..6029f6aba99 100644
--- a/indra/newview/llfloateruipreview.cpp
+++ b/indra/newview/llfloateruipreview.cpp
@@ -890,7 +890,7 @@ void LLFloaterUIPreview::displayFloater(BOOL click, S32 ID)
std::string full_path = getLocalizedDirectory() + path;
std::string floater_lang = "EN";
llstat dummy;
- if(!LLFile::stat(full_path.c_str(), &dummy)) // if the file does not exist
+ if(!LLFile::stat(full_path, &dummy)) // if the file does not exist
{
floater_lang = getLocStr(ID);
}
@@ -965,7 +965,7 @@ void LLFloaterUIPreview::onClickEditFloater()
// stat file to see if it exists (some localized versions may not have it there are no diffs, and then we try to open an nonexistent file)
llstat dummy;
- if(LLFile::stat(file_path.c_str(), &dummy)) // if the file does not exist
+ if(LLFile::stat(file_path, &dummy)) // if the file does not exist
{
popupAndPrintWarning("No file for this floater exists in the selected localization. Opening the EN version instead.");
file_path = get_xui_dir() + mDelim + "en" + mDelim + file_name; // open the en version instead, by default
@@ -1119,7 +1119,7 @@ void LLFloaterUIPreview::onClickToggleDiffHighlighting()
}
llstat dummy;
- if(LLFile::stat(path_in_textfield.c_str(), &dummy) && !error) // check if the file exists (empty check is reduntant but useful for the informative error message)
+ if(LLFile::stat(path_in_textfield, &dummy) && !error) // check if the file exists (empty check is reduntant but useful for the informative error message)
{
std::string warning = std::string("Unable to highlight differences because an invalid path to a difference file was provided:\"") + path_in_textfield + "\"";
popupAndPrintWarning(warning);
diff --git a/indra/newview/llviewerassetstorage.cpp b/indra/newview/llviewerassetstorage.cpp
index 5af87d07329..2542e857346 100644
--- a/indra/newview/llviewerassetstorage.cpp
+++ b/indra/newview/llviewerassetstorage.cpp
@@ -186,7 +186,7 @@ void LLViewerAssetStorage::storeAssetData(
else
{
// LLAssetStorage metric: Successful Request
- S32 size = LLFileSystem::getFileSize(asset_id, asset_type);
+ S32 size = vfile.getSize();
const char *message = "Added to upload queue";
reportMetric( asset_id, asset_type, LLStringUtil::null, LLUUID::null, size, MR_OKAY, __FILE__, __LINE__, message );
--
GitLab