Add more functionality to LLFile and cleanup LLAPRFile

[fmartian]

Description

This patch adds LLFile::size(), LLFile::read() and LLFile::write() static functions and replaces all calls to various LLAPRFile functions with the according functions from LLFile.

LLAPRFile::remove() replaced with LLFile::remove()
LLAPRFile::size() replaced with LLFile::size()
LLAPRFile::isExist() replaced with LLFile::isfile()

The according LLAPRFile functions are retired together with the already unused LLAPRFile::rename(), LLAPRFile::makeDir() and LLAPRFile::removeDir() functions.

Also cleans up remarks about thread safety of used APRPools where that is not relevant anymore since the according LLFile functions do not require a special memory pool.

Related Issues

• [*] Related to the discussion in Add Memory Map and 64 bit file support to LLAPRFile class #4566

Checklist

Please ensure the following before requesting review:

• I have provided a clear title and detailed description for this pull request.
• The PR is linked to a relevant issue with sufficient context.
• I have tested the changes locally and verified they work as intended.
• Code follows the project's style guidelines.
• Documentation has been updated if needed.
• I have reviewed the contributing guidelines.

Additional Notes

Next step will be to add non-static methods to the LLFile class, and make it a full RAII class in terms of the internal resources managed in that way.

[Ansariel]

Wouldn't it be smarter to use standard filesystem library at this point instead of replacing platform independent code provided via 3p with platform specific code and API calls?

[akleshchev]

