You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@mesos.apache.org by Bernd Mathiske <be...@mesosphere.io> on 2015/02/07 17:57:41 UTC
Re: Review Request 30609: Added os::filesize(). This returns a file's
size as reported by lstat().
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30609/
-----------------------------------------------------------
(Updated Feb. 7, 2015, 8:57 a.m.)
Review request for mesos, Adam B, Benjamin Hindman, Till Toenshoff, and Timothy Chen.
Changes
-------
summary fixed.
Summary (updated)
-----------------
Added os::filesize(). This returns a file's size as reported by lstat().
Bugs: MESOS-2072
https://issues.apache.org/jira/browse/MESOS-2072
Repository: mesos
Description
-------
Added os::filesize(). This returns a file's size as reported by lstat().
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
Re: Review Request 30609: Added a function that reports file size,
not following links.
Posted by Bernd Mathiske <be...@mesosphere.io>.
> On Feb. 23, 2015, 4:05 p.m., Timothy Chen wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp, line 409
> > <https://reviews.apache.org/r/30609/diff/3/?file=859975#file859975line409>
> >
> > I was quite confused by the "ownsize" name, I think everyone will be too, since it's a term no one is familiar with.
> >
> > I'm more in favor of just size, but looks like you want to distinguish getting the size of the symbolic link or the actual linked file size.
> >
> > How about we add a option to follow the link or not?
Brilliant! Thanks!
- Bernd
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30609/#review73716
-----------------------------------------------------------
On Feb. 22, 2015, 11:02 p.m., Bernd Mathiske wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/30609/
> -----------------------------------------------------------
>
> (Updated Feb. 22, 2015, 11:02 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.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
>
>
Re: Review Request 30609: Added a function that reports file size,
not following links.
Posted by Timothy Chen <tn...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30609/#review73716
-----------------------------------------------------------
3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp
<https://reviews.apache.org/r/30609/#comment120101>
I was quite confused by the "ownsize" name, I think everyone will be too, since it's a term no one is familiar with.
I'm more in favor of just size, but looks like you want to distinguish getting the size of the symbolic link or the actual linked file size.
How about we add a option to follow the link or not?
3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp
<https://reviews.apache.org/r/30609/#comment120102>
This method is calling lstat but logs stat, I think we should make it consistent. I propose let's fix this line to call lstat too.
- Timothy Chen
On Feb. 23, 2015, 7:02 a.m., Bernd Mathiske wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/30609/
> -----------------------------------------------------------
>
> (Updated Feb. 23, 2015, 7:02 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 (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.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
>
>
Re: Review Request 30609: Added a function that reports file size,
not following links.
Posted by Timothy Chen <tn...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30609/#review73717
-----------------------------------------------------------
3rdparty/libprocess/3rdparty/stout/tests/os_tests.cpp
<https://reviews.apache.org/r/30609/#comment120103>
You want to also test the symlink case too right?
- Timothy Chen
On Feb. 23, 2015, 7:02 a.m., Bernd Mathiske wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/30609/
> -----------------------------------------------------------
>
> (Updated Feb. 23, 2015, 7:02 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 (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.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
>
>
Re: Review Request 30609: Added a function that reports file size,
not following links.
Posted by Bernd Mathiske <be...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30609/
-----------------------------------------------------------
(Updated May 8, 2015, 1:21 p.m.)
Review request for mesos, Adam B, Benjamin Hindman, Till Toenshoff, and Timothy Chen.
Changes
-------
Enum for following symlinks or not instead of bool.
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 (updated)
-----
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
Re: Review Request 30609: Added a function that reports file size,
not following links.
Posted by Bernd Mathiske <be...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30609/#review82848
-----------------------------------------------------------
3rdparty/libprocess/3rdparty/stout/include/stout/os/stat.hpp
<https://reviews.apache.org/r/30609/#comment133684>
bool->enum
- Bernd Mathiske
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
>
>
Re: Review Request 30609: Added a function that reports file size,
not following links.
Posted by Bernd Mathiske <be...@mesosphere.io>.
-----------------------------------------------------------
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.
Changes
-------
Added test comment.
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 (updated)
-----
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
Re: Review Request 30609: Added a function that reports file size,
not following links.
Posted by Timothy Chen <tn...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30609/#review80596
-----------------------------------------------------------
Ship it!
Ship It!
- Timothy Chen
On March 11, 2015, 5:06 p.m., Bernd Mathiske wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/30609/
> -----------------------------------------------------------
>
> (Updated March 11, 2015, 5:06 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 af940a48b161c194f2efb529b3d589c543b12f61
> 3rdparty/libprocess/3rdparty/stout/tests/os_tests.cpp c396c1d2d833b2f1721092fa35b23b5c3c3d99b3
>
> 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
>
>
Re: Review Request 30609: Added a function that reports file size,
not following links.
Posted by Bernd Mathiske <be...@mesosphere.io>.
> On April 29, 2015, 2:51 p.m., Till Toenshoff wrote:
> > 3rdparty/libprocess/3rdparty/stout/tests/os_tests.cpp, line 209
> > <https://reviews.apache.org/r/30609/diff/5/?file=891428#file891428line209>
> >
> > Add a short comment on what this test does, please. We are trying to get this done for all new tests and for all those we touch.
Good point. Will do.
- Bernd
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30609/#review82028
-----------------------------------------------------------
On March 11, 2015, 10:06 a.m., Bernd Mathiske wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/30609/
> -----------------------------------------------------------
>
> (Updated March 11, 2015, 10:06 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 (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 af940a48b161c194f2efb529b3d589c543b12f61
> 3rdparty/libprocess/3rdparty/stout/tests/os_tests.cpp c396c1d2d833b2f1721092fa35b23b5c3c3d99b3
>
> 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
>
>
Re: Review Request 30609: Added a function that reports file size,
not following links.
Posted by Till Toenshoff <to...@me.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30609/#review82028
-----------------------------------------------------------
Ship it!
3rdparty/libprocess/3rdparty/stout/tests/os_tests.cpp
<https://reviews.apache.org/r/30609/#comment132654>
Add a short comment on what this test does, please. We are trying to get this done for all new tests and for all those we touch.
- Till Toenshoff
On March 11, 2015, 5:06 p.m., Bernd Mathiske wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/30609/
> -----------------------------------------------------------
>
> (Updated March 11, 2015, 5:06 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 af940a48b161c194f2efb529b3d589c543b12f61
> 3rdparty/libprocess/3rdparty/stout/tests/os_tests.cpp c396c1d2d833b2f1721092fa35b23b5c3c3d99b3
>
> 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
>
>
Re: Review Request 30609: Added a function that reports file size,
not following links.
Posted by Bernd Mathiske <be...@mesosphere.io>.
> 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
@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?
- 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
>
>
Re: Review Request 30609: Added a function that reports file size,
not following links.
Posted by Bernd Mathiske <be...@mesosphere.io>.
> 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.
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
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30609/#review82060
-----------------------------------------------------------
On March 11, 2015, 10:06 a.m., Bernd Mathiske wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/30609/
> -----------------------------------------------------------
>
> (Updated March 11, 2015, 10:06 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 (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 af940a48b161c194f2efb529b3d589c543b12f61
> 3rdparty/libprocess/3rdparty/stout/tests/os_tests.cpp c396c1d2d833b2f1721092fa35b23b5c3c3d99b3
>
> 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
>
>
Re: Review Request 30609: Added a function that reports file size,
not following links.
Posted by Bernd Mathiske <be...@mesosphere.io>.
> 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
>
>
Re: Review Request 30609: Added a function that reports file size,
not following links.
Posted by Ben Mahler <be...@gmail.com>.
> 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
>
>
Re: Review Request 30609: Added a function that reports file size,
not following links.
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30609/#review82060
-----------------------------------------------------------
3rdparty/libprocess/3rdparty/stout/include/stout/os/stat.hpp
<https://reviews.apache.org/r/30609/#comment132722>
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.
- Ben Mahler
On March 11, 2015, 5:06 p.m., Bernd Mathiske wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/30609/
> -----------------------------------------------------------
>
> (Updated March 11, 2015, 5:06 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 af940a48b161c194f2efb529b3d589c543b12f61
> 3rdparty/libprocess/3rdparty/stout/tests/os_tests.cpp c396c1d2d833b2f1721092fa35b23b5c3c3d99b3
>
> 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
>
>
Re: Review Request 30609: Added a function that reports file size,
not following links.
Posted by Bernd Mathiske <be...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30609/
-----------------------------------------------------------
(Updated March 11, 2015, 10:06 a.m.)
Review request for mesos, Adam B, Benjamin Hindman, Till Toenshoff, and Timothy Chen.
Changes
-------
Rebased. Moved os::size() to os::stat::size().
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 (updated)
-----
3rdparty/libprocess/3rdparty/stout/include/stout/os/stat.hpp af940a48b161c194f2efb529b3d589c543b12f61
3rdparty/libprocess/3rdparty/stout/tests/os_tests.cpp c396c1d2d833b2f1721092fa35b23b5c3c3d99b3
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
Re: Review Request 30609: Added a function that reports file size,
not following links.
Posted by Bernd Mathiske <be...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30609/
-----------------------------------------------------------
(Updated Feb. 26, 2015, 1:16 a.m.)
Review request for mesos, Adam B, Benjamin Hindman, Till Toenshoff, and Timothy Chen.
Changes
-------
Combining stat() and lstat() as suggested by Tim and Till. Thanks, guys!
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 (updated)
-----
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
Re: Review Request 30609: Added a function that reports file size,
not following links.
Posted by Till Toenshoff <to...@me.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30609/#review74026
-----------------------------------------------------------
3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp
<https://reviews.apache.org/r/30609/#comment120480>
+1
I would vote for:
```
// Returns the size in Bytes of a given filesystem entry. When applied on a symbolic link,
// while having `followSymlinks` disabled, this will return the length of the entry name
// (strlen).
inline Try<Bytes> size(const std::string& path, bool followSymlinks = true)
```
3rdparty/libprocess/3rdparty/stout/tests/os_tests.cpp
<https://reviews.apache.org/r/30609/#comment120481>
+1
- Till Toenshoff
On Feb. 23, 2015, 7:02 a.m., Bernd Mathiske wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/30609/
> -----------------------------------------------------------
>
> (Updated Feb. 23, 2015, 7:02 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 (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.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
>
>
Re: Review Request 30609: Added a function that reports file size,
not following links.
Posted by Bernd Mathiske <be...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30609/
-----------------------------------------------------------
(Updated Feb. 22, 2015, 11:02 p.m.)
Review request for mesos, Adam B, Benjamin Hindman, Till Toenshoff, and Timothy Chen.
Changes
-------
Changed title to be more descriptive, not mentioning lstat.
Summary (updated)
-----------------
Added a function that reports file size, not following links.
Bugs: MESOS-2072
https://issues.apache.org/jira/browse/MESOS-2072
Repository: mesos
Description (updated)
-------
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.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
Re: Review Request 30609: Added os::lstatsize().
Posted by Ben Mahler <be...@gmail.com>.
> 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.
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.
- 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
>
>
Re: Review Request 30609: Added os::lstatsize().
Posted by Bernd Mathiske <be...@mesosphere.io>.
> 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.
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
-----------------------------------------------------------
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
>
>
Re: Review Request 30609: Added os::lstatsize().
Posted by Bernd Mathiske <be...@mesosphere.io>.
> 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
>
>
Re: Review Request 30609: Added os::lstatsize().
Posted by Bernd Mathiske <be...@mesosphere.io>.
> 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. :-)
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.
- 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
>
>
Re: Review Request 30609: Added os::lstatsize().
Posted by Ben Mahler <be...@gmail.com>.
> 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
>
>
Re: Review Request 30609: Added os::lstatsize().
Posted by Bernd Mathiske <be...@mesosphere.io>.
> 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.
That would reduce portability.
- 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
>
>
Re: Review Request 30609: Added os::lstatsize().
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30609/#review71880
-----------------------------------------------------------
It might be time to introduce os::stat and os::lstat wrappers to avoid needing to introduce so many special case functions.
- Ben Mahler
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
>
>
Re: Review Request 30609: Added os::lstatsize().
Posted by Bernd Mathiske <be...@mesosphere.io>.
-----------------------------------------------------------
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.
Changes
-------
Addressed Ian's review. Proposed name now: ownsize().
Bugs: MESOS-2072
https://issues.apache.org/jira/browse/MESOS-2072
Repository: mesos
Description (updated)
-------
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 (updated)
-----
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
Re: Review Request 30609: Added os::lstatsize().
Posted by Ian Downes <ia...@gmail.com>.
> On Feb. 9, 2015, 12:07 p.m., Ian Downes wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp, line 405
> > <https://reviews.apache.org/r/30609/diff/2/?file=853657#file853657line405>
> >
> > this is leaking the implementation, both that it's using stat and that it's using lstat in particular. Why not just have size(path) do the right thing, e.g., follow the link if it's a symlink, and return an error if it's something for which it doesn't make sense?
>
> Bernd Mathiske wrote:
> What would be a good name that conveys that in case of a link we just want the link's size and not the referenced file's size? How can we make it so that a user realizes what this does without reading the implementation? Absent other suggestions, and not using lstat, I am proposing "ownsize".
>
> Reporting the referenced file's size is the wrong thing in my use case. Thanks for pointing that out in a prior review! I do not have any use right now for the "size()" variant that follows the link.
Ahh, apologies, I was stuck on the more common need to get the size of the linked file and didn't register that you did want the size of the link.
- Ian
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30609/#review71686
-----------------------------------------------------------
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
>
>
Re: Review Request 30609: Added os::lstatsize().
Posted by Bernd Mathiske <be...@mesosphere.io>.
> On Feb. 9, 2015, 12:07 p.m., Ian Downes wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp, line 405
> > <https://reviews.apache.org/r/30609/diff/2/?file=853657#file853657line405>
> >
> > this is leaking the implementation, both that it's using stat and that it's using lstat in particular. Why not just have size(path) do the right thing, e.g., follow the link if it's a symlink, and return an error if it's something for which it doesn't make sense?
What would be a good name that conveys that in case of a link we just want the link's size and not the referenced file's size? How can we make it so that a user realizes what this does without reading the implementation? Absent other suggestions, and not using lstat, I am proposing "ownsize".
Reporting the referenced file's size is the wrong thing in my use case. Thanks for pointing that out in a prior review! I do not have any use right now for the "size()" variant that follows the link.
- Bernd
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30609/#review71686
-----------------------------------------------------------
On Feb. 7, 2015, 8:59 a.m., Bernd Mathiske wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/30609/
> -----------------------------------------------------------
>
> (Updated Feb. 7, 2015, 8:59 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().
>
>
> 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
>
>
Re: Review Request 30609: Added os::lstatsize().
Posted by Ian Downes <ia...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30609/#review71686
-----------------------------------------------------------
3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp
<https://reviews.apache.org/r/30609/#comment117553>
this is leaking the implementation, both that it's using stat and that it's using lstat in particular. Why not just have size(path) do the right thing, e.g., follow the link if it's a symlink, and return an error if it's something for which it doesn't make sense?
3rdparty/libprocess/3rdparty/stout/tests/os_tests.cpp
<https://reviews.apache.org/r/30609/#comment117551>
Bytes expectedSize = Bytes(53);
...
EXPECT_SOME_EQ(expectedSize, os::lstatsize(file))
3rdparty/libprocess/3rdparty/stout/tests/os_tests.cpp
<https://reviews.apache.org/r/30609/#comment117552>
EXPECT_ERROR(os::lstatsize(...))
- Ian Downes
On Feb. 7, 2015, 8:59 a.m., Bernd Mathiske wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/30609/
> -----------------------------------------------------------
>
> (Updated Feb. 7, 2015, 8:59 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().
>
>
> 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
>
>
Re: Review Request 30609: Added os::lstatsize().
Posted by Bernd Mathiske <be...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30609/
-----------------------------------------------------------
(Updated Feb. 7, 2015, 8:59 a.m.)
Review request for mesos, Adam B, Benjamin Hindman, Till Toenshoff, and Timothy Chen.
Changes
-------
Naming in summary and description fixed.
Summary (updated)
-----------------
Added os::lstatsize().
Bugs: MESOS-2072
https://issues.apache.org/jira/browse/MESOS-2072
Repository: mesos
Description (updated)
-------
This returns a file's size as reported by lstat().
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