----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/42587/#review117632 -----------------------------------------------------------
src/slave/containerizer/mesos/isolators/cgroups/net_cls.hpp (lines 74 - 79) <https://reviews.apache.org/r/42587/#comment178871> Can you move that helper to the cpp file as a file local helper? I don't see it being used in the header. src/slave/containerizer/mesos/isolators/cgroups/net_cls.cpp (lines 55 - 64) <https://reviews.apache.org/r/42587/#comment178885> I would suggest wrapping the comments in 70 char. src/slave/containerizer/mesos/isolators/cgroups/net_cls.cpp (line 69) <https://reviews.apache.org/r/42587/#comment178874> Can we put 0x to `hexify`? src/slave/containerizer/mesos/isolators/cgroups/net_cls.cpp (line 74) <https://reviews.apache.org/r/42587/#comment178881> This is not needed, right? You can just do: ``` used[primary].set(0); ``` src/slave/containerizer/mesos/isolators/cgroups/net_cls.cpp (line 75) <https://reviews.apache.org/r/42587/#comment178879> NOTE: src/slave/containerizer/mesos/isolators/cgroups/net_cls.cpp (lines 81 - 85) <https://reviews.apache.org/r/42587/#comment178884> I would put this under else branch: ``` if (!used.contains(primary)) { ... } else if (used[primary].all()) { return Error( "..."); } ``` src/slave/containerizer/mesos/isolators/cgroups/net_cls.cpp (lines 87 - 89) <https://reviews.apache.org/r/42587/#comment178886> Wrap the comment in 70 char width. src/slave/containerizer/mesos/isolators/cgroups/net_cls.cpp (lines 90 - 102) <https://reviews.apache.org/r/42587/#comment178887> I would prefer a for loop here: ``` for (int i = 1; i < used[primary].size(); i++) { if (!used[primary].test(i)) { used[primary].set(i); return NetClsHandle(primary, i); } } ``` src/slave/containerizer/mesos/isolators/cgroups/net_cls.cpp (lines 104 - 107) <https://reviews.apache.org/r/42587/#comment178888> This should be UNREACHABLE() src/slave/containerizer/mesos/isolators/cgroups/net_cls.cpp (line 119) <https://reviews.apache.org/r/42587/#comment178890> Please use explict check ``` if (handle.secondary == 0) ``` src/slave/containerizer/mesos/isolators/cgroups/net_cls.cpp (line 120) <https://reviews.apache.org/r/42587/#comment178889> Indentation. src/slave/containerizer/mesos/isolators/cgroups/net_cls.cpp (lines 124 - 126) <https://reviews.apache.org/r/42587/#comment178891> Ditto on my previous comments. src/slave/containerizer/mesos/isolators/cgroups/net_cls.cpp (line 129) <https://reviews.apache.org/r/42587/#comment178892> No need for this. src/slave/containerizer/mesos/isolators/cgroups/net_cls.cpp (line 153) <https://reviews.apache.org/r/42587/#comment178893> Ditto on explict check. src/slave/containerizer/mesos/isolators/cgroups/net_cls.cpp (line 154) <https://reviews.apache.org/r/42587/#comment178894> Ditto on indentation. src/slave/containerizer/mesos/isolators/cgroups/net_cls.cpp (line 158) <https://reviews.apache.org/r/42587/#comment178895> Remove extra space between return and Error. src/slave/containerizer/mesos/isolators/cgroups/net_cls.cpp (line 163) <https://reviews.apache.org/r/42587/#comment178896> No need for this temp variable. - Jie Yu On Feb. 3, 2016, 6:46 a.m., Avinash sridharan wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/42587/ > ----------------------------------------------------------- > > (Updated Feb. 3, 2016, 6:46 a.m.) > > > Review request for mesos and Jie Yu. > > > Bugs: MESOS-4345 > https://issues.apache.org/jira/browse/MESOS-4345 > > > Repository: mesos > > > Description > ------- > > Implemented the `NetClsHandleManager` class. > > > Diffs > ----- > > src/slave/containerizer/mesos/isolators/cgroups/net_cls.hpp > b4bc52114389d1c1efce2830f4292bd89bb0de7c > src/slave/containerizer/mesos/isolators/cgroups/net_cls.cpp > ddc1bf0939e5e8995e6f34fe7b8509b51704f63e > > Diff: https://reviews.apache.org/r/42587/diff/ > > > Testing > ------- > > make > > > Thanks, > > Avinash sridharan > >