Agree, using std::filestystem where possible would be nicer. It should be mostly functional now that minimum MacOS version was bumped to 12. But personaly I would be happy with having less APR regardless (we have been planing to remove the thing for years, but it's just too entrenched).

[fmartian]

Agree, using std::filestystem where possible would be nicer. It should be mostly functional now that minimum MacOS version was bumped to 12. But personally I would be happy with having less APR regardless (we have been planning to remove the thing for years, but it's just too entrenched).

With my last changes, use of the APR library is pretty much limited to the cache implementation and only very few other places. The cache is a somewhat daunting thing to replace with different functions. A solution that works 99% of the time is not enough for this. The rest seems pretty trivial to chuck once there is a working alternative, and very low risk.

Rather than waiting for std::filesystem to be fully functional it seems actually easier to extend LLFile with the necessary methods. I already have a halfway working implementation of a simple LLFile class instance with open(), seek(), tell(), and read() and write() methods. As long as the desired support for platforms is limited to Windows/Mac/Linux, it's not that a difficult exercise.
It even allows share locks, although on non-Windows it only provides advisory locks using the flock() function. Not perfect but enough to provide a means to let multiple viewer instances lock specific files from each other. Other applications with the necessary access rights but not using flock() still can clobber the files, but that is something that is almost impossible to prevent on non-Windows systems.

I did on purpose not create an all or nothing solution but wanted to do this step by step, starting with the more easy functions operating on just filenames.

But replacing the rename, remove, size and similar calls with std::filesystem methods may indeed be an option. I'll have a look at that. Still think that a fully RAII capable LLFile may be useful for normal file access. I'll check what std::filesystem can provide here and do some tests.

[fmartian]

Agree, using std::filestystem where possible would be nicer. It should be mostly functional now that minimum MacOS version was bumped to 12. But personaly I would be happy with having less APR regardless (we have been planing to remove the thing for years, but it's just too entrenched).

There is one rather implicating point about using std::filesystem functions: They tend to throw exceptions on pretty much any underlaying error. The current viewer code is definitely not prepared to deal with that in most places, so replacing functions like LLFile::rename() or LLFile::remove with the according std::filesystem functions is going to be a fairly substantial refactoring exercise. Anyone willing to venture into that?

[Ansariel]

There is one rather implicating point about using std::filesystem functions: They tend to throw exceptions on pretty much any underlaying error.

They only do if you use the throwing version of the methods. Each method is overloaded and take a std::error_code parameter - those are non-throwing.

The current viewer code is definitely not prepared to deal with that in most places, so replacing functions like LLFile::rename() or LLFile::remove with the according std::filesystem functions is going to be a fairly substantial refactoring exercise. Anyone willing to venture into that?

The idea was to use std::filesystem inside LLFile, not replacing LLFile everywhere.

[fmartian]

There is one rather implicating point about using std::filesystem functions: They tend to throw exceptions on pretty much any underlaying error.

They only do if you use the throwing version of the methods. Each method is overloaded and take a std::error_code parameter - those are non-throwing.

The current viewer code is definitely not prepared to deal with that in most places, so replacing functions like LLFile::rename() or LLFile::remove with the according std::filesystem functions is going to be a fairly substantial refactoring exercise. Anyone willing to venture into that?

The idea was to use std::filesystem inside LLFile, not replacing LLFile everywhere.

Thanks for the nudge into the right direction. I was initially seriously considering to throw out LLFile too and directly call std::filesystem and co, but that would be a real mess to manage.

So it appears that use of std::filesystem for the path related operations would be a viable option and I kind of like the noexcept variants of that interface. There are slight adaptions of behavior necessary mostly related to the viewers preferences to treat some errors not as an error at all and create_directory() does not have a permission mask, which is a minor inconvenience since the only place it is used is an obscure dir_exists_or die() and according to a comment inside, this function is only used in the simulator, which I don't think is any concern for the current viewer codebase. Even if it was, that call uses the current default value too, so not sure what relevance it has at all.

For a sane file read/write access to fully replace APR we would then have to use ifstream/ofstream and/or fstream to make LLFile a thin wrapper around this interface. The only three drawbacks I see with this:

1. There seems no public stat() like functionality in the STL
   The viewer uses this in several places to not only detect if a file or directory exists but to also then use the st_ctime, st_mtime, and st_size. The STL provides functions to return the two last values but it will do the whole string->path->openfile->stat->closefile each time. There seems no way to retrieve the st_ctime through this interface.

2. No support of long paths under Windows
   The STL maintainers have repeatedly refused to make the Windows STL implementation long path capable mainly on the grounds of backwards compatibility and their believe that they can not implement a fully safe path normalization, which would be mandatory since the long path name prefix bypasses most kernel interface path normalization and sanitation measures and passes the path unchecked to the kernel itself. This is relatively harmless in an environment where users can't directly enter full pathnames, but the STL library doesn't have the comfort of such a guarantee.

3. No shared/exclusive file locking on open
   This would be a very handy feature as it makes management of concurrent access to files from different processes a lot easier. The fact that it is only advisory on non-Windows systems isn't a killing argument, as it still would allow to manage concurrent access to files from multiple viewer instances. The native_handle that could make this possible to call the according platform function is unfortunately only a C++26 feature, so a nogo here.

And fstreams error handling is a bit awkward, not in any way similar to the more modern std::filestream error handling. Yes we could enable exceptions() on the fstream, but that does not make for cleaner code either and adds quite a bit of complexity in getting it exactly right.

[akleshchev]

So it appears that use of std::filesystem for the path related operations would be a viable option and I kind of like the noexcept variants of that interface.

Viewer already uses std's existance check in some places (not sure why), can act as an example.

and according to a comment inside, this function is only used in the simulator

You can safely ignore that. There is no mechanism to sync viewer/simulator base even for llsd, much less LLFile.

No shared/exclusive file locking on open

That's the bigger problem with replacing APR's file access (but APR as a whole has some additional issues with network and processes).

P.S. Also note that some places use boost::filesystem. Some of that is safe to replace with std::filesystem and boost can pad missing std support in other places (as not everything is supported on mac). boost::interprocess::file_lock might be a compensation for APR's locking.

[fmartian]

So it appears that use of std::filesystem for the path related operations would be a viable option and I kind of like the noexcept variants of that interface.

Viewer already uses std's existance check in some places (not sure why), can act as an example.

I think it would be useful to make the whole viewer all use the same function, most probably the LLFile wrapped function.

No shared/exclusive file locking on open

That's the bigger problem with replacing APR's file access (but APR as a whole has some additional issues with network and processes).

The file_lock is currently only used in one place really: llappviewer.cpp for the marker file itself.
It might be possible to hack that sort of into. Accessing the native handle of an existing fstream is not an option as that is only a C++26 feature, but a possible workaround might be to create a file descriptor in the old way and wrap it in a stream_buf. The possibility seems to exist but it is not really part of the official standard. That way we would have access to the underlaying file descriptor and can do flock() on that (or LockFileEx() on Windows). But it would probably be an option in the open() call, not a separate method.

P.S. Also note that some places use boost::filesystem. Some of that is safe to replace with std::filesystem and boost can pad missing std support in other places. boost::interprocess::file_lock might be a compensation for APR's locking.

boost::filesystem is basically the origin of what eventually got std::filesystem but with adaptions. So does boost::filesystem try to support the long path name prefix, although I'm not sure it fully can handle it. It at least does not bork the path when you try to determine the root of a path that has the Windows NT kernel object space prefix. More functional IMHO would be to keep that completely out of the actual path layer and to simply apply that prefix whenever a path is approaching the MAX_PATH limit before passing it to the WinAPI wide char function. But that does require proper path sanitation and while I did some of that in the old implementation it is not really enough to sanitize random user entered paths.

[fmartian]

I'm running into some rather annoying problems trying to run the unit tests.

The first problem are the annoying firewall popups that appear for every other test or so. There must be a solution for this!

Then the tests randomly seem to get stuck after reporting success. They typically start, do their thing report that they happily succeeded but never quite terminate. By far not all but enough to be a real nuisance. After 4 or so such stuck tests, the Visual Studio process scheduler has exhausted its pool and sits just idling and waiting for one of these to finally return. The only thing that helps is to go into Task Schedular and terminate the child process for these stuck tests, which of course causes the batch wrapper to report an error, but lets Visual Studio continue with launching more tests until it exhausts its pool again.

[akleshchev]

You should be able to run specific tests individually instead of all of them. Never had that issue with firewall or tests being stuck, don't have any ideas how to fix that.

[fmartian]

You should be able to run specific tests individually instead of all of them. Never had that issue with firewall or tests being stuck, don't have any ideas how to fix that.

That's my plan but I found a few other tests that depend directly or indirectly on LLFile and need to run them too, to be sure. One specific change to remove rmdir() since remove() will happily do that now too, did actually break compiling for 3 or so tests. Rather than trying to remember what all could break in the myriad of tests and how to look for these occurrences it seemed easier to just build them and run them all, except it isn't ;-(

The Firewall dialog is maybe not the actual issue. I couldn't so far establish a relationship between the tests this dialog popups for and the processes that get stuck on exit. In fact there are tests that terminate cleanly and terminate properly where this dialog actually appears, long before I get a chance to acknowledge the dialog. But those dialogs for sure are very annoying and defeat any unit test run attempt as they require you to be present at the machine to acknowledge them.

And that process hang after the test has successfully ended, really irks me. But it reminds me vaguely on a similar issue when running external tools in Visual Studio through a batch file that could make them sometimes hang after process termination. Will try a command line run of the whole autobuild build command as next step.

[Copilot]

Pull Request Overview

This pull request refactors file operations by migrating from LLAPRFile to LLFile static functions, eliminating the need for APR memory pools in file operations. The changes add new static methods LLFile::size(), LLFile::read(), and LLFile::write(), and replace all calls to deprecated LLAPRFile functions throughout the codebase.

Key changes:

• Added LLFile::size(), LLFile::read(), and LLFile::write() static functions with cross-platform implementations
• Replaced LLAPRFile::remove(), LLAPRFile::size(), and LLAPRFile::isExist() with LLFile equivalents across viewer code
• Removed deprecated LLAPRFile functions including remove(), rename(), size(), isExist(), makeDir(), and removeDir()
• Cleaned up thread safety comments that referenced APR pools, which are no longer relevant
• Improved Windows-specific implementations using native Windows APIs (CreateFileW, DeleteFileW, RemoveDirectoryW, MoveFileExW) for better performance and long path support

Reviewed Changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 6 comments.

Show a summary per file

File	Description
indra/llcommon/llfile.h	Added declarations for size(), read(), and write() static functions with documentation; reorganized comments and fixed typedef formatting
indra/llcommon/llfile.cpp	Implemented new size(), read(), and write() functions; refactored get_fileattr() to return status code via parameter; improved Windows API usage in remove(), rmdir(), and rename()
indra/llcommon/llapr.h	Removed declarations for deprecated LLAPRFile static functions
indra/llcommon/llapr.cpp	Removed implementations of deprecated LLAPRFile::remove(), rename(), isExist(), size(), makeDir(), and removeDir()
indra/newview/llvocache.cpp	Replaced LLAPRFile::remove() and LLAPRFile::isExist() with LLFile::remove() and LLFile::isfile()
indra/newview/llviewerassetupload.cpp	Replaced LLAPRFile::size() with LLFile::size() with explicit cast to S32
indra/newview/lltexturecache.cpp	Replaced all LLAPRFile::size(), LLAPRFile::remove(), and LLAPRFile::isExist() calls with LLFile equivalents; removed APR pool thread safety comments
indra/newview/llappviewer.cpp	Replaced LLAPRFile::isExist() and LLAPRFile::remove() with LLFile::isfile() and LLFile::remove() for marker file operations

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[fmartian]

This last addition is a considerable redesign of the former approach. It uses std::filesystem where possible for file operations as suggested by Ansariel. There is a drawback to this approach, because std::filesystem does not properly support long pathnames under Windows, but I believe it is acceptable for now. If proper long path support is wanted, it is relatively easy to revert to the previous implementation with native platform APIs. For stat() it may be anyhow needed since there is a considerable difference in what each platform supports and the creationData aka birthDate of a file under Linux is only officially retrievable through statx(), which has a different structure layout and element names to stat(), so our trick with a typedef alias doesn't work as easily with that.
It also adds a functional implementation of a basic file IO API to the LLFile class to support various operations needed elsewhere and a start to an implementation of a unit test for LLFile.
As Copilot correctly found, allowing random write access to a file with the FILE* based C runtime functions is difficult. The only mode that supports that is r+ but that will fail if the file doesn't already exist. Because of that I did select to not use the std::stream functions (which implement the same file open semantics as the FILE* based C runtime functions) to emulate the LLAPRFile::writeEx() function, nor were the std::stream functions enough to support any file locking.
Instead I used the rudimentary beginnings of a File class in LLUniqueFile and extended them to provide a fully working class implementation in LLFile that can be used to replace pretty much any occurrence of LLAPRFile already. Because of aforementioned limitations this is not possible without some platform specific code.
I did refrain from aggressively replacing all LLAPRFile occurrences's at this point, but it is something I'm planning to do in an upcoming PR since this PR is already starting to get quite large.

[Copilot]

Pull Request Overview

Copilot reviewed 27 out of 27 changed files in this pull request and generated 5 comments.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.