> On Feb. 10, 2015, 4:08 p.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. > > Ben Mahler wrote: > 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?
Yes, "lstatsize" leaks implementation detail. Ian pointed this out above and I changed it, because that really was non-optimal naming. I am going to change the review title, too, now, to make this more clear. If you pefer structs, what would you like to have in an lstat struct? I gather so far: isdir, isfile, islink, size, mtime. While we are at it, would you like me to change the existing calls to stat accordingly? What shall we call the resulting functions? stat and lstat? I take it we are going to leave out the rest of the available fields and add them by need? atime, ctime, blksize, mode, ... And do you really want me to complicate the call sites of the existing functions? - Bernd ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/30609/#review71880 ----------------------------------------------------------- On Feb. 10, 2015, 3: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, 3: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 > >
