> On Feb. 11, 2015, 12:08 a.m., Ben Mahler wrote: > > It might be time to introduce os::stat and os::lstat wrappers to avoid > > needing to introduce so many special case functions. > > Bernd Mathiske wrote: > That would reduce portability. > > Ben Mahler wrote: > Oh? Could you elaborate please? :) > > To be specific, I'm referring to `Try<os::Stat> os::stat()` where > `os::Stat` only contains what is specified by > [POSIX](http://pubs.opengroup.org/onlinepubs/9699919799/basedefs/sys_stat.h.html) > and uses things like `Bytes`, etc. > > Bernd Mathiske wrote: > 1. Not every OS is a POSIX OS. Can we foresee which OSs Mesos will ever > run on in its life-time? > > 2. Do we have any precedent for exposing POSIX APIs this way? Are you > sure you want to limit Mesos to POSIX-compliant OSs only? > > 3. There are always some jitters in APIs across UNIX-like OSs, despite > POSIX. Run for example "man -s 2 stat" on Mac OS X. You will find different > stat struct definitions depending on whether _DARWIN_FEATURE_64_BIT_INODE is > defined. > > 4. "man -s 2 stat" on Ubuntu. See all the differences between different > UNIXes mentioned there. This also illustrates that exposing the full struct > parameter implies exposing all the macros and constants that come with it to > discern its contents. These are even more vulnerable than the struct itself > to changes over time. > > 5. There are changes in the POSIX specification every few years and parts > of stat have been affected at least twice in the past 15 years (2001 and > 2008). What if we get caught with some code by the next change? Do you then > want to introduce #ifdef at call sites or rather follow the path of hiding > any such changes behind a well-defined-*by-us* interface? The latter is what > an OS abstraction layer does best and primarily exists for IMHO. :-) > > Bernd Mathiske wrote: > OK, the way I read it now, you mean offering a version of stat that has > paremeters strictly defined by us and thus we can keep them stable, hiding > the actual stat() behind it. Fine. But we then create more API surface than > we are ever going to need. And/or we buy into UNIX-like struct parameter > passing as an accepted, not avoided, pattern. Personally, I prefer > narrowly-defined, well-targeted special case functions, no matter how many > there are. They make the call sites simpler to write and to read.
Seems like we're already "leaking" this information by having something called `lstatsize`: this implies `lstat` + `st_size`. The caller has to understand POSIX `stat` vs `lstat` in order to make the right call. The caller also understands that to get the "size" of something, this needs to come from one of the "stat" family of functions. Returning something like `os::FileStatistics` or `file::Statistics` seems quite complementary to our existing structs in os.hpp: `os::Load`, `os::Memory`, etc. No? - Ben ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/30609/#review71880 ----------------------------------------------------------- On Feb. 10, 2015, 11:19 a.m., Bernd Mathiske wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/30609/ > ----------------------------------------------------------- > > (Updated Feb. 10, 2015, 11:19 a.m.) > > > Review request for mesos, Adam B, Benjamin Hindman, Till Toenshoff, and > Timothy Chen. > > > Bugs: MESOS-2072 > https://issues.apache.org/jira/browse/MESOS-2072 > > > Repository: mesos > > > Description > ------- > > This returns a file's size as reported by lstat(). (Not stat(). It is desired > that in case of a link, the size of the link, not the size of the referenced > file, is returned.) > > > Diffs > ----- > > 3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp > 8a4fda97ee29c185471a69f60803efc193cbe922 > 3rdparty/libprocess/3rdparty/stout/tests/os_tests.cpp > 8a2d445c2edaa70da0e7b50e14ef88b2d9224a16 > > Diff: https://reviews.apache.org/r/30609/diff/ > > > Testing > ------- > > Wrote a simple test that creates a file and tests its size, and also checks > if a non-existing file yields an error. > > > Thanks, > > Bernd Mathiske > >
