mesos-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Ben Mahler" <benjamin.mah...@gmail.com>
Subject Re: Review Request 30609: Added os::lstatsize().
Date Wed, 18 Feb 2015 23:54:54 GMT


> 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
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message