You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@mesos.apache.org by Cong Wang <cw...@twopensource.com> on 2015/03/11 00:27:12 UTC
Re: Review Request 31504: Add a basic filter to match all packets
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31504/
-----------------------------------------------------------
(Updated March 10, 2015, 11:27 p.m.)
Review request for mesos, Chi Zhang, Ian Downes, and Jie Yu.
Changes
-------
Address review comments
Summary (updated)
-----------------
Add a basic filter to match all packets
Bugs: MESOS-2422
https://issues.apache.org/jira/browse/MESOS-2422
Repository: mesos
Description
-------
We need a default filter which has the lowest priority and can match all packets that are not matched by the previous filters, so that no packet will escape (otherwise it would be dropped by fq_codel). Because basic filter can accept protocol as a parameter, the arp filter which is based on basic can move to this as well.
Diffs (updated)
-----
src/Makefile.am 3059818231c46484039d179cd6916932eff6cd68
src/linux/routing/filter/basic.hpp PRE-CREATION
src/linux/routing/filter/basic.cpp PRE-CREATION
src/tests/routing_tests.cpp 7cc3b57a3b71544874557d2b1cf88a241b7062ba
Diff: https://reviews.apache.org/r/31504/diff/
Testing
-------
Run the testcase.
Thanks,
Cong Wang
Re: Review Request 31504: Add a basic filter to match all packets
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31504/#review76009
-----------------------------------------------------------
src/linux/routing/filter/basic.hpp
<https://reviews.apache.org/r/31504/#comment123393>
We wrap comment in 70 char width. Please fix here and everywhere else.
src/linux/routing/filter/basic.hpp
<https://reviews.apache.org/r/31504/#comment123395>
Again, please use 'classid' consistently.
src/linux/routing/filter/basic.cpp
<https://reviews.apache.org/r/31504/#comment123401>
Please expose this Classifier in the header file.
src/linux/routing/filter/basic.cpp
<https://reviews.apache.org/r/31504/#comment123398>
Reorder these two.
src/linux/routing/filter/basic.cpp
<https://reviews.apache.org/r/31504/#comment123400>
Oops. Shoudn't you do
```
rtnl_cls_set_protocol(cls.get(), classifier.protocol());
```
src/linux/routing/filter/basic.cpp
<https://reviews.apache.org/r/31504/#comment123402>
Oops, do yo still need this check?
- Jie Yu
On March 10, 2015, 11:27 p.m., Cong Wang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/31504/
> -----------------------------------------------------------
>
> (Updated March 10, 2015, 11:27 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
> -------
>
> We need a default filter which has the lowest priority and can match all packets that are not matched by the previous filters, so that no packet will escape (otherwise it would be dropped by fq_codel). Because basic filter can accept protocol as a parameter, the arp filter which is based on basic can move to this as well.
>
>
> Diffs
> -----
>
> src/Makefile.am 3059818231c46484039d179cd6916932eff6cd68
> src/linux/routing/filter/basic.hpp PRE-CREATION
> src/linux/routing/filter/basic.cpp PRE-CREATION
> src/tests/routing_tests.cpp 7cc3b57a3b71544874557d2b1cf88a241b7062ba
>
> Diff: https://reviews.apache.org/r/31504/diff/
>
>
> Testing
> -------
>
> Run the testcase.
>
>
> Thanks,
>
> Cong Wang
>
>
Re: Review Request 31504: Add a basic filter to match all packets
Posted by Cong Wang <cw...@twopensource.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31504/
-----------------------------------------------------------
(Updated May 7, 2015, 11:20 p.m.)
Review request for mesos, Chi Zhang, Ian Downes, and Jie Yu.
Changes
-------
Address review comments
Bugs: MESOS-2422
https://issues.apache.org/jira/browse/MESOS-2422
Repository: mesos
Description
-------
We need a default filter which has the lowest priority and can match all packets that are not matched by the previous filters, so that no packet will escape (otherwise it would be dropped by fq_codel). Because basic filter can accept protocol as a parameter, the arp filter which is based on basic can move to this as well.
Diffs (updated)
-----
src/Makefile.am 93c7c8a807a33ab639be6289535bbd32022aa85b
src/linux/routing/filter/basic.hpp PRE-CREATION
src/linux/routing/filter/basic.cpp PRE-CREATION
src/tests/routing_tests.cpp ce583b59bf9fb2ef855aa82ab6083ea11b138e55
Diff: https://reviews.apache.org/r/31504/diff/
Testing
-------
Run the testcase.
Thanks,
Cong Wang
Re: Review Request 31504: Add a basic filter to match all packets
Posted by Cong Wang <cw...@twopensource.com>.
> On May 7, 2015, 10:03 p.m., Jie Yu wrote:
> > src/linux/routing/filter/basic.cpp, lines 95-98
> > <https://reviews.apache.org/r/31504/diff/5/?file=950511#file950511line95>
> >
> > Should you pass a `protocol` as well to the `exists` function?
> >
> > ```
> > Try<bool> exists(link, parent, uint16_t protocol)
> > {
> > return internal::exists(link, parent, Classifier(protocol));
> > }
> > ```
That is what I plan to do when we move ARP filter on top of basic filter, so before we do that we don't need to pass protocol?
- Cong
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31504/#review82918
-----------------------------------------------------------
On May 5, 2015, 6:37 p.m., Cong Wang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/31504/
> -----------------------------------------------------------
>
> (Updated May 5, 2015, 6:37 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
> -------
>
> We need a default filter which has the lowest priority and can match all packets that are not matched by the previous filters, so that no packet will escape (otherwise it would be dropped by fq_codel). Because basic filter can accept protocol as a parameter, the arp filter which is based on basic can move to this as well.
>
>
> Diffs
> -----
>
> src/Makefile.am 93c7c8a807a33ab639be6289535bbd32022aa85b
> src/linux/routing/filter/basic.hpp PRE-CREATION
> src/linux/routing/filter/basic.cpp PRE-CREATION
> src/tests/routing_tests.cpp ce583b59bf9fb2ef855aa82ab6083ea11b138e55
>
> Diff: https://reviews.apache.org/r/31504/diff/
>
>
> Testing
> -------
>
> Run the testcase.
>
>
> Thanks,
>
> Cong Wang
>
>
Re: Review Request 31504: Add a basic filter to match all packets
Posted by Jie Yu <yu...@gmail.com>.
> On May 7, 2015, 10:03 p.m., Jie Yu wrote:
> > src/linux/routing/filter/basic.cpp, lines 95-98
> > <https://reviews.apache.org/r/31504/diff/5/?file=950511#file950511line95>
> >
> > Should you pass a `protocol` as well to the `exists` function?
> >
> > ```
> > Try<bool> exists(link, parent, uint16_t protocol)
> > {
> > return internal::exists(link, parent, Classifier(protocol));
> > }
> > ```
>
> Cong Wang wrote:
> That is what I plan to do when we move ARP filter on top of basic filter, so before we do that we don't need to pass protocol?
IC, but that's pretty straight forward to do to me. So probably just do that in this patch?
- Jie
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31504/#review82918
-----------------------------------------------------------
On May 5, 2015, 6:37 p.m., Cong Wang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/31504/
> -----------------------------------------------------------
>
> (Updated May 5, 2015, 6:37 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
> -------
>
> We need a default filter which has the lowest priority and can match all packets that are not matched by the previous filters, so that no packet will escape (otherwise it would be dropped by fq_codel). Because basic filter can accept protocol as a parameter, the arp filter which is based on basic can move to this as well.
>
>
> Diffs
> -----
>
> src/Makefile.am 93c7c8a807a33ab639be6289535bbd32022aa85b
> src/linux/routing/filter/basic.hpp PRE-CREATION
> src/linux/routing/filter/basic.cpp PRE-CREATION
> src/tests/routing_tests.cpp ce583b59bf9fb2ef855aa82ab6083ea11b138e55
>
> Diff: https://reviews.apache.org/r/31504/diff/
>
>
> Testing
> -------
>
> Run the testcase.
>
>
> Thanks,
>
> Cong Wang
>
>
Re: Review Request 31504: Add a basic filter to match all packets
Posted by Jie Yu <yu...@gmail.com>.
> On May 7, 2015, 10:03 p.m., Jie Yu wrote:
> > src/linux/routing/filter/basic.cpp, lines 95-98
> > <https://reviews.apache.org/r/31504/diff/5/?file=950511#file950511line95>
> >
> > Should you pass a `protocol` as well to the `exists` function?
> >
> > ```
> > Try<bool> exists(link, parent, uint16_t protocol)
> > {
> > return internal::exists(link, parent, Classifier(protocol));
> > }
> > ```
>
> Cong Wang wrote:
> That is what I plan to do when we move ARP filter on top of basic filter, so before we do that we don't need to pass protocol?
>
> Jie Yu wrote:
> IC, but that's pretty straight forward to do to me. So probably just do that in this patch?
Or you have the next patch ready and I'll commit both of them togeter.
- Jie
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31504/#review82918
-----------------------------------------------------------
On May 5, 2015, 6:37 p.m., Cong Wang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/31504/
> -----------------------------------------------------------
>
> (Updated May 5, 2015, 6:37 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
> -------
>
> We need a default filter which has the lowest priority and can match all packets that are not matched by the previous filters, so that no packet will escape (otherwise it would be dropped by fq_codel). Because basic filter can accept protocol as a parameter, the arp filter which is based on basic can move to this as well.
>
>
> Diffs
> -----
>
> src/Makefile.am 93c7c8a807a33ab639be6289535bbd32022aa85b
> src/linux/routing/filter/basic.hpp PRE-CREATION
> src/linux/routing/filter/basic.cpp PRE-CREATION
> src/tests/routing_tests.cpp ce583b59bf9fb2ef855aa82ab6083ea11b138e55
>
> Diff: https://reviews.apache.org/r/31504/diff/
>
>
> Testing
> -------
>
> Run the testcase.
>
>
> Thanks,
>
> Cong Wang
>
>
Re: Review Request 31504: Add a basic filter to match all packets
Posted by Cong Wang <cw...@twopensource.com>.
> On May 7, 2015, 10:03 p.m., Jie Yu wrote:
> > src/linux/routing/filter/basic.cpp, lines 95-98
> > <https://reviews.apache.org/r/31504/diff/5/?file=950511#file950511line95>
> >
> > Should you pass a `protocol` as well to the `exists` function?
> >
> > ```
> > Try<bool> exists(link, parent, uint16_t protocol)
> > {
> > return internal::exists(link, parent, Classifier(protocol));
> > }
> > ```
>
> Cong Wang wrote:
> That is what I plan to do when we move ARP filter on top of basic filter, so before we do that we don't need to pass protocol?
>
> Jie Yu wrote:
> IC, but that's pretty straight forward to do to me. So probably just do that in this patch?
>
> Jie Yu wrote:
> Or you have the next patch ready and I'll commit both of them togeter.
Yes, a following patch is ready for review. So I just add protocol there.
- Cong
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31504/#review82918
-----------------------------------------------------------
On May 5, 2015, 6:37 p.m., Cong Wang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/31504/
> -----------------------------------------------------------
>
> (Updated May 5, 2015, 6:37 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
> -------
>
> We need a default filter which has the lowest priority and can match all packets that are not matched by the previous filters, so that no packet will escape (otherwise it would be dropped by fq_codel). Because basic filter can accept protocol as a parameter, the arp filter which is based on basic can move to this as well.
>
>
> Diffs
> -----
>
> src/Makefile.am 93c7c8a807a33ab639be6289535bbd32022aa85b
> src/linux/routing/filter/basic.hpp PRE-CREATION
> src/linux/routing/filter/basic.cpp PRE-CREATION
> src/tests/routing_tests.cpp ce583b59bf9fb2ef855aa82ab6083ea11b138e55
>
> Diff: https://reviews.apache.org/r/31504/diff/
>
>
> Testing
> -------
>
> Run the testcase.
>
>
> Thanks,
>
> Cong Wang
>
>
Re: Review Request 31504: Add a basic filter to match all packets
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31504/#review82918
-----------------------------------------------------------
src/linux/routing/filter/basic.hpp
<https://reviews.apache.org/r/31504/#comment133751>
s/struct/class
Make the member field `procotol_` private and the rest `public`.
src/linux/routing/filter/basic.hpp
<https://reviews.apache.org/r/31504/#comment133750>
We don't use const ref for primitive types (e.g., int, double, etc.).
```
explicit Classifier(uint16_t _protocol)
```
src/linux/routing/filter/basic.hpp
<https://reviews.apache.org/r/31504/#comment133752>
Please adjust the comments.
"set the classid for packets."
src/linux/routing/filter/basic.cpp
<https://reviews.apache.org/r/31504/#comment133764>
Should you pass a `protocol` as well to the `exists` function?
```
Try<bool> exists(link, parent, uint16_t protocol)
{
return internal::exists(link, parent, Classifier(protocol));
}
```
src/linux/routing/filter/basic.cpp
<https://reviews.apache.org/r/31504/#comment133765>
Ditto. Please pass in a `protocol`
src/linux/routing/filter/basic.cpp
<https://reviews.apache.org/r/31504/#comment133766>
Ditto here.
- Jie Yu
On May 5, 2015, 6:37 p.m., Cong Wang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/31504/
> -----------------------------------------------------------
>
> (Updated May 5, 2015, 6:37 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
> -------
>
> We need a default filter which has the lowest priority and can match all packets that are not matched by the previous filters, so that no packet will escape (otherwise it would be dropped by fq_codel). Because basic filter can accept protocol as a parameter, the arp filter which is based on basic can move to this as well.
>
>
> Diffs
> -----
>
> src/Makefile.am 93c7c8a807a33ab639be6289535bbd32022aa85b
> src/linux/routing/filter/basic.hpp PRE-CREATION
> src/linux/routing/filter/basic.cpp PRE-CREATION
> src/tests/routing_tests.cpp ce583b59bf9fb2ef855aa82ab6083ea11b138e55
>
> Diff: https://reviews.apache.org/r/31504/diff/
>
>
> Testing
> -------
>
> Run the testcase.
>
>
> Thanks,
>
> Cong Wang
>
>
Re: Review Request 31504: Add a basic filter to match all packets
Posted by Cong Wang <cw...@twopensource.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31504/
-----------------------------------------------------------
(Updated May 5, 2015, 6:37 p.m.)
Review request for mesos, Chi Zhang, Ian Downes, and Jie Yu.
Changes
-------
Address review comments
Bugs: MESOS-2422
https://issues.apache.org/jira/browse/MESOS-2422
Repository: mesos
Description
-------
We need a default filter which has the lowest priority and can match all packets that are not matched by the previous filters, so that no packet will escape (otherwise it would be dropped by fq_codel). Because basic filter can accept protocol as a parameter, the arp filter which is based on basic can move to this as well.
Diffs (updated)
-----
src/Makefile.am 93c7c8a807a33ab639be6289535bbd32022aa85b
src/linux/routing/filter/basic.hpp PRE-CREATION
src/linux/routing/filter/basic.cpp PRE-CREATION
src/tests/routing_tests.cpp ce583b59bf9fb2ef855aa82ab6083ea11b138e55
Diff: https://reviews.apache.org/r/31504/diff/
Testing
-------
Run the testcase.
Thanks,
Cong Wang