-
Notifications
You must be signed in to change notification settings - Fork 85
Improve LLFile to be consistent between Windows and Linux/Mac #4831
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
base: develop
Are you sure you want to change the base?
Conversation
int rc = _wrename(utf16filename.c_str(),utf16newname.c_str()); | ||
// Posix rename() will gladly overwrite a file at newname if it exists, the Windows | ||
// rename(), respectively _wrename(), will bark on that. Instead call directly the Windows | ||
// API MoveFileEx() and use its flags to specify that overwrite is allowed. |
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.
Isn't this less safe? If multiple viewers are running in parralel it's probably better for one to fail on an existing file than for something weird to happen.
Making rename's code less platform dependent is good, but I'm not sure if 'always rewrite' is the right way.
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.
If you check in the source code you will find that there are 16 uses of LLFile:rename(). In at least 12 of them there is an (almost) direct LLFile::remove() (or in one case a LLAPRFile::remove()) call before, often in a conditional #if LL_WINDOWS precompiler clause (because of the Posix incompatibility in _wrename(). Most of them I removed in this change too, but I left some in because there exists a codepath that could result in remove() being called without rename() being called, so removing that remove() would change existing behavior (although likely not causing bad behavior as far as I could find).
The notable exceptions are:
llxfer_file.cpp (363): LLFile::remove() called a few lines before but might be called without LLFile::rename() being called because of the if clause in between.
llappviewer.cpp (2387): there is a remove() for the old_log_file about 50 lines above, and much logic in between that should not cause the dataflow to miss the rename() but not removing the file might cause some changes in the code in between. Likely not a problem but I did not try to assess every possible condition just yet, so left the remove in.
llappviewer.cpp (2402): Missed the remove() before this one and it could be removed too.
llappviewer.cpp (4258): only done once during cache migration, the new cache location should not exist at this point, why otherwise try to migrate it.
llconversationlog.cpp (466 + 470): creates a unique backup name to move old_file into, then moves current_file to old_file
lllogchat.cpp (771 + 778): Again creates a unique backup name to move old_file into, then moves current_file to old_file
lllogchat.cpp (288): only called if new_name is not a file (potential problem if there would be a directory with that name) and old file exists.
llsechandler_basic.cpp (1482): there is a remove() in the line just before but it is part of a compound condition and I was not sure if removing remove() would be fully safe here, so left it in.
There could be of course race conditions if running with multiple viewers at the same time, but they exist in the old situation just as much or even more, since the window of opportunity is larger.
Also if this change was causing trouble it would have certainly caused trouble in Linux and Mac for a long time already.
Precommit failes bacause of a space after '/// rename a file,' |
…re the LLFile::rename()
Should be fixed now and I also removed the remove() call just before rename() in llappviewer.cpp (2402) |
LLFile::remove() will now remove both files and empty directories under Windows, just as its Posix counter part on other platforms
LLFile::rename() will rename the file even if there is already another file at the new location whose permissions allow it to be overwritten.
This allows to remove multiple remove() calls throughout the source code just before rename(), which were placed to circumvent the Windows limitation of rename()
It also prepends "\\?\" for long filenames to the filenames before passing them to the Windows functions, which removes the limitation of Windows file functions to filenames with not more than MAX_PATH characters. It also tries to sanitize filenames by replacing slashes with backslashes, since the above prefix skips certain path sanitation steps.
And it adds a function getattr() which on Windows can query the file attributes without some of the overhead of _wstat(). This function is then used in isdir() and isfile().
Related Issues