-
Notifications
You must be signed in to change notification settings - Fork 380
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
'Mass file loader' for use in skin and asset loading #6727
Closed
Closed
Changes from all commits
Commits
Show all changes
28 commits
Select commit
Hold shift + click to select a range
096897c
initial file loader
ewancg e961fb8
flesh out description, minor fixes
ewancg 5021136
small fixes
ewancg 646d526
that's not why
ewancg 76db8df
move all error checking to load step, remove fs_is_readable
ewancg babd456
change std fs symlink detection to lstat, std fs api not on macos <10.15
ewancg 74d1152
struct stat not stat
ewancg b88e8ea
hopefully satisfy clang-format and clang-tidy
ewancg b7e89d7
hopefully satisfy clang-format
ewancg 3d47803
fix header guard
ewancg e8e1ed7
hopefully satisfy clang-format
ewancg 9b5aac1
turn not init error into runtime assert
ewancg 21fef42
break dbg_assert at file loader impl load into multiple checks
ewancg 5114f76
basic async impl, move from /game/client to /engine/shared, fixes as …
ewancg 1617774
forgot to add that
ewancg 0c65fd5
hopefully please dev warnings
ewancg 3c4a970
add flag for skip bom
ewancg 98334ce
merge implementations, get rid of the pseudo-interface nonsense, ebb …
ewancg 1a44575
remove symlink functionality
ewancg 3276b1c
use job instead of raw thread
ewancg eb0a28f
cleanup
ewancg d7cb3ba
add lockfree thread communication
ewancg e795af7
remove a few more std string
ewancg 18d21ce
cleanup
ewancg a071edd
cleanup
ewancg 2e2bfca
fix crash on shutdown where cskins dtor expects valid ptr from Graphi…
ewancg ab44dbc
wtf
ewancg 2d8c571
maybe smarter mechanism for handling job state
ewancg File filter
Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,234 @@ | ||
#include "file_loader.h" | ||
#include <base/system.h> | ||
|
||
CMassFileLoader::CMassFileLoader(IEngine *pEngine, IStorage *pStorage, uint8_t Flags) | ||
{ | ||
m_pEngine = pEngine; | ||
m_pStorage = pStorage; | ||
m_Flags = Flags; | ||
} | ||
|
||
CMassFileLoader::~CMassFileLoader() | ||
{ | ||
if(m_pExtension) | ||
free(m_pExtension); | ||
for(const auto &it : m_PathCollection) | ||
{ | ||
delete it.second; | ||
} | ||
} | ||
|
||
void CMassFileLoader::SetFileExtension(const std::string_view Extension) | ||
{ | ||
m_pExtension = static_cast<char *>(malloc((Extension.length()) + 1 * sizeof(char))); | ||
str_format(m_pExtension, Extension.length() + 1, "%s", Extension.data()); | ||
} | ||
|
||
inline bool CMassFileLoader::CompareExtension(const std::filesystem::path &Filename, const std::string_view Extension) | ||
{ | ||
// std::string is justified here because of std::transform, and because std::filesystem::path::c_str() will | ||
// return const wchar_t *, but char width is handled automatically when using string | ||
std::string FileExtension = Filename.extension().string(); | ||
std::transform(FileExtension.begin(), FileExtension.end(), FileExtension.begin(), | ||
[](unsigned char c) { return std::tolower(c); }); | ||
return FileExtension == Extension; // Extension is already lowered | ||
} | ||
|
||
[[maybe_unused]] int CMassFileLoader::ListDirectoryCallback(const char *Name, int IsDir, int, void *User) | ||
{ | ||
auto *pUserData = reinterpret_cast<SListDirectoryCallbackUserInfo *>(User); | ||
if(pUserData->m_pThis->m_Continue) | ||
{ | ||
auto *pFileList = pUserData->m_pThis->m_PathCollection.find(pUserData->m_pCurrentDirectory)->second; | ||
char AbsolutePath[IO_MAX_PATH_LENGTH]; | ||
str_format(AbsolutePath, sizeof(AbsolutePath), "%s/%s", pUserData->m_pCurrentDirectory, Name); | ||
|
||
if(!str_comp(Name, ".") || !str_comp(Name, "..")) | ||
return 0; | ||
|
||
if(!IsDir) | ||
{ | ||
if((pUserData->m_pThis->m_pExtension == nullptr || str_comp(pUserData->m_pThis->m_pExtension, "")) || CompareExtension(Name, pUserData->m_pThis->m_pExtension)) | ||
pFileList->push_back(Name); | ||
} | ||
else if(pUserData->m_pThis->m_Flags & LOAD_FLAGS_RECURSE_SUBDIRECTORIES) | ||
{ | ||
// Note that adding data to a SORTED container that is currently being iterated on higher in | ||
// scope would invalidate the iterator. This is not sorted | ||
pUserData->m_pThis->m_PathCollection.insert({AbsolutePath, new std::vector<std::string>}); | ||
SListDirectoryCallbackUserInfo Data{AbsolutePath, pUserData->m_pThis}; | ||
|
||
// Directory item is a directory, must be recursed | ||
pUserData->m_pThis->m_pStorage->ListDirectory(IStorage::TYPE_ALL, AbsolutePath, ListDirectoryCallback, &Data); | ||
} | ||
} | ||
|
||
return 0; | ||
} | ||
|
||
unsigned int CMassFileLoader::Begin(CMassFileLoader *pUserData) | ||
{ | ||
char aPathBuffer[IO_MAX_PATH_LENGTH]; | ||
for(auto &It : pUserData->m_RequestedPaths) | ||
{ | ||
if(pUserData->m_Continue) | ||
{ | ||
int StorageType = It.find(':') == 0 ? IStorage::STORAGETYPE_BASIC : IStorage::STORAGETYPE_CLIENT; | ||
if(StorageType == IStorage::STORAGETYPE_BASIC) | ||
It.erase(0, 1); | ||
pUserData->m_pStorage->GetCompletePath(StorageType, It.c_str(), aPathBuffer, sizeof(aPathBuffer)); | ||
if(fs_is_dir(aPathBuffer)) // Exists and is a directory | ||
pUserData->m_PathCollection.insert({std::string(aPathBuffer), new std::vector<std::string>}); | ||
else | ||
pUserData->m_Continue = TryCallback(pUserData->m_fnLoadFailedCallback, LOAD_ERROR_INVALID_SEARCH_PATH, It.c_str(), pUserData->m_pUser); | ||
} | ||
} | ||
|
||
if(pUserData->m_pExtension && !str_comp(pUserData->m_pExtension, "")) | ||
{ | ||
// must be .x at the shortest | ||
if(str_length(pUserData->m_pExtension) == 1 || pUserData->m_pExtension[0] != '.') | ||
pUserData->m_Continue = TryCallback(pUserData->m_fnLoadFailedCallback, LOAD_ERROR_INVALID_EXTENSION, pUserData->m_pExtension, pUserData->m_pUser); | ||
for(int i = 0; i < str_length(pUserData->m_pExtension); i++) | ||
pUserData->m_pExtension[i] = std::tolower(pUserData->m_pExtension[i]); | ||
} | ||
|
||
for(const auto &It : pUserData->m_PathCollection) | ||
{ | ||
if(pUserData->m_Continue) | ||
{ | ||
const char *Key = It.first.c_str(); | ||
SListDirectoryCallbackUserInfo Data{Key, pUserData}; | ||
pUserData->m_pStorage->ListDirectory(IStorage::TYPE_ALL, Key, ListDirectoryCallback, &Data); | ||
} | ||
} | ||
|
||
// Index is now populated, load the files | ||
unsigned char *pData = nullptr; | ||
unsigned int Size, Count = 0; | ||
IOHANDLE Handle; | ||
for(const auto &Directory : pUserData->m_PathCollection) | ||
{ | ||
for(const auto &File : *Directory.second) | ||
{ | ||
if(pUserData->m_Continue) | ||
{ | ||
// Wait for our turn | ||
if(pUserData->m_Flags & LOAD_FLAGS_ASYNC) | ||
{ | ||
bool Wait = true; | ||
while(Wait) | ||
{ | ||
switch(pUserData->GetJobStatus()) | ||
{ | ||
case CFileLoadJob::FILE_LOAD_JOB_STATUS_YIELD: | ||
std::this_thread::yield(); | ||
break; | ||
case CFileLoadJob::FILE_LOAD_JOB_STATUS_DONE: | ||
pUserData->m_Continue = false; | ||
[[fallthrough]]; | ||
default: | ||
Wait = false; | ||
break; | ||
} | ||
} | ||
} | ||
|
||
// Construct file path | ||
char FilePath[IO_MAX_PATH_LENGTH]; | ||
str_format(FilePath, sizeof(FilePath), "%s/%s", Directory.first.c_str(), File.c_str()); | ||
|
||
// Return if dry run | ||
if(pUserData->m_Flags & LOAD_FLAGS_DONT_READ_FILE) | ||
{ | ||
pUserData->m_fnFileLoadedCallback(pUserData->m_Flags & LOAD_FLAGS_ABSOLUTE_PATH ? FilePath : File, nullptr, 0, pUserData->m_pUser); | ||
Count++; | ||
continue; | ||
} | ||
|
||
// Probe readability | ||
Handle = io_open(FilePath, IOFLAG_READ | (pUserData->m_Flags & LOAD_FLAGS_SKIP_BOM ? IOFLAG_SKIP_BOM : 0)); | ||
if(!Handle) | ||
{ | ||
pUserData->m_Continue = TryCallback(pUserData->m_fnLoadFailedCallback, LOAD_ERROR_FILE_UNREADABLE, FilePath, pUserData->m_pUser); | ||
continue; | ||
} | ||
|
||
// Check size | ||
// system.cpp APIs are only good up to 2 GiB at the moment (signed long cannot describe a size any larger) | ||
io_seek(Handle, 0, IOSEEK_END); | ||
long ExpectedSize = io_tell(Handle); | ||
io_seek(Handle, 0, IOSEEK_START); | ||
|
||
// File is either too large for io_tell/ftell to say or it's actually empty (MinGW returns 0; MSVC returns -1) | ||
if(ExpectedSize <= 0) | ||
{ | ||
size_t RealSize = std::filesystem::file_size(FilePath); | ||
if(static_cast<size_t>(ExpectedSize) != RealSize) | ||
{ | ||
pUserData->m_Continue = TryCallback(pUserData->m_fnLoadFailedCallback, LOAD_ERROR_FILE_TOO_LARGE, FilePath, pUserData->m_pUser); | ||
continue; | ||
} | ||
} | ||
|
||
// Load file | ||
io_read_all(Handle, reinterpret_cast<void **>(&pData), &Size); | ||
if(static_cast<unsigned int>(ExpectedSize) != Size) // Possibly redundant, but accounts for memory allocation shortcomings and not just IO | ||
{ | ||
pUserData->m_Continue = TryCallback(pUserData->m_fnLoadFailedCallback, LOAD_ERROR_FILE_TOO_LARGE, FilePath, pUserData->m_pUser); | ||
continue; | ||
} | ||
|
||
// Return & cleanup | ||
pUserData->m_fnFileLoadedCallback(pUserData->m_Flags & LOAD_FLAGS_ABSOLUTE_PATH ? FilePath : File, pData, Size, pUserData->m_pUser); | ||
free(pData); | ||
Count++; | ||
io_close(Handle); | ||
} | ||
} | ||
} | ||
|
||
if(pUserData->m_Flags & LOAD_FLAGS_ASYNC) | ||
{ | ||
if(pUserData->m_fnLoadFinishedCallback) | ||
pUserData->m_fnLoadFinishedCallback(Count, pUserData->m_pUser); | ||
pUserData->m_Finished = true; | ||
return 0; | ||
} | ||
else | ||
return Count; | ||
} | ||
|
||
#define MASS_FILE_LOADER_ERROR_PREFIX "Mass file loader used " | ||
std::optional<unsigned int> CMassFileLoader::Load() | ||
{ | ||
dbg_assert(!m_RequestedPaths.empty(), MASS_FILE_LOADER_ERROR_PREFIX "without adding paths."); // Ensure paths have been added | ||
dbg_assert(bool(m_pStorage), MASS_FILE_LOADER_ERROR_PREFIX "without passing a valid IStorage instance."); // Ensure storage is valid | ||
dbg_assert(bool(m_fnFileLoadedCallback), MASS_FILE_LOADER_ERROR_PREFIX "without implementing file loaded callback."); // Ensure file loaded callback is implemented | ||
dbg_assert(m_Flags ^ LOAD_FLAGS_MASK, MASS_FILE_LOADER_ERROR_PREFIX "with invalid flags."); // Ensure flags are in bounds | ||
dbg_assert(!m_Finished, MASS_FILE_LOADER_ERROR_PREFIX "after having already been used."); // Ensure is not reused | ||
if(m_Flags & LOAD_FLAGS_ASYNC) | ||
{ | ||
dbg_assert(bool(m_pEngine), MASS_FILE_LOADER_ERROR_PREFIX "without passing a valid IEngine instance."); // Ensure engine is valid | ||
m_FileLoadJob = std::make_shared<CFileLoadJob>(&CMassFileLoader::Begin, this); | ||
m_pEngine->AddJob(m_FileLoadJob); | ||
return std::nullopt; | ||
} | ||
else | ||
return Begin(this); | ||
} | ||
|
||
#undef MASS_FILE_LOADER_ERROR_PREFIX | ||
|
||
/* TODO: | ||
* [+] test error callback return value, make sure if false is returned the callback is never called again | ||
* [ ] test every combination of flags | ||
* [+,+] test symlink and readable detection on windows and unix | ||
* ^ (waiting on working readable/symlink detection) | ||
* [x] see if you can error check regex | ||
* | ||
* [ ] fix unreadable | ||
* | ||
* [+] test multiple directories | ||
* [ ] async implementation | ||
*/ |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems redundant and like a TOCTOU bug. It's better to just read the entire file immediately with
IStorage::ReadFile
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure whether it's a good idea to always load the files into memory though. Some of our loaders only work with files, like datafiles (maps).
Edit: Never mind, I saw there is a flag
LOAD_FLAGS_DONT_READ_FILE
for this.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this is a TOCTOU bug. This is a very common way to probe file size (and as far as I know, the only way using the libc like the system APIs here are doing). Unless you mean I shouldn't be checking the file size at all?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, there is no reason to check the size first. The
io_read_all
andReadFile
functions ensure that all that can be read is read.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I originally added this so I could consistently differentiate between empty files (where fread returns 0) and files that are too big to be read (where fread also returns 0 on some compilers). I guess it's not vital that this distinction is made, but it would certainly be nice. IDK of another way to do this without checking the size first.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does
io_length
return-1
for empty files? If so, that's a bug and should be fixed separately. If not, then only useio_read_all
and removeExpectedSize
.If you want to check for too large files (which is unnecessary in my opinion), then maybe you could check in
io_read_all
instead (in a separate PR).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It returns -1 for files larger than 2GB on some compilers.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see no reason for all this code tbh. It seems to just call
io_read_all
. Ifio_read_all
has a bug, then that bug should be fixed, and not worked around.