----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/20295/#review40335 -----------------------------------------------------------
Looks good but there's a lot going on here! Could you add documentation to the functions so we have an idea of what the libnl code is doing - e.g., the high-level steps involved in creating a filter. src/linux/routing/filter/action.hpp <https://reviews.apache.org/r/20295/#comment73336> const? src/linux/routing/filter/icmp.hpp <https://reviews.apache.org/r/20295/#comment73338> what about dropping the Ip in dstIp since it's clear from the type, i.e., just calling it 'destination'? Can you document what it means for the IP address to be optional? src/linux/routing/filter/icmp.hpp <https://reviews.apache.org/r/20295/#comment73339> const? src/linux/routing/filter/icmp.hpp <https://reviews.apache.org/r/20295/#comment73377> which set of links? src/linux/routing/filter/icmp.cpp <https://reviews.apache.org/r/20295/#comment73379> return Error()? src/linux/routing/filter/internal.hpp <https://reviews.apache.org/r/20295/#comment73383> can "matches the user provided filter specific classifier" be simplified to "the specified classifier"? if so, change in all comments. src/linux/routing/filter/internal.hpp <https://reviews.apache.org/r/20295/#comment73382> s/any type of filters/any type of filter/ here and everywhere else. src/linux/routing/filter/internal.hpp <https://reviews.apache.org/r/20295/#comment73384> s/alloc/allocate/? src/linux/routing/filter/internal.hpp <https://reviews.apache.org/r/20295/#comment73385> Can you comment on the implications of this? src/linux/routing/filter/internal.hpp <https://reviews.apache.org/r/20295/#comment73386> this looks to fit on one line. you can bring up the error check too. src/linux/routing/filter/internal.hpp <https://reviews.apache.org/r/20295/#comment73387> s/oldCls/oldClassifier/? src/linux/routing/filter/internal.hpp <https://reviews.apache.org/r/20295/#comment73410> "Each filter type defines its own pack function."? src/linux/routing/filter/internal.hpp <https://reviews.apache.org/r/20295/#comment73388> ditto src/linux/routing/filter/priority.hpp <https://reviews.apache.org/r/20295/#comment73414> s/The order/The priority order/ src/linux/routing/filter/priority.hpp <https://reviews.apache.org/r/20295/#comment73390> Can you comment on why ARP is before ICMP and IP? src/tests/routing_tests.cpp <https://reviews.apache.org/r/20295/#comment73408> can we have helpers for creating IP addresses so we can avoid writing them in hex! e.g., parsing a string "1.2.3.4" or 4-tuple of bytes (1, 2, 3, 4) src/tests/routing_tests.cpp <https://reviews.apache.org/r/20295/#comment73411> we probably shouldn't be using routable IPs. What about 127.1.2.3 and 127.4.5.6 so we're on the local 127.0.0.0/8? src/tests/routing_tests.cpp <https://reviews.apache.org/r/20295/#comment73418> Can we write tests that confirm functionality, e.g., can we ping the interfaces? (see suggestion for IPs on 127/8) - Ian Downes On April 13, 2014, 11:45 p.m., Jie Yu wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/20295/ > ----------------------------------------------------------- > > (Updated April 13, 2014, 11:45 p.m.) > > > Review request for mesos, Benjamin Hindman, Chi Zhang, Ian Downes, Vinod > Kone, and Cong Wang. > > > Repository: mesos-git > > > Description > ------- > > This is Part 2 of the linux routing library. > > > Diffs > ----- > > src/Makefile.am 560b4c7 > src/linux/routing/filter/action.hpp PRE-CREATION > src/linux/routing/filter/icmp.hpp PRE-CREATION > src/linux/routing/filter/icmp.cpp PRE-CREATION > src/linux/routing/filter/internal.hpp PRE-CREATION > src/linux/routing/filter/priority.hpp PRE-CREATION > src/tests/routing_tests.cpp PRE-CREATION > > Diff: https://reviews.apache.org/r/20295/diff/ > > > Testing > ------- > > make check > sudo make check > > > Thanks, > > Jie Yu > >
