Review Request 129267: [OS X] emulate posix_fallocate()

[Michael Pyne]

> On Oct. 27, 2016, 1:55 a.m., Michael Pyne wrote:
> > src/lib/caching/posix_fallocate_mac.h, line 97
> > <https://git.reviewboard.kde.org/r/129267/diff/1/?file=482987#file482987line97>
> >
> >     Given that the user has some input into the size of the cache I'd prefer to have some kind of integer overflow check on offset + len (since off_t is a signed integer type, this addition is susceptible to undefined behavior). A potential overflow check is available at http://stackoverflow.com/a/1514309, and maybe Qt has something as well.
> 
> René J.V. Bertin wrote:
>     I think I can safely use `__builtin_saddll_overflow` for the check, possibly using `__has_builtin(__builtin_saddll_overflow)` as a condition for providing the posix_fallocate emulation. The builtin is supposedly available for clang 3.4 and upwards, which means all reasonably recent OS X versions should have it in their system compiler.
>     What do you think?

That's fine, and using the __has_builtin guard isn't necessary (I figure you know better than I do what build configurations are supported on Mac OS X).


> On Oct. 27, 2016, 1:55 a.m., Michael Pyne wrote:
> > src/lib/caching/posix_fallocate_mac.h, line 108
> > <https://git.reviewboard.kde.org/r/129267/diff/1/?file=482987#file482987line108>
> >
> >     Given that the ftruncate man page is documented as leaving the current file offset unchanged, why are we trying to read the old offset and reset it later? Does ftruncate fail on Mac OS X if the offset is set wrong?
> >     
> >     The reason I ask is that we don't check the return value of lseek() here and while it seems like it should be safe it's at least theoretically possible to have it return an error.
> 
> René J.V. Bertin wrote:
>     That's because to me the man page does not say anything about what it does to file content, when extending a file. The magic with the file position is just to give a bit of additional assurance, by setting the pointer to the specified offset. I would be less shocked if a posix_fallocate() implementation messed up file content after the offset I specified than elsewhere. Then again I'm not entirely sure either how to interpret the meaning of the offset parameter either.
>     
>     If you think that's pointless I'll remove the whole thing, and of course it is moot currently because offset is hardcoded to 0 in KCoreAddons.
>     Let me know.

Checking the POSIX man page for ftruncate says that it should treat any file contents between the old length and extended new length of the file as if it were zero-filled, independent of the offset currently set for the file descriptor. I think it's safe to assume Mac OS X is POSIX-compatible unless they specifically say otherwise in the man page, so I'd say to please remove the check to avoid confusion later.


- Michael


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/129267/#review100318
-----------------------------------------------------------


On Oct. 26, 2016, 6:23 p.m., René J.V. Bertin wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/129267/
> -----------------------------------------------------------
> 
> (Updated Oct. 26, 2016, 6:23 p.m.)
> 
> 
> Review request for KDE Software on Mac OS X and KDE Frameworks.
> 
> 
> Repository: kcoreaddons
> 
> 
> Description
> -------
> 
> Mac OS X doesn't have `posix_fallocate()` but as written in the comments, this function can be emulated (at least for simple use with `offset=0`).
> 
> This patch introduces such an emulation, based on `mozilla::fallocation()`. There's so little of that function left that it may be overkill to maintain its original copyright statement but I'll leave that to the legalese experts.
> 
> 
> Diffs
> -----
> 
>   src/lib/caching/kshareddatacache_p.h 9fd0bb1 
>   src/lib/caching/posix_fallocate_mac.h PRE-CREATION 
> 
> Diff: https://git.reviewboard.kde.org/r/129267/diff/
> 
> 
> Testing
> -------
> 
> Works as intended on OS X 10.9.5 as far as I can tell; original file contents were not overwritten in my testing.
> 
> The autotest succeeds (with an additional debug statement left out of the patch):
> 
> ```
> > kf5-kcoreaddons/work/build/autotests/kshareddatacachetest 
> ********* Start testing of KSharedDataCacheTest *********
> Config: Using QtTest library 5.6.1, Qt 5.6.1 (x86_64-little_endian-lp64 shared (dynamic) release build; by Clang 6.0 (clang-600.0.57) (Apple))
> PASS   : KSharedDataCacheTest::initTestCase()
> QWARN  : KSharedDataCacheTest::simpleInsert() void KSharedDataCache::Private::mapSharedMemory() posix_fallocated 5273696 bytes for file "/Users/bertin/.cache/myTestCache.kcache"
> PASS   : KSharedDataCacheTest::simpleInsert()
> PASS   : KSharedDataCacheTest::cleanupTestCase()
> Totals: 3 passed, 0 failed, 0 skipped, 0 blacklisted
> ********* Finished testing of KSharedDataCacheTest *********
> ```
> 
> 
> File Attachments
> ----------------
> 
> a simple commandline test app
>   https://git.reviewboard.kde.org/media/uploaded/files/2016/10/26/e0af195f-b484-4647-b211-cc78b5b0584b__posix_fallocate.c
> 
> 
> Thanks,
> 
> René J.V. Bertin
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-mac/attachments/20161028/a4fae81e/attachment-0001.html>