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.