You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@mesos.apache.org by Chi Zhang <ch...@gmail.com> on 2014/06/16 21:02:00 UTC
Review Request 22593: added interfaces to find host public and loopback
interface
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/22593/
-----------------------------------------------------------
Review request for mesos, Ian Downes, Jie Yu, Vinod Kone, and Cong Wang.
Repository: mesos-git
Description
-------
Routing: added interfaces to find the name of host public and loopback device.
Diffs
-----
3rdparty/libprocess/3rdparty/stout/include/stout/net.hpp 97215af
src/linux/routing/link/link.hpp 4264a1e
src/linux/routing/link/link.cpp ebcd119
src/tests/routing_tests.cpp 0e8f61a
Diff: https://reviews.apache.org/r/22593/diff/
Testing
-------
make check
Thanks,
Chi Zhang
Re: Review Request 22593: added interfaces to find host public and loopback
interface
Posted by Chi Zhang <ch...@gmail.com>.
> On June 16, 2014, 11:24 p.m., Jie Yu wrote:
> > src/linux/routing/link/link.cpp, line 103
> > <https://reviews.apache.org/r/22593/diff/2/?file=610950#file610950line103>
> >
> > I think we should return Error if 'test' is an error or none, instead of moving on silently.
> >
> > if (test.isError()) {
> > return Error(...);
> > } else if (test.isNone()) {
> > return Error(...);
> > } else {
> > return link;
> > }
I kept the isError() check but dropped the isNone() check since it's possible for a link to be safely removed after net::links is called, before isNone() is called.
- Chi
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/22593/#review45857
-----------------------------------------------------------
On June 16, 2014, 11:07 p.m., Chi Zhang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/22593/
> -----------------------------------------------------------
>
> (Updated June 16, 2014, 11:07 p.m.)
>
>
> Review request for mesos, Ian Downes, Jie Yu, Nikita Vetoshkin, Vinod Kone, and Cong Wang.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Routing: added interfaces to find the name of host public and loopback device.
>
>
> Diffs
> -----
>
> src/linux/routing/link/link.hpp 4264a1e
> src/linux/routing/link/link.cpp ebcd119
> src/tests/routing_tests.cpp 0e8f61a
>
> Diff: https://reviews.apache.org/r/22593/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Chi Zhang
>
>
Re: Review Request 22593: added interfaces to find host public and loopback
interface
Posted by Jie Yu <yu...@gmail.com>.
> On June 16, 2014, 11:24 p.m., Jie Yu wrote:
> > src/linux/routing/link/link.cpp, line 103
> > <https://reviews.apache.org/r/22593/diff/2/?file=610950#file610950line103>
> >
> > I think we should return Error if 'test' is an error or none, instead of moving on silently.
> >
> > if (test.isError()) {
> > return Error(...);
> > } else if (test.isNone()) {
> > return Error(...);
> > } else {
> > return link;
> > }
>
> Chi Zhang wrote:
> I kept the isError() check but dropped the isNone() check since it's possible for a link to be safely removed after net::links is called, before isNone() is called.
Why? What about the operator remove the link using command in between? Do you want to move on silently or returns an error?
- Jie
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/22593/#review45857
-----------------------------------------------------------
On June 16, 2014, 11:07 p.m., Chi Zhang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/22593/
> -----------------------------------------------------------
>
> (Updated June 16, 2014, 11:07 p.m.)
>
>
> Review request for mesos, Ian Downes, Jie Yu, Nikita Vetoshkin, Vinod Kone, and Cong Wang.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Routing: added interfaces to find the name of host public and loopback device.
>
>
> Diffs
> -----
>
> src/linux/routing/link/link.hpp 4264a1e
> src/linux/routing/link/link.cpp ebcd119
> src/tests/routing_tests.cpp 0e8f61a
>
> Diff: https://reviews.apache.org/r/22593/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Chi Zhang
>
>
Re: Review Request 22593: added interfaces to find host public and loopback
interface
Posted by Chi Zhang <ch...@gmail.com>.
> On June 16, 2014, 11:24 p.m., Jie Yu wrote:
> > src/linux/routing/link/link.cpp, line 103
> > <https://reviews.apache.org/r/22593/diff/2/?file=610950#file610950line103>
> >
> > I think we should return Error if 'test' is an error or none, instead of moving on silently.
> >
> > if (test.isError()) {
> > return Error(...);
> > } else if (test.isNone()) {
> > return Error(...);
> > } else {
> > return link;
> > }
>
> Chi Zhang wrote:
> I kept the isError() check but dropped the isNone() check since it's possible for a link to be safely removed after net::links is called, before isNone() is called.
>
> Jie Yu wrote:
> Why? What about the operator remove the link using command in between? Do you want to move on silently or returns an error?
I think that scenario is not an error, since we only care about lo in this context. say net::links cached some vethx, which gets removed while we are in the loop. There is nothing wrong with that for the purpose of locating lo, is it?
- Chi
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/22593/#review45857
-----------------------------------------------------------
On June 16, 2014, 11:56 p.m., Chi Zhang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/22593/
> -----------------------------------------------------------
>
> (Updated June 16, 2014, 11:56 p.m.)
>
>
> Review request for mesos, Ian Downes, Jie Yu, Nikita Vetoshkin, Vinod Kone, and Cong Wang.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Routing: added interfaces to find the name of host public and loopback device.
>
>
> Diffs
> -----
>
> src/linux/routing/link/link.hpp 4264a1e
> src/linux/routing/link/link.cpp ebcd119
> src/tests/routing_tests.cpp 0e8f61a
>
> Diff: https://reviews.apache.org/r/22593/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Chi Zhang
>
>
Re: Review Request 22593: added interfaces to find host public and loopback
interface
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/22593/#review45857
-----------------------------------------------------------
Ship it!
src/linux/routing/link/link.cpp
<https://reviews.apache.org/r/22593/#comment80881>
Doesn't seem to be addressed?
src/linux/routing/link/link.cpp
<https://reviews.apache.org/r/22593/#comment80880>
return Error(
rule.link() + " is in the routing table but not in the system");
to be consistent with above.
src/linux/routing/link/link.cpp
<https://reviews.apache.org/r/22593/#comment80876>
I think we should return Error if 'test' is an error or none, instead of moving on silently.
if (test.isError()) {
return Error(...);
} else if (test.isNone()) {
return Error(...);
} else {
return link;
}
- Jie Yu
On June 16, 2014, 11:07 p.m., Chi Zhang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/22593/
> -----------------------------------------------------------
>
> (Updated June 16, 2014, 11:07 p.m.)
>
>
> Review request for mesos, Ian Downes, Jie Yu, Nikita Vetoshkin, Vinod Kone, and Cong Wang.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Routing: added interfaces to find the name of host public and loopback device.
>
>
> Diffs
> -----
>
> src/linux/routing/link/link.hpp 4264a1e
> src/linux/routing/link/link.cpp ebcd119
> src/tests/routing_tests.cpp 0e8f61a
>
> Diff: https://reviews.apache.org/r/22593/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Chi Zhang
>
>
Re: Review Request 22593: added interfaces to find host public and loopback
interface
Posted by Chi Zhang <ch...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/22593/
-----------------------------------------------------------
(Updated June 16, 2014, 11:56 p.m.)
Review request for mesos, Ian Downes, Jie Yu, Nikita Vetoshkin, Vinod Kone, and Cong Wang.
Changes
-------
addressed comments.
Repository: mesos-git
Description
-------
Routing: added interfaces to find the name of host public and loopback device.
Diffs (updated)
-----
src/linux/routing/link/link.hpp 4264a1e
src/linux/routing/link/link.cpp ebcd119
src/tests/routing_tests.cpp 0e8f61a
Diff: https://reviews.apache.org/r/22593/diff/
Testing
-------
make check
Thanks,
Chi Zhang
Re: Review Request 22593: added interfaces to find host public and loopback
interface
Posted by Chi Zhang <ch...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/22593/
-----------------------------------------------------------
(Updated June 16, 2014, 11:07 p.m.)
Review request for mesos, Ian Downes, Jie Yu, Nikita Vetoshkin, Vinod Kone, and Cong Wang.
Changes
-------
addressed comments.
Repository: mesos-git
Description
-------
Routing: added interfaces to find the name of host public and loopback device.
Diffs (updated)
-----
src/linux/routing/link/link.hpp 4264a1e
src/linux/routing/link/link.cpp ebcd119
src/tests/routing_tests.cpp 0e8f61a
Diff: https://reviews.apache.org/r/22593/diff/
Testing
-------
make check
Thanks,
Chi Zhang
Re: Review Request 22593: added interfaces to find host public and loopback
interface
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/22593/#review45807
-----------------------------------------------------------
src/tests/routing_tests.cpp
<https://reviews.apache.org/r/22593/#comment80793>
Also, can you move these two tests below the LinkExists test because they are part of the link tests.
- Jie Yu
On June 16, 2014, 7:01 p.m., Chi Zhang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/22593/
> -----------------------------------------------------------
>
> (Updated June 16, 2014, 7:01 p.m.)
>
>
> Review request for mesos, Ian Downes, Jie Yu, Vinod Kone, and Cong Wang.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Routing: added interfaces to find the name of host public and loopback device.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/3rdparty/stout/include/stout/net.hpp 97215af
> src/linux/routing/link/link.hpp 4264a1e
> src/linux/routing/link/link.cpp ebcd119
> src/tests/routing_tests.cpp 0e8f61a
>
> Diff: https://reviews.apache.org/r/22593/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Chi Zhang
>
>
Re: Review Request 22593: added interfaces to find host public and loopback
interface
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/22593/#review45801
-----------------------------------------------------------
3rdparty/libprocess/3rdparty/stout/include/stout/net.hpp
<https://reviews.apache.org/r/22593/#comment80782>
See comments below. You can remove that.
src/linux/routing/link/link.hpp
<https://reviews.apache.org/r/22593/#comment80783>
Should be:
"the first interface in the routing table that has an empty 'destination'"?
Also, comment on in what situation, None will be returned?
src/linux/routing/link/link.hpp
<https://reviews.apache.org/r/22593/#comment80784>
See comments below. Adjust the comments here.
src/linux/routing/link/link.cpp
<https://reviews.apache.org/r/22593/#comment80786>
Please move this line right below #include "linux/routing/internal.hpp"
src/linux/routing/link/link.cpp
<https://reviews.apache.org/r/22593/#comment80787>
We probably want to return an Error here because the link is supposed to be there.
src/linux/routing/link/link.cpp
<https://reviews.apache.org/r/22593/#comment80781>
Ditto. You should be able to use link::internal::test(IFF_LOOPBACK) to test if a 'link' is a loopback device.
src/linux/routing/link/link.cpp
<https://reviews.apache.org/r/22593/#comment80788>
Return an Error here.
src/tests/routing_tests.cpp
<https://reviews.apache.org/r/22593/#comment80789>
What if eth0 is None?
src/tests/routing_tests.cpp
<https://reviews.apache.org/r/22593/#comment80790>
Ditto.
- Jie Yu
On June 16, 2014, 7:01 p.m., Chi Zhang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/22593/
> -----------------------------------------------------------
>
> (Updated June 16, 2014, 7:01 p.m.)
>
>
> Review request for mesos, Ian Downes, Jie Yu, Vinod Kone, and Cong Wang.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Routing: added interfaces to find the name of host public and loopback device.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/3rdparty/stout/include/stout/net.hpp 97215af
> src/linux/routing/link/link.hpp 4264a1e
> src/linux/routing/link/link.cpp ebcd119
> src/tests/routing_tests.cpp 0e8f61a
>
> Diff: https://reviews.apache.org/r/22593/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Chi Zhang
>
>
Re: Review Request 22593: added interfaces to find host public and loopback
interface
Posted by Chi Zhang <ch...@gmail.com>.
> On June 16, 2014, 7:23 p.m., Nikita Vetoshkin wrote:
> > src/linux/routing/link/link.cpp, line 102
> > <https://reviews.apache.org/r/22593/diff/1/?file=609702#file609702line102>
> >
> > "man netdevice" mentions IFF_LOOPBACK flag whith should be set in ifa_flags attribute of struct ifaddrs. Seems like it is right what we need here, doesn't it?
thanks. fixed.
- Chi
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/22593/#review45796
-----------------------------------------------------------
On June 16, 2014, 11:07 p.m., Chi Zhang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/22593/
> -----------------------------------------------------------
>
> (Updated June 16, 2014, 11:07 p.m.)
>
>
> Review request for mesos, Ian Downes, Jie Yu, Nikita Vetoshkin, Vinod Kone, and Cong Wang.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Routing: added interfaces to find the name of host public and loopback device.
>
>
> Diffs
> -----
>
> src/linux/routing/link/link.hpp 4264a1e
> src/linux/routing/link/link.cpp ebcd119
> src/tests/routing_tests.cpp 0e8f61a
>
> Diff: https://reviews.apache.org/r/22593/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Chi Zhang
>
>
Re: Review Request 22593: added interfaces to find host public and loopback
interface
Posted by Nikita Vetoshkin <ni...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/22593/#review45796
-----------------------------------------------------------
src/linux/routing/link/link.cpp
<https://reviews.apache.org/r/22593/#comment80776>
"man netdevice" mentions IFF_LOOPBACK flag whith should be set in ifa_flags attribute of struct ifaddrs. Seems like it is right what we need here, doesn't it?
- Nikita Vetoshkin
On June 16, 2014, 7:01 p.m., Chi Zhang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/22593/
> -----------------------------------------------------------
>
> (Updated June 16, 2014, 7:01 p.m.)
>
>
> Review request for mesos, Ian Downes, Jie Yu, Vinod Kone, and Cong Wang.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Routing: added interfaces to find the name of host public and loopback device.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/3rdparty/stout/include/stout/net.hpp 97215af
> src/linux/routing/link/link.hpp 4264a1e
> src/linux/routing/link/link.cpp ebcd119
> src/tests/routing_tests.cpp 0e8f61a
>
> Diff: https://reviews.apache.org/r/22593/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Chi Zhang
>
>