----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/5394/#review8408 -----------------------------------------------------------
Ship it! Awesome! src/linux/cgroups.cpp <https://reviews.apache.org/r/5394/#comment18076> Okay, I think it's time to take this out of the mesos::internal namespace, and just put it in the cgroups namespace. src/linux/cgroups.cpp <https://reviews.apache.org/r/5394/#comment18075> Liking the name better, but given that you're the variable name 'info' and 'infos' everywhere, I'm thinking the right name here is SubsystemInfo. src/linux/cgroups.cpp <https://reviews.apache.org/r/5394/#comment18077> Let's stick all these _* functions in an cgroups::internal namespace. src/linux/cgroups.cpp <https://reviews.apache.org/r/5394/#comment18078> s/mountVFS/mount/ src/linux/cgroups.cpp <https://reviews.apache.org/r/5394/#comment18079> s/unmountVFS/unmount/ src/linux/cgroups.cpp <https://reviews.apache.org/r/5394/#comment18080> return !disabled; src/linux/cgroups.cpp <https://reviews.apache.org/r/5394/#comment18081> Why do we need both cgroups::enabled(subsystems) and cgroups::subsystems()? It seems like if nothing more, one should be implemented in terms of the other. src/linux/cgroups.cpp <https://reviews.apache.org/r/5394/#comment18083> Add a comment here that you're basically trying to get the intersection of enabled subsystems in the system and mount options. It would be even cooler if we could also have exactly that code, something like: Try<set<string> > subsystems = cgroups::subsystems(); if (subsystems.isError()) { ...; } Try<set<string> > options = hierarchyMountEntry.options(); if (options.isError()) { ...; } return sets::intersect(subsystems.get(), options.get()); src/linux/cgroups.cpp <https://reviews.apache.org/r/5394/#comment18085> Now I see why you created this. I think we need some set abstractions! How about a new header, sets.hpp, which provides things like 'intersection', 'union', etc (they can wrap the STL versions appropriately). src/linux/cgroups.cpp <https://reviews.apache.org/r/5394/#comment18084> No need for explicit '== false'. src/linux/cgroups.cpp <https://reviews.apache.org/r/5394/#comment18086> s/non/none/ src/linux/cgroups.cpp <https://reviews.apache.org/r/5394/#comment18087> while ((node = fts_read(tree) != NULL) { src/linux/cgroups.cpp <https://reviews.apache.org/r/5394/#comment18088> if (errno != 0) { src/linux/cgroups.cpp <https://reviews.apache.org/r/5394/#comment18089> if (fts_close(tree) != 0) { - Benjamin Hindman On June 19, 2012, 4:52 a.m., Jie Yu wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/5394/ > ----------------------------------------------------------- > > (Updated June 19, 2012, 4:52 a.m.) > > > Review request for mesos, Benjamin Hindman and Vinod Kone. > > > Description > ------- > > Add simple utility function to control Linux control groups (cgroups). > > > Diffs > ----- > > src/Makefile.am 8760f59 > src/linux/cgroups.hpp PRE-CREATION > src/linux/cgroups.cpp PRE-CREATION > src/tests/cgroups_tests.cpp PRE-CREATION > > Diff: https://reviews.apache.org/r/5394/diff/ > > > Testing > ------- > > On Linux machine, make check. > > (Root permission required. It is detected automatically, thanks to the gtest > hack). > > > Thanks, > > Jie Yu > >
