> On April 29, 2015, 11:48 p.m., Ben Mahler wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/os/stat.hpp, lines 61-79
> > <https://reviews.apache.org/r/30609/diff/5/?file=891427#file891427line61>
> >
> >     If we add `followSymlinks` here, it seems that one could add 
> > `followSymlinks` on all of the `stat` related methods, but that seems like 
> > it would be a bit of a mess?
> >     
> >     I'm curious whether we should just expose two simple functions to force 
> > the callers to think about following links whenever they want file 
> > statistics:
> >     
> >     ```
> >     Try<Stat> stat(const std::string& path);
> >     Try<Stat> lstat(const std::string& path);
> >     ```
> >     
> >     Otherwise, do we want 1 stout function per `stat` struct member? For 
> > each function, are we going to have a `followSymlinks` boolean? Just 
> > thinking of how to provide a complete and consistent interface that is 
> > simple to reason about.
> 
> Bernd Mathiske wrote:
>     Yes, adding followSymlinks to existing functions could be a bit of a 
> mess, because we would end up with different defaults, since stat and lstat 
> are not being used consistently at the moment.
>     
>     But adding the two wrapper functions stat and lstat without removing all 
> the others and rewriting all the call sites would be a bit of a mess, too. 
>     
>     Suggestions?
>     
>     Bernd
> 
> Bernd Mathiske wrote:
>     @bmahler, how about this: I try adding the followLinks Parameter to all 
> other functions that already exist where appropriate and try to come up with 
> a set of functions that is "orderly", including patching any call sites if 
> necessary (hopefully none or few). If the latter quality fails to emerge or 
> for any other reason you still prefer functions that return structs, I switch 
> to that. Is this worth a try?

Well, let's keep this change minimal, so what you have here is fine. Although 
the bool argument like this isn't ideal (it's too bad C++ doesn't have named 
parameters!) Consider `os::size` and `os::lsize`, or an enum FOLLOW_SYMLINKS / 
DO_NOT_FOLLOW_SYMLINKS? We can follow up with other cleanups, I'm just planting 
a seed :)

It's interesting to look at how other language libraries deal with this, for 
example python exposes 
[`os.stat`](https://docs.python.org/2/library/os.html#os.stat), 
[`os.lstat`](https://docs.python.org/2/library/os.html#os.lstat) as well as 
[`os.path.getsize`](https://docs.python.org/2/library/os.path.html#os.path.getsize).
 It's not clear from the documentation whether `os.path.getsize` follows 
symbolic links, which seems prone to confusion. Have you looked at other 
languages or libraries?


- Ben


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30609/#review82060
-----------------------------------------------------------


On April 30, 2015, 8:38 p.m., Bernd Mathiske wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/30609/
> -----------------------------------------------------------
> 
> (Updated April 30, 2015, 8:38 p.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 (on UNIXes 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/stat.hpp 
> 270c4c848fc0460dcdb9a90823281d735f4550c2 
>   3rdparty/libprocess/3rdparty/stout/tests/os_tests.cpp 
> 343f95be7f316170b37c9358627f3c2090f0e29e 
> 
> 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
> 
>

Reply via email to