----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34321/#review84796 -----------------------------------------------------------
Ship it! Looks good to me. A few style nits. Thanks for the merging! This is great! src/linux/routing/filter/basic.hpp <https://reviews.apache.org/r/34321/#comment136173> Move this above `#include "linux/routing/filter/action.hpp"` and insert a blank line between them. src/linux/routing/filter/basic.cpp <https://reviews.apache.org/r/34321/#comment136172> Move this right above `#include "linux/routing/internal.hpp"` src/linux/routing/filter/filter.hpp <https://reviews.apache.org/r/34321/#comment136171> Move this above `#include "linux/routing/filter/action.hpp"` and insert a blank line between them. src/linux/routing/filter/handle.hpp <https://reviews.apache.org/r/34321/#comment136170> Swap these two. We include C headers first. src/linux/routing/filter/handle.hpp <https://reviews.apache.org/r/34321/#comment136166> I don't think you need the `routing::` prefix here. src/linux/routing/filter/icmp.hpp <https://reviews.apache.org/r/34321/#comment136169> Move this above `#include "linux/routing/filter/action.hpp"` and insert a blank line between them. src/linux/routing/filter/icmp.cpp <https://reviews.apache.org/r/34321/#comment136168> Move this right above `#include "linux/routing/internal.hpp"` src/linux/routing/filter/internal.hpp <https://reviews.apache.org/r/34321/#comment136167> Move this right above `#include "linux/routing/internal.hpp"` src/linux/routing/filter/ip.hpp <https://reviews.apache.org/r/34321/#comment136174> DItto. src/linux/routing/filter/ip.cpp <https://reviews.apache.org/r/34321/#comment136175> Ditto. src/linux/routing/handle.hpp <https://reviews.apache.org/r/34321/#comment136160> Please swap these two and add a space between them. <stdint.h> is considered a C header and <ostream> is a C++ header. src/linux/routing/handle.hpp <https://reviews.apache.org/r/34321/#comment136165> Please add a blank line between methods. src/linux/routing/handle.hpp <https://reviews.apache.org/r/34321/#comment136161> I would use one line for each of those functions: ``` uint16_t primary() const { return handle >> 16; } uint16_t secondary() const { return handle & 0x0000ffff; } ... ``` src/linux/routing/handle.hpp <https://reviews.apache.org/r/34321/#comment136162> Please put this funciton outside the Handle class. See src/linux/routing/filter/ip.hpp. BTW, is this funciton used anywhere? We don't usually introduce a function that's not needed. src/linux/routing/handle.hpp <https://reviews.apache.org/r/34321/#comment136164> I don't think this is a good practice and we don't have to follow what `tc` command does. What not always print the secondary? src/linux/routing/queueing/handle.hpp <https://reviews.apache.org/r/34321/#comment136176> +1 on Cong's comment. Please adjust the comments accordingly. src/tests/routing_tests.cpp <https://reviews.apache.org/r/34321/#comment136177> Move this right above `#include "linux/routing/route.hpp"` - Jie Yu On May 19, 2015, 8:27 p.m., Paul Brett wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/34321/ > ----------------------------------------------------------- > > (Updated May 19, 2015, 8:27 p.m.) > > > Review request for mesos, Chi Zhang, Ian Downes, Jie Yu, and Cong Wang. > > > Bugs: mesos-2665 > https://issues.apache.org/jira/browse/mesos-2665 > > > Repository: mesos > > > Description > ------- > > Merge class Handle which is duplicated between filter/handle and > queueing/handle. > > > Diffs > ----- > > src/linux/routing/filter/basic.hpp 99b0b058633403e334e7230dfa7d705c99b3cb10 > src/linux/routing/filter/basic.cpp 755be7de5c43c2f3b336af6cb47af9c97a21218c > src/linux/routing/filter/filter.hpp > 024582cf8274da2e5bcccc4ebd00fcf83d6930e2 > src/linux/routing/filter/handle.hpp > 190177441bc95cf77690dbdeca189a816c6e2324 > src/linux/routing/filter/icmp.hpp d55eeee8f1e31036e75780c52690e812de265ec6 > src/linux/routing/filter/icmp.cpp 60703e727ecab9557911ab0562773771db51db90 > src/linux/routing/filter/internal.hpp > c74098dab97807084e6630998da354265680c763 > src/linux/routing/filter/ip.hpp 62bb5f89d35fc9622fca303aa01a789fcfbf2f76 > src/linux/routing/filter/ip.cpp 0d25e2d4f0ac4c3f151ad40e796dbdbc54dba9fa > src/linux/routing/handle.hpp PRE-CREATION > src/linux/routing/queueing/handle.hpp > 5f0cb7775f9190caba6b85cabf9019a97b2a7de2 > src/tests/routing_tests.cpp 6bf5e63aecf2dee36fb8cf3575c0bd2625d29dfa > > Diff: https://reviews.apache.org/r/34321/diff/ > > > Testing > ------- > > make check > > > Thanks, > > Paul Brett > >
