mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Bernd Mathiske" <be...@mesosphere.io>
Subject Re: Review Request 30609: Added a function that reports file size, not following links.
Date Tue, 05 May 2015 21:53:51 GMT


> On April 29, 2015, 4: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?
> 
> Ben Mahler wrote:
>     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?

Agree 100% on the bool and named parameters! I'll create a patch that has an enum. I am generally
in favor of moving to such practice for existing bools, BTW. So I wrongly inferred from existing
code that Mesos follows a code style that favors bools over enums? :-)

---

Virtually all languages provide some version of at least some of this functionality built-in
or by some library. It is quite common to provide file size and file existence queries without
obtaining the rest of the stat struct as well. More rarely used functions like nlinks, blksize,
gid or mtime tend to have a greater chance to only be available as part of a struct. 

So I have found both styles, but all too often without mentioning whether links are followed
for single-purpose functions. 

Having both individual functions and stat/lstat may become the right thing for us eventually.
If we ever need to return statistics in bulk in a time-critical manner. Or if in some circumstances
it makes sense to pass around this particular kind of aggregate as a whole. So far I do not
see use cases for this in Mesos. Once we have them, lets add the struct-returning variety!


- Bernd


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


On April 30, 2015, 1: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, 1: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
> 
>


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