----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/20292/#review40283 -----------------------------------------------------------
configure.ac <https://reviews.apache.org/r/20292/#comment73264> Are these CFLAGS or CPPFLAGS? You append them to MESOS_CPPFLAGS in the Makefile. src/linux/routing/link/internal.hpp <https://reviews.apache.org/r/20292/#comment73304> s/none/None/? src/linux/routing/link/internal.hpp <https://reviews.apache.org/r/20292/#comment73301> It's not intuitive to read "if (err != 0) return Error()" what about calling it status? "if (status != 0) return Error()" reads better to me. src/linux/routing/link/internal.hpp <https://reviews.apache.org/r/20292/#comment73302> Can this not be named "link" with the function argument as "_link" as you have done in test()? src/linux/routing/link/internal.hpp <https://reviews.apache.org/r/20292/#comment73303> s/none/None/ and everywhere else src/linux/routing/link/internal.hpp <https://reviews.apache.org/r/20292/#comment73306> Why do you need the bitwise & to test equality - shouldn't == suffice? src/linux/routing/link/internal.hpp <https://reviews.apache.org/r/20292/#comment73307> either: s/interface/interfaces/ or: s/has/have/ src/linux/routing/link/link.hpp <https://reviews.apache.org/r/20292/#comment73313> Document that if pid is None then the new veth is put into the caller's namespace. src/linux/routing/link/link.hpp <https://reviews.apache.org/r/20292/#comment73310> reference src/linux/routing/link/link.hpp <https://reviews.apache.org/r/20292/#comment73311> s/ifindex/ifIndex? src/linux/routing/link/link.hpp <https://reviews.apache.org/r/20292/#comment73312> This fits on one line. Any reason to not use hashmap? src/linux/routing/link/link.cpp <https://reviews.apache.org/r/20292/#comment73315> reference src/linux/routing/link/link.cpp <https://reviews.apache.org/r/20292/#comment73327> What does it mean to create a virtual pair of interfaces if pid is None - both are in the same namespace? I thought one of the pair was in the parent's namespace and the other had to be in a different namespace. src/linux/routing/link/link.cpp <https://reviews.apache.org/r/20292/#comment73319> ditto src/tests/routing_tests.cpp <https://reviews.apache.org/r/20292/#comment73324> s/LinkIfindex/LinkIfIndex/? src/tests/routing_tests.cpp <https://reviews.apache.org/r/20292/#comment73328> Move the LinkCreate test before the LinkRemove test? src/tests/routing_tests.cpp <https://reviews.apache.org/r/20292/#comment73330> Is this as simple as checking the output of an os::shell() with "ip setns exec ..."? src/tests/routing_tests.cpp <https://reviews.apache.org/r/20292/#comment73332> can you bring up the TEST_PEER_LINK? src/tests/routing_tests.cpp <https://reviews.apache.org/r/20292/#comment73333> can you link::setMac on the TEST_PEER_LINK? - Ian Downes On April 13, 2014, 11:39 p.m., Jie Yu wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/20292/ > ----------------------------------------------------------- > > (Updated April 13, 2014, 11:39 p.m.) > > > Review request for mesos, Benjamin Hindman, Chi Zhang, Ian Downes, Vinod > Kone, and Cong Wang. > > > Repository: mesos-git > > > Description > ------- > > This is Part 1 of the linux routing library. > > > Diffs > ----- > > configure.ac c1de6d7eec1fc740c031bebb21a662e654328943 > src/Makefile.am 560b4c77a4e44ef1833b3b4afd95bb8040c9f229 > src/linux/routing/internal.hpp PRE-CREATION > src/linux/routing/link/internal.hpp PRE-CREATION > src/linux/routing/link/link.hpp PRE-CREATION > src/linux/routing/link/link.cpp PRE-CREATION > src/tests/routing_tests.cpp PRE-CREATION > > Diff: https://reviews.apache.org/r/20292/diff/ > > > Testing > ------- > > make check > sudo make check > > > Thanks, > > Jie Yu > >
