----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31503/#review74806 -----------------------------------------------------------
src/linux/routing/filter/action.hpp <https://reviews.apache.org/r/31503/#comment121568> The name 'classifier' is used in filter.hpp; I feel like also using it for action might lead to confusion. src/linux/routing/filter/internal.hpp <https://reviews.apache.org/r/31503/#comment121573> "Failed to set the classifier _action_" (thats why suggested using a different name :) src/linux/routing/filter/internal.hpp <https://reviews.apache.org/r/31503/#comment121576> If the classifier action was set but setting terminal had an error. What would happen? is rewinding necessary? src/linux/routing/filter/internal.hpp <https://reviews.apache.org/r/31503/#comment121577> The function comment looks like you are not doing anything with any non-u32 filters but returning error. Maybe document what you are doing with basic filters as well? src/linux/routing/filter/internal.hpp <https://reviews.apache.org/r/31503/#comment121570> Be consistent with other err msgs? :) "Failed to do XXX: errMsg" src/linux/routing/filter/ip.hpp <https://reviews.apache.org/r/31503/#comment121579> Let's give this public interface some comments like other functions in this file? src/tests/routing_tests.cpp <https://reviews.apache.org/r/31503/#comment121601> If you can have a way to programmatically determine the existence of this condition, this test should be checked before enabling (environemnt.cpp has control for filtering tests; sometimes we also do it in the test fixture). If that's impossible, you should print out error messages to explain this and give instructions to disable this test manually. src/tests has examples for that. EXPECT(condition) << errMsg; Also could you provide a link to the email thread or webpage where this bug is discussed / fixed in upstream for context (like how we document kernel issues in port_mapping.cpp) src/tests/routing_tests.cpp <https://reviews.apache.org/r/31503/#comment121602> kill this line src/tests/routing_tests.cpp <https://reviews.apache.org/r/31503/#comment121604> kill this line. - Chi Zhang On Feb. 27, 2015, 9:39 p.m., Cong Wang wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/31503/ > ----------------------------------------------------------- > > (Updated Feb. 27, 2015, 9:39 p.m.) > > > Review request for mesos, Chi Zhang, Ian Downes, and Jie Yu. > > > Bugs: MESOS-2422 > https://issues.apache.org/jira/browse/MESOS-2422 > > > Repository: mesos > > > Description > ------- > > Introduce a new action which saves the classid for a specified filter. > Similar to the u32 terminal action, it is not a real action defined by TC, it > is just for setting a filter attribute. The classid or flowid is used to > match the fq_codel flows. > > > Diffs > ----- > > src/linux/routing/filter/action.hpp > 410c15ad20ffa694bdb4c4187f1544ef9617ebac > src/linux/routing/filter/arp.hpp fa0ea6f93218fb3bee9f435f0268022d5edc5e11 > src/linux/routing/filter/arp.cpp bf19264ccc83e8a8b8c2affe53779ac070472925 > src/linux/routing/filter/icmp.hpp 7b478c4e23a2fb0895021ce5fb30bdf78131b871 > src/linux/routing/filter/icmp.cpp 86bd67b71a590b88344adbe10fd1b44ea1b5148d > src/linux/routing/filter/internal.hpp > 8a6c0c0390c4ed119a11f0f808e0a244f7734c45 > src/linux/routing/filter/ip.hpp 932ed4bbf57e60261ff3b48ae242a7dd5e1f4260 > src/linux/routing/filter/ip.cpp 922a732c3543a072674208b123fdfadbef2b15f2 > src/tests/routing_tests.cpp 3cda6ab8c1ad24e4b7d0b9aeda2abc595fc857a5 > > Diff: https://reviews.apache.org/r/31503/diff/ > > > Testing > ------- > > Run the testcase. > > > Thanks, > > Cong Wang > >
