Add more functionality to LLFile and cleanup LLAPRFile

[akleshchev]

Draft, applying 'Add more functionality to LLFile and cleanup LLAPRFile' on top of a develop-linux branch to see how it goes.

[akleshchev]

@RyeMutt are you fine with adding this to linux branch?

Seems like the only issue is 'out of memory' for mac builds which does not look to be specific to this change.

[RyeMutt]

I'm currently working on a changeset to resolve the out of memory/diskspace issue on the macos runner by splitting the test and viewer builds into seperate jobs. I'd like to get that in first and have a few days to put this branch through a good round of regression testing before it gets merged into linux.

[akleshchev]

I'd like to get that in first and have a few days to put this branch through a good round of regression testing before it gets merged into linux.

File operations are mostly covered by unit tests, like llcontrol test - creates a settings file, writes, reads. If you think something isn't properly covered yet should be checked, perhaps you can suggest a unit test to implement?

[akleshchev]

Thanks, adding those changes.

[RyeMutt]

It looks like this is potentially caused by LLFileSystem::getFileSize now wrapping LLFile::size which can return -1 where as the previous version a failure would return 0.

[akleshchev]

hich can return -1 where as the previous version a failure would return 0.

Yes, it's probably better to return 0 (it returns ec either way) instead of looking for everything that expects non zero value.

Lots of places do !vf.getSize()

[fmartian]

hich can return -1 where as the previous version a failure would return 0.

Yes, it's probably better to return 0 (it returns ec either way) instead of looking for everything that expects non zero value.

Lots of places do !vf.getSize()

The idea was to fix the callers of LLFile::size() and I think I got them almost all but seems one or two escaped (or might be caused by Linux specific changes, the patch was developed against the development branch). For LLFileSystem::getSize() I think it is easiest for now to limit the returned value to 0 although the checks should be probably rather changed to "vf.getSize() > 0" for a more generic fix since that function is defined to return a signed value.

I do have more thorough changes that will avoid some of these problems (and completely do away with LLAPRFile) but it got somewhat hard to manage with the new Linux compatibility requirement too.

[akleshchev]

Switched size functions to return 0. Double-checked logs and I see nothing suspicious.

The idea was to fix the callers of LLFile::size()

I don't think it adds much value. If someone needs to check the error state, they can check ec variant.

but it got somewhat hard to manage with the new Linux compatibility requirement too.

Sorry for adding more work, at the moment it might be wise to wait a bit with that.

[fmartian]

Switched size functions to return 0.

The idea was to fix the callers of LLFile::size()

I don't think it adds much value. If someone needs to check the error state, they can check ec variant.

The problem is not in the places that call LLFile::size(). It's called only a handful times throughout so far and except in LLFileSystem the caller is actually properly checking for the value to be positive to determine if the file is valid.
The problem is with LLFileSystem::getSize() which is called all over the place without checking the return value. Most times the code just happens to not crash by chance while it does repeatedly do an allocation for 0 bytes/elements. That's not really safe.

#5172 is an attempt to fix this although testing has been limited.

A quick and dirty fix would be to limit the return value of LLFileSystem::getSize(). However that does only mask the real problem. There is more opportunity to clean/optimize things in several of those places, but I tried to make it not to big.

[Hecklezz]

Seems you accidentality changed the permissions of 20 of the 33 files to be executable (chmod 755) from their original read and user owner write (chmod 644). There is really no need for that to have happened and seemed like it was done on accident.

[akleshchev]

Seems you accidentality changed the permissions of 20 of the 33 files to be executable

Thanks for noticing. No idea how that happened, probably was same way in the original commit. Will fix some time later, probably after the holidays.

[Hecklezz]

Seems you accidentality changed the permissions of 20 of the 33 files to be executable

Thanks for noticing. No idea how that happened, probably was same way in the original commit. Will fix some time later, probably after the holidays.

After your recent commit, there are still 9 more that were changed to be executable that shouldn't be.
I also noticed there are a number of files that shouldn't be executable that have snuck in with other commits over the years, but those are outside the scope of this PR.
When this PR gets merged I'll run a script to fix them all and put up a separate PR :)

[akleshchev]

After your recent commit, there are still 9 more that were changed to be executable that shouldn't be.

Fixed. For some reason those weren't visible locally.

[RyeMutt]

LGTM!

[akleshchev]

@RyeMutt Thank you!