Add FileHandle::truncate and ftruncate #7162
Conversation
|
I am not sure we want the side effects of this feature on flash based file systems |
If a particular filing system cannot or does not want to implement the call, then that's fine. Specialised filing systems can have basically arbitrary limitations. Only fully general-purpose random-access filing systems would be expected to implement every call and usage pattern and thus be usable by every application. If you are one of those, then truncating is trivial, and extension can be no worse than the application writing all the zeroes manually, which you must support. If you're not general purpose, maybe you don't support changing file lengths after creation at all. That would be fine. In a specialised system, one potential use of this is for efficient buffer pre-allocation. Eg a system that supports the sequence "open(), write(), write(), write() close()" to create a file might have a fixed-size buffer pre-allocation and thus limit on the total. But you could have a contract such that if "truncate" was issued before the first write, you set the write buffer to that size - can be used as an API to specify the total amount you're intending to write. |
|
@ARMmbed/mbed-os-storage Would y'all mind posting a response to @kjbracey-arm explanation/rationale ? |
|
Sorry about delay, this looked reasonable to me, but I want to toss in the implementations that came out of the discussions with @OscarGarciaF:
Next chance I get I can bring them in if no one beats me to it (on top of this pr hopefully). |
|
We need to have a test for this new feature |
|
Please do not merge |
|
Added unit test covering the FileHandle/retarget bits being added here. Actual functional tests would be a matter for any invidual filing systems implementing it at the minute - we've no general filing system-agnostic test suite afaik. |
|
Hey @kjbracey-arm, I tacked on the filesystem system implementations of truncate. Feel free to remove if they're just getting the in way. Thanks for writing up the documentation and other glue layers. |
|
My general approach is that I do not want to add new functionality to mbedos that is not clearly required. In addition, we need good testing at the file system level. |
|
For this particular call, I find it hard to think of it as new functionality - I think this just covers an odd omission in the existing "support general-purpose filesystems" functionality. My first thought on seeing a request to add it was "whoops, why wasn't that already there?". If we can extend files, and seek, we should be able to truncate them.
We've had a couple of user requests to access this for the general-purposes filing systems, and it also has potential utility as a pre-write size hint for key/value-type filing systems as described above. Agree that it should be added to FAT and LFS tests when we hook it up to them, so @geky's added commit now requires that. |
|
@kjbracey-arm @davidsaada @geky @dannybenor |
|
Truncate is a fundamental function for a POSIX filesystem. If it's not going in now, it should go in eventually. It's just seems to be a low priority item. @cmonr, could be move this to a feature branch for now to get it out of the pr pool? |
|
@geky Yeah, that works for me, unless anyone else opposes. I can look into it tomorrow. |
|
Branch |
|
@cmonr, turns out GitHub supports reassigning branches |
|
@geky Take a look at the last Travis CI run. GitHub may work with it just bine, but from what I've observed, Travis tends to not like it. We can see is that's still the case after a rebase ;) |
|
Closing and re-opening to get Ttravis to recognize the updated base branch. |
|
/morph build |
Build : SUCCESSBuild number : 3473 Triggering tests/morph test |
Exporter Build : SUCCESSBuild number : 3094 |
Test : FAILUREBuild number : 3264 |
|
@kjbracey-arm When you get a chance, the test failures were quite consistent across devices. |
ARM C library is really good at optimising out calls to underlying seek. The only ones we were actually detecting in the empty file case were the ones that the default FileHandle::size() made itself during the SEEK_END case. When we implement TestFile::size() directly, we will no longer see a single seek call from the ARM C library in the empty file case, so remove those tests. Beef up the non-empty file case, adding checks that we are making underlying read+write calls in the correct position, as a proxy for direct checks for underlying seek being called.
- File::truncate - FileSystem::file_truncate - FATFileSystem::file_truncate - LittleFileSystem::file_truncate
|
Okay, that was a pain. As @dannybenor suggested, the ARM C library is really good at optimising out underlying seek calls. In fact, the only ones the test was detecting in the ARM C empty file case were the ones that the default I've run the modified tests locally on ARM C and GCC. |
|
/morph build |
Build : SUCCESSBuild number : 3507 Triggering tests/morph test |
Test : SUCCESSBuild number : 3292 |
Exporter Build : SUCCESSBuild number : 3125 |
Should I go ahead and make a merging PR for the feature branch? Could there be anything else that would be aiming for the feature branch in 5.11 timeframe? |
|
Not that I'm aware of. @ARMmbed/mbed-os-storage ^^^ |
|
Probably better to aim for 5.12 at this point, but is there any reason not to move this into master? |
Description
Add support for file truncation (or extension) to the abstract API.
Unit tests added.
Implementation added for:
File::truncateFileSystem::file_truncateFATFileSystem::file_truncateLittleFileSystem::file_truncatePull request type