You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@mesos.apache.org by Jie Yu <yu...@gmail.com> on 2012/05/30 02:57:51 UTC
Re: Review Request: Add utilities to monitor and control file systems on
Linux.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/5186/
-----------------------------------------------------------
(Updated 2012-05-30 00:57:51.168327)
Review request for mesos and Benjamin Hindman.
Changes
-------
Add file system related function to src/linux/fs. Move mount related stuff into src/linux/fs.
Summary (updated)
-------
Add two files (src/linux/fs.h|cpp) to handle file system operations on Linux.
Diffs (updated)
-----
src/Makefile.am 96cb4c6
src/linux/fs.hpp PRE-CREATION
src/linux/fs.cpp PRE-CREATION
src/tests/fs_tests.cpp PRE-CREATION
Diff: https://reviews.apache.org/r/5186/diff
Testing (updated)
-------
On linux machines, make check.
Mount/unmount is not tested since root permission is required.
Thanks,
Jie
Re: Review Request: Add utilities to monitor and control file systems on
Linux.
Posted by Benjamin Hindman <be...@berkeley.edu>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/5186/#review8242
-----------------------------------------------------------
Ship it!
I'm going to commit this now.
- Benjamin
On 2012-05-30 18:03:53, Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/5186/
> -----------------------------------------------------------
>
> (Updated 2012-05-30 18:03:53)
>
>
> Review request for mesos and Benjamin Hindman.
>
>
> Summary
> -------
>
> Add two files (src/linux/fs.h|cpp) to handle file system operations on Linux.
>
>
> Diffs
> -----
>
> src/Makefile.am 96cb4c6
> src/linux/fs.hpp PRE-CREATION
> src/linux/fs.cpp PRE-CREATION
> src/tests/fs_tests.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/5186/diff
>
>
> Testing
> -------
>
> On linux machines, make check.
>
> Mount/unmount is not tested since root permission is required.
>
>
> Thanks,
>
> Jie
>
>
Re: Review Request: Add utilities to monitor and control file systems on
Linux.
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/5186/
-----------------------------------------------------------
(Updated 2012-05-30 18:03:53.215507)
Review request for mesos and Benjamin Hindman.
Changes
-------
Address Ben's review feedbacks.
Summary
-------
Add two files (src/linux/fs.h|cpp) to handle file system operations on Linux.
Diffs (updated)
-----
src/Makefile.am 96cb4c6
src/linux/fs.hpp PRE-CREATION
src/linux/fs.cpp PRE-CREATION
src/tests/fs_tests.cpp PRE-CREATION
Diff: https://reviews.apache.org/r/5186/diff
Testing
-------
On linux machines, make check.
Mount/unmount is not tested since root permission is required.
Thanks,
Jie
Re: Review Request: Add utilities to monitor and control file systems on
Linux.
Posted by Jie Yu <yu...@gmail.com>.
> On 2012-05-30 17:16:34, Benjamin Hindman wrote:
> > src/linux/fs.hpp, lines 23-24
> > <https://reviews.apache.org/r/5186/diff/5/?file=110848#file110848line23>
> >
> > Separate these two with a newline please.
Done.
> On 2012-05-30 17:16:34, Benjamin Hindman wrote:
> > src/linux/fs.cpp, lines 25-26
> > <https://reviews.apache.org/r/5186/diff/5/?file=110849#file110849line25>
> >
> > Newline unless common prefix directories.
Done.
> On 2012-05-30 17:16:34, Benjamin Hindman wrote:
> > src/linux/fs.cpp, line 35
> > <https://reviews.apache.org/r/5186/diff/5/?file=110849#file110849line35>
> >
> > s/Lock */Lock* /
Done.
> On 2012-05-30 17:16:34, Benjamin Hindman wrote:
> > src/linux/fs.cpp, line 37
> > <https://reviews.apache.org/r/5186/diff/5/?file=110849#file110849line37>
> >
> > s/Lock */Lock* /
Done.
> On 2012-05-30 17:16:34, Benjamin Hindman wrote:
> > src/linux/fs.cpp, line 43
> > <https://reviews.apache.org/r/5186/diff/5/?file=110849#file110849line43>
> >
> > s/char */char*/ (here and below please).
Done.
> On 2012-05-30 17:16:34, Benjamin Hindman wrote:
> > src/linux/fs.cpp, line 49
> > <https://reviews.apache.org/r/5186/diff/5/?file=110849#file110849line49>
> >
> > How about just 'return ::hasmntopt(...) != NULL;'
Done.
> On 2012-05-30 17:16:34, Benjamin Hindman wrote:
> > src/linux/fs.cpp, line 56
> > <https://reviews.apache.org/r/5186/diff/5/?file=110849#file110849line56>
> >
> > Just call this 'table' instead of rv. In general, we prefer names which are more descriptive than 'rv', 'ret', 'r', etc.
Done.
> On 2012-05-30 17:16:34, Benjamin Hindman wrote:
> > src/linux/fs.cpp, line 58
> > <https://reviews.apache.org/r/5186/diff/5/?file=110849#file110849line58>
> >
> > s/FILE */FILE* /
Done.
> On 2012-05-30 17:16:34, Benjamin Hindman wrote:
> > src/linux/fs.cpp, line 85
> > <https://reviews.apache.org/r/5186/diff/5/?file=110849#file110849line85>
> >
> > This line should not compile.
Done.
> On 2012-05-30 17:16:34, Benjamin Hindman wrote:
> > src/linux/fs.cpp, line 111
> > <https://reviews.apache.org/r/5186/diff/5/?file=110849#file110849line111>
> >
> > Don't think this comment is necessary (or the one in the previous function). ;)
Done.
> On 2012-05-30 17:16:34, Benjamin Hindman wrote:
> > src/linux/fs.cpp, line 125
> > <https://reviews.apache.org/r/5186/diff/5/?file=110849#file110849line125>
> >
> > We prefer to do explicit null checks, i.e., 'if (fstab == NULL) {'. Also, if the semantics here are that NULL means end of entries, please say so in a comment.
Done.
> On 2012-05-30 17:16:34, Benjamin Hindman wrote:
> > src/linux/fs.cpp, line 112
> > <https://reviews.apache.org/r/5186/diff/5/?file=110849#file110849line112>
> >
> > Again, 'table' is preferred (and it reads better other places, i.e., 'table.entries.push_back(...)').
Done.
> On 2012-05-30 17:16:34, Benjamin Hindman wrote:
> > src/linux/fs.cpp, line 124
> > <https://reviews.apache.org/r/5186/diff/5/?file=110849#file110849line124>
> >
> > s/fstab */fstab* /
Done.
> On 2012-05-30 17:16:34, Benjamin Hindman wrote:
> > src/linux/fs.cpp, lines 159-160
> > <https://reviews.apache.org/r/5186/diff/5/?file=110849#file110849line159>
> >
> > Formatting seems wrong, +4 for this please.
Done.
> On 2012-05-30 17:16:34, Benjamin Hindman wrote:
> > src/linux/fs.cpp, line 173
> > <https://reviews.apache.org/r/5186/diff/5/?file=110849#file110849line173>
> >
> > +4 spaces please.
Done.
> On 2012-05-30 17:16:34, Benjamin Hindman wrote:
> > src/tests/fs_tests.cpp, lines 42-43
> > <https://reviews.apache.org/r/5186/diff/5/?file=110850#file110850line42>
> >
> > Great idea! But you should use Option instead of Result here.
Done.
> On 2012-05-30 17:16:34, Benjamin Hindman wrote:
> > src/tests/fs_tests.cpp, line 53
> > <https://reviews.apache.org/r/5186/diff/5/?file=110850#file110850line53>
> >
> > This should be an ASSERT_TRUE since you're doing a '.get()' on the next line.
Done.
> On 2012-05-30 17:16:34, Benjamin Hindman wrote:
> > src/tests/fs_tests.cpp, line 60
> > <https://reviews.apache.org/r/5186/diff/5/?file=110850#file110850line60>
> >
> > s/rv/table/
Done.
> On 2012-05-30 17:16:34, Benjamin Hindman wrote:
> > src/tests/fs_tests.cpp, line 64
> > <https://reviews.apache.org/r/5186/diff/5/?file=110850#file110850line64>
> >
> > Again, Option instead of Result.
Done.
> On 2012-05-30 17:16:34, Benjamin Hindman wrote:
> > src/tests/fs_tests.cpp, line 71
> > <https://reviews.apache.org/r/5186/diff/5/?file=110850#file110850line71>
> >
> > This should be an ASSERT_TRUE.
Done.
> On 2012-05-30 17:16:34, Benjamin Hindman wrote:
> > src/tests/fs_tests.cpp, line 80
> > <https://reviews.apache.org/r/5186/diff/5/?file=110850#file110850line80>
> >
> > s/rv/table/
Done.
> On 2012-05-30 17:16:34, Benjamin Hindman wrote:
> > src/tests/fs_tests.cpp, lines 84-85
> > <https://reviews.apache.org/r/5186/diff/5/?file=110850#file110850line84>
> >
> > s/Result/Option/
Done.
> On 2012-05-30 17:16:34, Benjamin Hindman wrote:
> > src/tests/fs_tests.cpp, line 95
> > <https://reviews.apache.org/r/5186/diff/5/?file=110850#file110850line95>
> >
> > ASSERT_TRUE.
Done.
> On 2012-05-30 17:16:34, Benjamin Hindman wrote:
> > src/linux/fs.cpp, line 70
> > <https://reviews.apache.org/r/5186/diff/5/?file=110849#file110849line70>
> >
> > Is an errno set which can be used to determine whether or not you just need to try again with a larger buffer?
Not said in the specification. I looked the glibc code, and seems that we cannot distinguish between (1) buffer is too small and (2) reach the end of entries.
However, in glibc implementation, they use a buffer of size 1024. Therefore, I think PATH_MAX (4096) should be enough.
> On 2012-05-30 17:16:34, Benjamin Hindman wrote:
> > src/tests/fs_tests.cpp, line 38
> > <https://reviews.apache.org/r/5186/diff/5/?file=110850#file110850line38>
> >
> > s/rv/table/
Done.
- Jie
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/5186/#review8217
-----------------------------------------------------------
On 2012-05-30 18:03:53, Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/5186/
> -----------------------------------------------------------
>
> (Updated 2012-05-30 18:03:53)
>
>
> Review request for mesos and Benjamin Hindman.
>
>
> Summary
> -------
>
> Add two files (src/linux/fs.h|cpp) to handle file system operations on Linux.
>
>
> Diffs
> -----
>
> src/Makefile.am 96cb4c6
> src/linux/fs.hpp PRE-CREATION
> src/linux/fs.cpp PRE-CREATION
> src/tests/fs_tests.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/5186/diff
>
>
> Testing
> -------
>
> On linux machines, make check.
>
> Mount/unmount is not tested since root permission is required.
>
>
> Thanks,
>
> Jie
>
>
Re: Review Request: Add utilities to monitor and control file systems on
Linux.
Posted by Benjamin Hindman <be...@berkeley.edu>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/5186/#review8217
-----------------------------------------------------------
src/linux/fs.hpp
<https://reviews.apache.org/r/5186/#comment17768>
Separate these two with a newline please.
src/linux/fs.cpp
<https://reviews.apache.org/r/5186/#comment17769>
Newline unless common prefix directories.
src/linux/fs.cpp
<https://reviews.apache.org/r/5186/#comment17771>
s/Lock */Lock* /
src/linux/fs.cpp
<https://reviews.apache.org/r/5186/#comment17772>
s/Lock */Lock* /
src/linux/fs.cpp
<https://reviews.apache.org/r/5186/#comment17773>
s/char */char*/ (here and below please).
src/linux/fs.cpp
<https://reviews.apache.org/r/5186/#comment17793>
How about just 'return ::hasmntopt(...) != NULL;'
src/linux/fs.cpp
<https://reviews.apache.org/r/5186/#comment17774>
Just call this 'table' instead of rv. In general, we prefer names which are more descriptive than 'rv', 'ret', 'r', etc.
src/linux/fs.cpp
<https://reviews.apache.org/r/5186/#comment17777>
s/FILE */FILE* /
src/linux/fs.cpp
<https://reviews.apache.org/r/5186/#comment17775>
Is an errno set which can be used to determine whether or not you just need to try again with a larger buffer?
src/linux/fs.cpp
<https://reviews.apache.org/r/5186/#comment17776>
This line should not compile.
src/linux/fs.cpp
<https://reviews.apache.org/r/5186/#comment17778>
Don't think this comment is necessary (or the one in the previous function). ;)
src/linux/fs.cpp
<https://reviews.apache.org/r/5186/#comment17785>
Again, 'table' is preferred (and it reads better other places, i.e., 'table.entries.push_back(...)').
src/linux/fs.cpp
<https://reviews.apache.org/r/5186/#comment17779>
s/fstab */fstab* /
src/linux/fs.cpp
<https://reviews.apache.org/r/5186/#comment17782>
We prefer to do explicit null checks, i.e., 'if (fstab == NULL) {'. Also, if the semantics here are that NULL means end of entries, please say so in a comment.
src/linux/fs.cpp
<https://reviews.apache.org/r/5186/#comment17789>
Formatting seems wrong, +4 for this please.
src/linux/fs.cpp
<https://reviews.apache.org/r/5186/#comment17790>
+4 spaces please.
src/tests/fs_tests.cpp
<https://reviews.apache.org/r/5186/#comment17794>
s/rv/table/
src/tests/fs_tests.cpp
<https://reviews.apache.org/r/5186/#comment17795>
Great idea! But you should use Option instead of Result here.
src/tests/fs_tests.cpp
<https://reviews.apache.org/r/5186/#comment17799>
This should be an ASSERT_TRUE since you're doing a '.get()' on the next line.
src/tests/fs_tests.cpp
<https://reviews.apache.org/r/5186/#comment17796>
s/rv/table/
src/tests/fs_tests.cpp
<https://reviews.apache.org/r/5186/#comment17797>
Again, Option instead of Result.
src/tests/fs_tests.cpp
<https://reviews.apache.org/r/5186/#comment17798>
This should be an ASSERT_TRUE.
src/tests/fs_tests.cpp
<https://reviews.apache.org/r/5186/#comment17800>
s/rv/table/
src/tests/fs_tests.cpp
<https://reviews.apache.org/r/5186/#comment17801>
s/Result/Option/
src/tests/fs_tests.cpp
<https://reviews.apache.org/r/5186/#comment17802>
ASSERT_TRUE.
- Benjamin
On 2012-05-30 15:25:22, Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/5186/
> -----------------------------------------------------------
>
> (Updated 2012-05-30 15:25:22)
>
>
> Review request for mesos and Benjamin Hindman.
>
>
> Summary
> -------
>
> Add two files (src/linux/fs.h|cpp) to handle file system operations on Linux.
>
>
> Diffs
> -----
>
> src/Makefile.am 96cb4c6
> src/linux/fs.hpp PRE-CREATION
> src/linux/fs.cpp PRE-CREATION
> src/tests/fs_tests.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/5186/diff
>
>
> Testing
> -------
>
> On linux machines, make check.
>
> Mount/unmount is not tested since root permission is required.
>
>
> Thanks,
>
> Jie
>
>
Re: Review Request: Add utilities to monitor and control file systems on
Linux.
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/5186/
-----------------------------------------------------------
(Updated 2012-05-30 15:25:22.484183)
Review request for mesos and Benjamin Hindman.
Summary
-------
Add two files (src/linux/fs.h|cpp) to handle file system operations on Linux.
Diffs (updated)
-----
src/Makefile.am 96cb4c6
src/linux/fs.hpp PRE-CREATION
src/linux/fs.cpp PRE-CREATION
src/tests/fs_tests.cpp PRE-CREATION
Diff: https://reviews.apache.org/r/5186/diff
Testing
-------
On linux machines, make check.
Mount/unmount is not tested since root permission is required.
Thanks,
Jie
Re: Review Request: Add utilities to monitor and control file systems on
Linux.
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/5186/
-----------------------------------------------------------
(Updated 2012-05-30 03:01:54.184228)
Review request for mesos and Benjamin Hindman.
Changes
-------
Fix some format issues. Implement MountTable::Entry::hasOption function.
Summary
-------
Add two files (src/linux/fs.h|cpp) to handle file system operations on Linux.
Diffs (updated)
-----
src/linux/fs.hpp PRE-CREATION
src/Makefile.am 96cb4c6
src/linux/fs.cpp PRE-CREATION
src/tests/fs_tests.cpp PRE-CREATION
Diff: https://reviews.apache.org/r/5186/diff
Testing
-------
On linux machines, make check.
Mount/unmount is not tested since root permission is required.
Thanks,
Jie