mesos-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Jie Yu" <yujie....@gmail.com>
Subject Re: Review Request 31499: (3/6) Add protocol into Filter
Date Fri, 06 Mar 2015 00:25:52 GMT


> 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.

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.


- Jie


-----------------------------------------------------------
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
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message