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