You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@mesos.apache.org by Jie Yu <yu...@gmail.com> on 2014/04/14 01:39:36 UTC
Review Request 20292: Added API for managing links.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/20292/
-----------------------------------------------------------
Review request for mesos, Benjamin Hindman, Chi Zhang, Ian Downes, Vinod Kone, and Cong Wang.
Repository: mesos-git
Description
-------
This is Part 1 of the linux routing library.
Diffs
-----
configure.ac c1de6d7eec1fc740c031bebb21a662e654328943
src/Makefile.am 560b4c77a4e44ef1833b3b4afd95bb8040c9f229
src/linux/routing/internal.hpp PRE-CREATION
src/linux/routing/link/internal.hpp PRE-CREATION
src/linux/routing/link/link.hpp PRE-CREATION
src/linux/routing/link/link.cpp PRE-CREATION
src/tests/routing_tests.cpp PRE-CREATION
Diff: https://reviews.apache.org/r/20292/diff/
Testing
-------
make check
sudo make check
Thanks,
Jie Yu
Re: Review Request 20292: Added API for managing links.
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/20292/#review40234
-----------------------------------------------------------
src/Makefile.am
<https://reviews.apache.org/r/20292/#comment73174>
what about the header files?
- Vinod Kone
On April 13, 2014, 11:39 p.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/20292/
> -----------------------------------------------------------
>
> (Updated April 13, 2014, 11:39 p.m.)
>
>
> Review request for mesos, Benjamin Hindman, Chi Zhang, Ian Downes, Vinod Kone, and Cong Wang.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> This is Part 1 of the linux routing library.
>
>
> Diffs
> -----
>
> configure.ac c1de6d7eec1fc740c031bebb21a662e654328943
> src/Makefile.am 560b4c77a4e44ef1833b3b4afd95bb8040c9f229
> src/linux/routing/internal.hpp PRE-CREATION
> src/linux/routing/link/internal.hpp PRE-CREATION
> src/linux/routing/link/link.hpp PRE-CREATION
> src/linux/routing/link/link.cpp PRE-CREATION
> src/tests/routing_tests.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/20292/diff/
>
>
> Testing
> -------
>
> make check
> sudo make check
>
>
> Thanks,
>
> Jie Yu
>
>
Re: Review Request 20292: Added API for managing links.
Posted by Jie Yu <yu...@gmail.com>.
> On May 13, 2014, 9:40 p.m., Dominic Hamon wrote:
> > src/tests/routing_tests.cpp, line 55
> > <https://reviews.apache.org/r/20292/diff/6/?file=579305#file579305line55>
> >
> > has this been a problem? is some test not calling TearDown?
Yeah, for example, user ctrl+c the test, or kill -9 the test:)
- Jie
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/20292/#review42892
-----------------------------------------------------------
On May 13, 2014, 12:33 a.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/20292/
> -----------------------------------------------------------
>
> (Updated May 13, 2014, 12:33 a.m.)
>
>
> Review request for mesos, Benjamin Hindman, Chi Zhang, Ian Downes, Vinod Kone, and Cong Wang.
>
>
> Bugs: MESOS-1228
> https://issues.apache.org/jira/browse/MESOS-1228
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> This is Part 1 of the linux routing library.
>
>
> Diffs
> -----
>
> configure.ac 69acaa1
> src/Makefile.am 812ad2c
> src/linux/routing/internal.hpp PRE-CREATION
> src/linux/routing/link/internal.hpp PRE-CREATION
> src/linux/routing/link/link.hpp PRE-CREATION
> src/linux/routing/link/link.cpp PRE-CREATION
> src/tests/routing_tests.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/20292/diff/
>
>
> Testing
> -------
>
> make check
> sudo make check
>
>
> Thanks,
>
> Jie Yu
>
>
Re: Review Request 20292: Added API for managing links.
Posted by Dominic Hamon <dh...@twopensource.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/20292/#review42892
-----------------------------------------------------------
src/linux/routing/link/link.cpp
<https://reviews.apache.org/r/20292/#comment76809>
i'd simplify this to:
if (link.isError()) {
return Error(link.error());
}
return link.isSome();
src/linux/routing/link/link.cpp
<https://reviews.apache.org/r/20292/#comment76810>
this could be:
if (err == -NLE_EXIST) {
return false;
}
return Error(nl_geterror(err));
src/linux/routing/link/link.cpp
<https://reviews.apache.org/r/20292/#comment76811>
this block is repeated a few times and might be worth abstracting.
src/tests/routing_tests.cpp
<https://reviews.apache.org/r/20292/#comment76812>
has this been a problem? is some test not calling TearDown?
- Dominic Hamon
On May 12, 2014, 5:33 p.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/20292/
> -----------------------------------------------------------
>
> (Updated May 12, 2014, 5:33 p.m.)
>
>
> Review request for mesos, Benjamin Hindman, Chi Zhang, Ian Downes, Vinod Kone, and Cong Wang.
>
>
> Bugs: MESOS-1228
> https://issues.apache.org/jira/browse/MESOS-1228
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> This is Part 1 of the linux routing library.
>
>
> Diffs
> -----
>
> configure.ac 69acaa1
> src/Makefile.am 812ad2c
> src/linux/routing/internal.hpp PRE-CREATION
> src/linux/routing/link/internal.hpp PRE-CREATION
> src/linux/routing/link/link.hpp PRE-CREATION
> src/linux/routing/link/link.cpp PRE-CREATION
> src/tests/routing_tests.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/20292/diff/
>
>
> Testing
> -------
>
> make check
> sudo make check
>
>
> Thanks,
>
> Jie Yu
>
>
Re: Review Request 20292: Added API for managing links.
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/20292/
-----------------------------------------------------------
(Updated May 15, 2014, 12:41 a.m.)
Review request for mesos, Benjamin Hindman, Chi Zhang, Ian Downes, Vinod Kone, and Cong Wang.
Changes
-------
Rebased.
Bugs: MESOS-1228
https://issues.apache.org/jira/browse/MESOS-1228
Repository: mesos-git
Description
-------
This is Part 1 of the linux routing library.
Diffs (updated)
-----
configure.ac 69acaa1
src/Makefile.am 12374c4
src/linux/routing/internal.hpp PRE-CREATION
src/linux/routing/link/internal.hpp PRE-CREATION
src/linux/routing/link/link.hpp PRE-CREATION
src/linux/routing/link/link.cpp PRE-CREATION
src/tests/routing_tests.cpp PRE-CREATION
Diff: https://reviews.apache.org/r/20292/diff/
Testing
-------
make check
sudo make check
Thanks,
Jie Yu
Re: Review Request 20292: Added API for managing links.
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/20292/
-----------------------------------------------------------
(Updated May 14, 2014, 9:51 p.m.)
Review request for mesos, Benjamin Hindman, Chi Zhang, Ian Downes, Vinod Kone, and Cong Wang.
Changes
-------
Review comments. Rebased.
Bugs: MESOS-1228
https://issues.apache.org/jira/browse/MESOS-1228
Repository: mesos-git
Description
-------
This is Part 1 of the linux routing library.
Diffs (updated)
-----
configure.ac 69acaa1
src/Makefile.am 12374c4
src/linux/routing/internal.hpp PRE-CREATION
src/linux/routing/link/internal.hpp PRE-CREATION
src/linux/routing/link/link.hpp PRE-CREATION
src/linux/routing/link/link.cpp PRE-CREATION
src/tests/routing_tests.cpp PRE-CREATION
Diff: https://reviews.apache.org/r/20292/diff/
Testing
-------
make check
sudo make check
Thanks,
Jie Yu
Re: Review Request 20292: Added API for managing links.
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/20292/
-----------------------------------------------------------
(Updated May 13, 2014, 12:33 a.m.)
Review request for mesos, Benjamin Hindman, Chi Zhang, Ian Downes, Vinod Kone, and Cong Wang.
Changes
-------
Renamed setMac -> setMAC to be consistent.
Fixed a problem in setMAC so that it also works for loopback device.
Bugs: MESOS-1228
https://issues.apache.org/jira/browse/MESOS-1228
Repository: mesos-git
Description
-------
This is Part 1 of the linux routing library.
Diffs (updated)
-----
configure.ac 69acaa1
src/Makefile.am 812ad2c
src/linux/routing/internal.hpp PRE-CREATION
src/linux/routing/link/internal.hpp PRE-CREATION
src/linux/routing/link/link.hpp PRE-CREATION
src/linux/routing/link/link.cpp PRE-CREATION
src/tests/routing_tests.cpp PRE-CREATION
Diff: https://reviews.apache.org/r/20292/diff/
Testing
-------
make check
sudo make check
Thanks,
Jie Yu
Re: Review Request 20292: Added API for managing links.
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/20292/
-----------------------------------------------------------
(Updated April 28, 2014, 4:54 p.m.)
Review request for mesos, Benjamin Hindman, Chi Zhang, Ian Downes, Vinod Kone, and Cong Wang.
Changes
-------
Rebased. Not for review.
Bugs: MESOS-1228
https://issues.apache.org/jira/browse/MESOS-1228
Repository: mesos-git
Description
-------
This is Part 1 of the linux routing library.
Diffs (updated)
-----
configure.ac 4967fe4
src/Makefile.am d2e006d
src/linux/routing/internal.hpp PRE-CREATION
src/linux/routing/link/internal.hpp PRE-CREATION
src/linux/routing/link/link.hpp PRE-CREATION
src/linux/routing/link/link.cpp PRE-CREATION
src/tests/routing_tests.cpp PRE-CREATION
Diff: https://reviews.apache.org/r/20292/diff/
Testing
-------
make check
sudo make check
Thanks,
Jie Yu
Re: Review Request 20292: Added API for managing links.
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/20292/
-----------------------------------------------------------
(Updated April 22, 2014, 6:02 a.m.)
Review request for mesos, Benjamin Hindman, Chi Zhang, Ian Downes, Vinod Kone, and Cong Wang.
Changes
-------
Updated "Bugs".
Bugs: MESOS-1228
https://issues.apache.org/jira/browse/MESOS-1228
Repository: mesos-git
Description
-------
This is Part 1 of the linux routing library.
Diffs
-----
configure.ac c1de6d7
src/Makefile.am a44ea42
src/linux/routing/internal.hpp PRE-CREATION
src/linux/routing/link/internal.hpp PRE-CREATION
src/linux/routing/link/link.hpp PRE-CREATION
src/linux/routing/link/link.cpp PRE-CREATION
src/tests/routing_tests.cpp PRE-CREATION
Diff: https://reviews.apache.org/r/20292/diff/
Testing
-------
make check
sudo make check
Thanks,
Jie Yu
Re: Review Request 20292: Added API for managing links.
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/20292/
-----------------------------------------------------------
(Updated April 22, 2014, 5:39 a.m.)
Review request for mesos, Benjamin Hindman, Chi Zhang, Ian Downes, Vinod Kone, and Cong Wang.
Changes
-------
Rebased.
Repository: mesos-git
Description
-------
This is Part 1 of the linux routing library.
Diffs (updated)
-----
configure.ac c1de6d7
src/Makefile.am a44ea42
src/linux/routing/internal.hpp PRE-CREATION
src/linux/routing/link/internal.hpp PRE-CREATION
src/linux/routing/link/link.hpp PRE-CREATION
src/linux/routing/link/link.cpp PRE-CREATION
src/tests/routing_tests.cpp PRE-CREATION
Diff: https://reviews.apache.org/r/20292/diff/
Testing
-------
make check
sudo make check
Thanks,
Jie Yu
Re: Review Request 20292: Added API for managing links.
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/20292/
-----------------------------------------------------------
(Updated April 18, 2014, 7:30 p.m.)
Review request for mesos, Benjamin Hindman, Chi Zhang, Ian Downes, Vinod Kone, and Cong Wang.
Changes
-------
Rebased.
Repository: mesos-git
Description
-------
This is Part 1 of the linux routing library.
Diffs (updated)
-----
configure.ac c1de6d7
src/Makefile.am 9e715bf
src/linux/routing/internal.hpp PRE-CREATION
src/linux/routing/link/internal.hpp PRE-CREATION
src/linux/routing/link/link.hpp PRE-CREATION
src/linux/routing/link/link.cpp PRE-CREATION
src/tests/routing_tests.cpp PRE-CREATION
Diff: https://reviews.apache.org/r/20292/diff/
Testing
-------
make check
sudo make check
Thanks,
Jie Yu
Re: Review Request 20292: Added API for managing links.
Posted by Jie Yu <yu...@gmail.com>.
> On April 15, 2014, 7:01 p.m., Chi Zhang wrote:
> > src/tests/routing_tests.cpp, line 205
> > <https://reviews.apache.org/r/20292/diff/1/?file=555695#file555695line205>
> >
> > set a multicast mac address and verify that kernel rejects?
Added a test.
- Jie
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/20292/#review40434
-----------------------------------------------------------
On May 13, 2014, 12:33 a.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/20292/
> -----------------------------------------------------------
>
> (Updated May 13, 2014, 12:33 a.m.)
>
>
> Review request for mesos, Benjamin Hindman, Chi Zhang, Ian Downes, Vinod Kone, and Cong Wang.
>
>
> Bugs: MESOS-1228
> https://issues.apache.org/jira/browse/MESOS-1228
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> This is Part 1 of the linux routing library.
>
>
> Diffs
> -----
>
> configure.ac 69acaa1
> src/Makefile.am 812ad2c
> src/linux/routing/internal.hpp PRE-CREATION
> src/linux/routing/link/internal.hpp PRE-CREATION
> src/linux/routing/link/link.hpp PRE-CREATION
> src/linux/routing/link/link.cpp PRE-CREATION
> src/tests/routing_tests.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/20292/diff/
>
>
> Testing
> -------
>
> make check
> sudo make check
>
>
> Thanks,
>
> Jie Yu
>
>
Re: Review Request 20292: Added API for managing links.
Posted by Chi Zhang <ch...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/20292/#review40434
-----------------------------------------------------------
src/tests/routing_tests.cpp
<https://reviews.apache.org/r/20292/#comment73436>
set a multicast mac address and verify that kernel rejects?
- Chi Zhang
On April 15, 2014, 5:46 p.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/20292/
> -----------------------------------------------------------
>
> (Updated April 15, 2014, 5:46 p.m.)
>
>
> Review request for mesos, Benjamin Hindman, Chi Zhang, Ian Downes, Vinod Kone, and Cong Wang.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> This is Part 1 of the linux routing library.
>
>
> Diffs
> -----
>
> configure.ac c1de6d7
> src/Makefile.am 560b4c7
> src/linux/routing/internal.hpp PRE-CREATION
> src/linux/routing/link/internal.hpp PRE-CREATION
> src/linux/routing/link/link.hpp PRE-CREATION
> src/linux/routing/link/link.cpp PRE-CREATION
> src/tests/routing_tests.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/20292/diff/
>
>
> Testing
> -------
>
> make check
> sudo make check
>
>
> Thanks,
>
> Jie Yu
>
>
Re: Review Request 20292: Added API for managing links.
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/20292/
-----------------------------------------------------------
(Updated April 15, 2014, 5:46 p.m.)
Review request for mesos, Benjamin Hindman, Chi Zhang, Ian Downes, Vinod Kone, and Cong Wang.
Changes
-------
Review comments.
Repository: mesos-git
Description
-------
This is Part 1 of the linux routing library.
Diffs (updated)
-----
configure.ac c1de6d7
src/Makefile.am 560b4c7
src/linux/routing/internal.hpp PRE-CREATION
src/linux/routing/link/internal.hpp PRE-CREATION
src/linux/routing/link/link.hpp PRE-CREATION
src/linux/routing/link/link.cpp PRE-CREATION
src/tests/routing_tests.cpp PRE-CREATION
Diff: https://reviews.apache.org/r/20292/diff/
Testing
-------
make check
sudo make check
Thanks,
Jie Yu
Re: Review Request 20292: Added API for managing links.
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/20292/
-----------------------------------------------------------
(Updated April 15, 2014, 4:41 a.m.)
Review request for mesos, Benjamin Hindman, Chi Zhang, Ian Downes, Vinod Kone, and Cong Wang.
Changes
-------
Removed submitted patch from "depends on".
Repository: mesos-git
Description
-------
This is Part 1 of the linux routing library.
Diffs
-----
configure.ac c1de6d7eec1fc740c031bebb21a662e654328943
src/Makefile.am 560b4c77a4e44ef1833b3b4afd95bb8040c9f229
src/linux/routing/internal.hpp PRE-CREATION
src/linux/routing/link/internal.hpp PRE-CREATION
src/linux/routing/link/link.hpp PRE-CREATION
src/linux/routing/link/link.cpp PRE-CREATION
src/tests/routing_tests.cpp PRE-CREATION
Diff: https://reviews.apache.org/r/20292/diff/
Testing
-------
make check
sudo make check
Thanks,
Jie Yu
Re: Review Request 20292: Added API for managing links.
Posted by Jie Yu <yu...@gmail.com>.
> On April 14, 2014, 2:27 a.m., Vinod Kone wrote:
> > src/linux/routing/internal.hpp, line 44
> > <https://reviews.apache.org/r/20292/diff/1/?file=555691#file555691line44>
> >
> > 'Managed' seems a bit generic. How about calling this Netlink?
Sure. Will rename it.
- Jie
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/20292/#review40230
-----------------------------------------------------------
On April 13, 2014, 11:39 p.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/20292/
> -----------------------------------------------------------
>
> (Updated April 13, 2014, 11:39 p.m.)
>
>
> Review request for mesos, Benjamin Hindman, Chi Zhang, Ian Downes, Vinod Kone, and Cong Wang.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> This is Part 1 of the linux routing library.
>
>
> Diffs
> -----
>
> configure.ac c1de6d7eec1fc740c031bebb21a662e654328943
> src/Makefile.am 560b4c77a4e44ef1833b3b4afd95bb8040c9f229
> src/linux/routing/internal.hpp PRE-CREATION
> src/linux/routing/link/internal.hpp PRE-CREATION
> src/linux/routing/link/link.hpp PRE-CREATION
> src/linux/routing/link/link.cpp PRE-CREATION
> src/tests/routing_tests.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/20292/diff/
>
>
> Testing
> -------
>
> make check
> sudo make check
>
>
> Thanks,
>
> Jie Yu
>
>
Re: Review Request 20292: Added API for managing links.
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/20292/#review40230
-----------------------------------------------------------
Ship it!
lgtm.
src/linux/routing/internal.hpp
<https://reviews.apache.org/r/20292/#comment73172>
'Managed' seems a bit generic. How about calling this Netlink?
src/linux/routing/internal.hpp
<https://reviews.apache.org/r/20292/#comment73168>
kill new line.
src/linux/routing/link/internal.hpp
<https://reviews.apache.org/r/20292/#comment73169>
this should be after the C headers.
- Vinod Kone
On April 13, 2014, 11:39 p.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/20292/
> -----------------------------------------------------------
>
> (Updated April 13, 2014, 11:39 p.m.)
>
>
> Review request for mesos, Benjamin Hindman, Chi Zhang, Ian Downes, Vinod Kone, and Cong Wang.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> This is Part 1 of the linux routing library.
>
>
> Diffs
> -----
>
> configure.ac c1de6d7eec1fc740c031bebb21a662e654328943
> src/Makefile.am 560b4c77a4e44ef1833b3b4afd95bb8040c9f229
> src/linux/routing/internal.hpp PRE-CREATION
> src/linux/routing/link/internal.hpp PRE-CREATION
> src/linux/routing/link/link.hpp PRE-CREATION
> src/linux/routing/link/link.cpp PRE-CREATION
> src/tests/routing_tests.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/20292/diff/
>
>
> Testing
> -------
>
> make check
> sudo make check
>
>
> Thanks,
>
> Jie Yu
>
>
Re: Review Request 20292: Added API for managing links.
Posted by Ian Downes <ia...@gmail.com>.
> On April 15, 2014, 12:17 a.m., Ian Downes wrote:
> > src/linux/routing/link/internal.hpp, line 89
> > <https://reviews.apache.org/r/20292/diff/1/?file=555692#file555692line89>
> >
> > Why do you need the bitwise & to test equality - shouldn't == suffice?
Okay, my mistake - you just want to see if those flags are set but it's not exclusive.
- Ian
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/20292/#review40283
-----------------------------------------------------------
On April 13, 2014, 11:39 p.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/20292/
> -----------------------------------------------------------
>
> (Updated April 13, 2014, 11:39 p.m.)
>
>
> Review request for mesos, Benjamin Hindman, Chi Zhang, Ian Downes, Vinod Kone, and Cong Wang.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> This is Part 1 of the linux routing library.
>
>
> Diffs
> -----
>
> configure.ac c1de6d7eec1fc740c031bebb21a662e654328943
> src/Makefile.am 560b4c77a4e44ef1833b3b4afd95bb8040c9f229
> src/linux/routing/internal.hpp PRE-CREATION
> src/linux/routing/link/internal.hpp PRE-CREATION
> src/linux/routing/link/link.hpp PRE-CREATION
> src/linux/routing/link/link.cpp PRE-CREATION
> src/tests/routing_tests.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/20292/diff/
>
>
> Testing
> -------
>
> make check
> sudo make check
>
>
> Thanks,
>
> Jie Yu
>
>
Re: Review Request 20292: Added API for managing links.
Posted by Jie Yu <yu...@gmail.com>.
> On April 15, 2014, 12:17 a.m., Ian Downes wrote:
> > src/linux/routing/link/link.cpp, line 114
> > <https://reviews.apache.org/r/20292/diff/1/?file=555694#file555694line114>
> >
> > What does it mean to create a virtual pair of interfaces if pid is None - both are in the same namespace? I thought one of the pair was in the parent's namespace and the other had to be in a different namespace.
No, it's not required. Having the peer in the same namespace can simply the tests a lot (no need to create a child process).
> On April 15, 2014, 12:17 a.m., Ian Downes wrote:
> > src/linux/routing/link/internal.hpp, line 69
> > <https://reviews.apache.org/r/20292/diff/1/?file=555692#file555692line69>
> >
> > Can this not be named "link" with the function argument as "_link" as you have done in test()?
I use the following convention: I use a single letter variable to name any raw libnl pointer.
> On April 15, 2014, 12:17 a.m., Ian Downes wrote:
> > src/linux/routing/link/internal.hpp, line 63
> > <https://reviews.apache.org/r/20292/diff/1/?file=555692#file555692line63>
> >
> > It's not intuitive to read "if (err != 0) return Error()" what about calling it status? "if (status != 0) return Error()" reads better to me.
err == 0 means no error
err != 0 means error
Looks fine to me. Feel lazy to change:) Let me know if you insist!
> On April 15, 2014, 12:17 a.m., Ian Downes wrote:
> > configure.ac, line 711
> > <https://reviews.apache.org/r/20292/diff/1/?file=555689#file555689line711>
> >
> > Are these CFLAGS or CPPFLAGS? You append them to MESOS_CPPFLAGS in the Makefile.
Libnl is written in C, so I prefer to using CFLAGS.
IIUC, CPPFLAGS is for preprocessing and CXXFLAGS is for c++ compiler.
> On April 15, 2014, 12:17 a.m., Ian Downes wrote:
> > src/tests/routing_tests.cpp, line 172
> > <https://reviews.apache.org/r/20292/diff/1/?file=555695#file555695line172>
> >
> > Is this as simple as checking the output of an os::shell() with "ip setns exec ..."?
Manually verified. Will add more sophisticated tests in the following patches.
- Jie
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/20292/#review40283
-----------------------------------------------------------
On April 15, 2014, 4:41 a.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/20292/
> -----------------------------------------------------------
>
> (Updated April 15, 2014, 4:41 a.m.)
>
>
> Review request for mesos, Benjamin Hindman, Chi Zhang, Ian Downes, Vinod Kone, and Cong Wang.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> This is Part 1 of the linux routing library.
>
>
> Diffs
> -----
>
> configure.ac c1de6d7eec1fc740c031bebb21a662e654328943
> src/Makefile.am 560b4c77a4e44ef1833b3b4afd95bb8040c9f229
> src/linux/routing/internal.hpp PRE-CREATION
> src/linux/routing/link/internal.hpp PRE-CREATION
> src/linux/routing/link/link.hpp PRE-CREATION
> src/linux/routing/link/link.cpp PRE-CREATION
> src/tests/routing_tests.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/20292/diff/
>
>
> Testing
> -------
>
> make check
> sudo make check
>
>
> Thanks,
>
> Jie Yu
>
>
Re: Review Request 20292: Added API for managing links.
Posted by Ian Downes <ia...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/20292/#review40283
-----------------------------------------------------------
configure.ac
<https://reviews.apache.org/r/20292/#comment73264>
Are these CFLAGS or CPPFLAGS? You append them to MESOS_CPPFLAGS in the Makefile.
src/linux/routing/link/internal.hpp
<https://reviews.apache.org/r/20292/#comment73304>
s/none/None/?
src/linux/routing/link/internal.hpp
<https://reviews.apache.org/r/20292/#comment73301>
It's not intuitive to read "if (err != 0) return Error()" what about calling it status? "if (status != 0) return Error()" reads better to me.
src/linux/routing/link/internal.hpp
<https://reviews.apache.org/r/20292/#comment73302>
Can this not be named "link" with the function argument as "_link" as you have done in test()?
src/linux/routing/link/internal.hpp
<https://reviews.apache.org/r/20292/#comment73303>
s/none/None/ and everywhere else
src/linux/routing/link/internal.hpp
<https://reviews.apache.org/r/20292/#comment73306>
Why do you need the bitwise & to test equality - shouldn't == suffice?
src/linux/routing/link/internal.hpp
<https://reviews.apache.org/r/20292/#comment73307>
either:
s/interface/interfaces/
or:
s/has/have/
src/linux/routing/link/link.hpp
<https://reviews.apache.org/r/20292/#comment73313>
Document that if pid is None then the new veth is put into the caller's namespace.
src/linux/routing/link/link.hpp
<https://reviews.apache.org/r/20292/#comment73310>
reference
src/linux/routing/link/link.hpp
<https://reviews.apache.org/r/20292/#comment73311>
s/ifindex/ifIndex?
src/linux/routing/link/link.hpp
<https://reviews.apache.org/r/20292/#comment73312>
This fits on one line.
Any reason to not use hashmap?
src/linux/routing/link/link.cpp
<https://reviews.apache.org/r/20292/#comment73315>
reference
src/linux/routing/link/link.cpp
<https://reviews.apache.org/r/20292/#comment73327>
What does it mean to create a virtual pair of interfaces if pid is None - both are in the same namespace? I thought one of the pair was in the parent's namespace and the other had to be in a different namespace.
src/linux/routing/link/link.cpp
<https://reviews.apache.org/r/20292/#comment73319>
ditto
src/tests/routing_tests.cpp
<https://reviews.apache.org/r/20292/#comment73324>
s/LinkIfindex/LinkIfIndex/?
src/tests/routing_tests.cpp
<https://reviews.apache.org/r/20292/#comment73328>
Move the LinkCreate test before the LinkRemove test?
src/tests/routing_tests.cpp
<https://reviews.apache.org/r/20292/#comment73330>
Is this as simple as checking the output of an os::shell() with "ip setns exec ..."?
src/tests/routing_tests.cpp
<https://reviews.apache.org/r/20292/#comment73332>
can you bring up the TEST_PEER_LINK?
src/tests/routing_tests.cpp
<https://reviews.apache.org/r/20292/#comment73333>
can you link::setMac on the TEST_PEER_LINK?
- Ian Downes
On April 13, 2014, 11:39 p.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/20292/
> -----------------------------------------------------------
>
> (Updated April 13, 2014, 11:39 p.m.)
>
>
> Review request for mesos, Benjamin Hindman, Chi Zhang, Ian Downes, Vinod Kone, and Cong Wang.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> This is Part 1 of the linux routing library.
>
>
> Diffs
> -----
>
> configure.ac c1de6d7eec1fc740c031bebb21a662e654328943
> src/Makefile.am 560b4c77a4e44ef1833b3b4afd95bb8040c9f229
> src/linux/routing/internal.hpp PRE-CREATION
> src/linux/routing/link/internal.hpp PRE-CREATION
> src/linux/routing/link/link.hpp PRE-CREATION
> src/linux/routing/link/link.cpp PRE-CREATION
> src/tests/routing_tests.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/20292/diff/
>
>
> Testing
> -------
>
> make check
> sudo make check
>
>
> Thanks,
>
> Jie Yu
>
>