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