> On March 5, 2015, 11:12 p.m., Jie Yu wrote:
> > I don't think adding `protocol` to `Filter` is a good idea. We should keep
> > `protocol` in `Classifier` because the `protocol` is used for classifying
> > packets, isn't it?
> >
> > Regarding the basic filter, you need to something the following:
> > ```
> > struct basic::Classifier
> > {
> > uint16_t protocol;
> > };
> >
> > Result<basic::Classifier> decode<basic::Classifier>(...)
> > {
> > if (rtnl_tc_get_kind(TC_CAST(cls.get())) == string("basic")) {
> > return basic::Classifier(rtnl_cls_get_protocol(cls.get()));
> > }
> >
> > return None();
> > }
> >
> > template <>
> > Try<Nothing> encode<basic::Classifier>(...)
> > {
> > rtnl_cls_set_protocol(cls.get(), classifier.protocol());
> > ...
> > }
> > ```
>
> Cong Wang wrote:
> Our goal is not just to make basic filter work for my case, as we
> discussed, arp filter will eventually (after this patchset) move on top of
> basic filter too, because their only difference is protocol. Therefore,
> leaving protocol inside basic::Classifier is not as good as exposing it in
> parameter, so that in future arp::create() can simply call basic::create(...,
> ETH_P_ARP)? Also, my way could save a little bit of code since protocol
> setting/getting is moved to the generic layer.
>
> Jie Yu wrote:
> First of all, I am not convinced that merging arp and basic filters will
> be hard in the way i just described. For example:
> ```
> filter::basic::create(
> ...
> basic::Classifier(ETH_P_ARP),
> ...);
> ```
>
> Also, regarding your second argument. In mesos, we usually value code
> cleaness and readability than performance and duplication.
>
> Cong Wang wrote:
> Put it in another way: why do we keep priority in Filter instead of
> filter::X::Classifier? Given that fact that different types of filters have
> different priorities. As generic filter attributes, priority and protocol
> equal here for discussion.
>
> Jie Yu wrote:
> Priority is not about how you "classify" a packet. It's about picking
> which filter to "classify" the packet first. While "protocol" is the about
> classifying a packet, right?
Yes and no. Procotocol can be used to select filters themselves too:
# /usr/sbin/tc filter show dev dummy0
filter parent 8001: protocol all pref 49151 basic
filter parent 8001: protocol all pref 49151 basic handle 0x1 flowid 8001:2
filter parent 8001: protocol arp pref 49152 basic
filter parent 8001: protocol arp pref 49152 basic handle 0x1 flowid 8001:1
# /usr/sbin/tc filter show dev dummy0 protocol all
filter parent 8001: pref 49151 basic
filter parent 8001: pref 49151 basic handle 0x1 flowid 8001:2
# /usr/sbin/tc filter show dev dummy0 protocol arp
filter parent 8001: pref 49152 basic
filter parent 8001: pref 49152 basic handle 0x1 flowid 8001:1
In this aspect, it is same with priority. It makes sense to say:
basic::remove(..., ETH_P_ALL);
basic::remove(..., ETH_P_ARP);
and this gives us more flexibility, as arp::remove(...) is literally a wrapper
of basic::remove(..., ETH_P_ARP) for convenience.
I think we should either take a step back as I did, not using the name "basic";
or make it as a real "basic" filter which takes a procotol.
- Cong
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31499/#review75408
-----------------------------------------------------------
On March 4, 2015, 8:10 p.m., Cong Wang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/31499/
> -----------------------------------------------------------
>
> (Updated March 4, 2015, 8:10 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
> -------
>
> Protocol is a generic attribute for a filter, so it makes sense to move it
> into Filter. This is a preparation for the next patch.
>
>
> Diffs
> -----
>
> src/linux/routing/filter/arp.cpp bf19264
> src/linux/routing/filter/filter.hpp a603d73
> src/linux/routing/filter/icmp.cpp 86bd67b
> src/linux/routing/filter/internal.hpp 8a6c0c0
> src/linux/routing/filter/ip.cpp 922a732
>
> Diff: https://reviews.apache.org/r/31499/diff/
>
>
> Testing
> -------
>
> Run test cases.
>
>
> Thanks,
>
> Cong Wang
>
>
