Re: [systemd-devel] [PATCH] networkd: Introduce ipip tunnel
On Mon, Apr 7, 2014 at 6:35 AM, Susant Sahani sus...@redhat.com wrote: I have addressed all your comments. Cool. However I have some queries Please find below. Hm, we can probably reuse some of the existing address parsing functions don't you think? And we should also check the address faimilies here right? I think I can reuse net_parse_inaddr . Yes, sounds like the right thing to do. It will also allow you to force the address family and fail if it is wrong. Hm, I guess these should be _append_in_addr() to get the typesafety right (might need to verify that we are using the right types for this in rtnl-types.c. I am missing something in the code . with the current rtnl code it does not get appended. Could you please give a example. Your follow-up patch does the right thing. The types were sort of wrong in the library, but it would not be noticed by the kernel as NLA_IN_ADDR is equivalent to NLA_U32 for IPv4 addresses. However, it is not for IPv6 addresses, so better to do this explicitly as your next patch suggests. This should be fixed in the kernel I think. All that is needed is to add MODULE_ALIA_RTNL_LINK(ipip) to the ipip module (and the same for the other modules). This is already done for many (most?) netdev kinds, which is why this stuff just works for bridges, bonds, etc. I am not sure how it will be fixed in kernel. If the module not present kernel will say not supported . Could you please give a example. The main reason for loading module from networkd is avoid users loading manually . The kernel will auto-load any missing module as long as it has the correct module alias. If it finds that a certain kind is unsupported it will try to load rtnl-link-kind. Have a look at modinfo bridge and compare with modinfo ipip to see why the one works and the other does not. I'll submit a kernel patch for this (but until we know whether or not that will be applied I'd just keep the module loading you currently have). Thanks! Tom ___ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] [PATCH] networkd: Introduce ipip tunnel
On 04/07/2014 04:35 AM, Susant Sahani wrote: This will be much nicer if we simply use ipip as the kind, rather than tunnel. Done ! Hmm... I think it got right the first place from a usability perspective as in kind=tunnel then we need to introduce mode= in the associated network file as in |.netdev| |[NetDev] Name=tunnel0 Kind=tunnel ||[Match] Name=enp2s0 .network [Network] |||# one of the following| Mode=ipip | gre | sit | isatap | vti Address=192.168.0.15/24 Gateway=192.168.0.1| Or have the Mode= in the .netdev file itself JBG ___ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] [PATCH] networkd: Introduce ipip tunnel
On 04/07/2014 03:13 PM, Jóhann B. Guðmundsson wrote: On 04/07/2014 04:35 AM, Susant Sahani wrote: This will be much nicer if we simply use ipip as the kind, rather than tunnel. Done ! Hmm... I think it got right the first place from a usability perspective as in kind=tunnel then we need to introduce mode= in the associated network file as in Yes from user perceptive this is nice but few line code more ;) |.netdev| |[NetDev] Name=tunnel0 Kind=tunnel ||[Match] Name=enp2s0 .network [Network] |||# one of the following| Mode=ipip | gre | sit | isatap | vti Address=192.168.0.15/24 Gateway=192.168.0.1| Or have the Mode= in the .netdev file itself JBG Thanks, Susant ___ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel ___ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] [PATCH] networkd: Introduce ipip tunnel
On 04/07/2014 11:09 AM, Susant Sahani wrote: On 04/07/2014 03:13 PM, Jóhann B. Guðmundsson wrote: On 04/07/2014 04:35 AM, Susant Sahani wrote: This will be much nicer if we simply use ipip as the kind, rather than tunnel. Done ! Hmm... I think it got right the first place from a usability perspective as in kind=tunnel then we need to introduce mode= in the associated network file as in Yes from user perceptive this is nice but few line code more ;) I think this is more correct since you are dealing with multiple types of tunnel so the kind= should specify you are using an tunnel and then specify what type of tunnel in either .netdev file itself or .network file JBG ___ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] [PATCH] networkd: Introduce ipip tunnel
On 04/04/2014 10:00 PM, Tom Gundersen wrote: Hi Susant, Hi Tom, Thanks for reviewing . Thanks for this, looking forward getting this merged! I have some comments below though. I have addressed all your comments. However I have some queries Please find below. On Fri, Apr 4, 2014 at 11:25 AM, Susant Sahani sus...@redhat.com wrote: This patch enables basic ipip tunnel support. It works with kernel module ipip Example configuration File : ipip.netdev [NetDev] Name=ipip-tun Kind=tunnel [Tunnel] Kind=ipip Maybe we should simply have [NetDev] Kind=ipip We can still use the same [Tunnel] section for each of the tunnel kinds though. This way we are closer to the rtnl interface, and it seems a bit simpler to me. My intention of kind=tunnel is to keep the all kind of tunnels under the umbrella tunnel. But this also nice. Local=192.168.8.102 Remote=10.4.4.4 Dev=em1 I don't think we should be using the interface name (anywhere, unless we really must). I suggest we do the same with tunnel devices as with other netdev devices. Simply add a Tunnel=ipip-tun to the [Network] section of the corresponding interface and match in this way. Ttl=64 Mtu=1480 I guess these should be upper-case, and MTUBytes should be used as in .link files. So to sum up, I suggest replacing your example with: / ipip.netdev: [NetDev] Name=ipip-tun Kind=tunnel [Tunnel] Local=192.168.8.102 Remote=10.4.4.4 TTL=64 MTUBytes=1480 foo.network: [Match] Name=em1 [Network] Tunnel=ipip-tun // Modified . Also, we need to make sure that we only start setting up the tunnel when the underlying device (em1) has reached the correct state, so we really want to initiate the tunnel creation from networkd-link.c (so hooking into this from the .network file is the most convenient). Yes agreed. In the future, we may want to allow a short-hand, where separate .network and .netdev files are not necessarily in some cases, but let's delay that for now. Makefile.am | 7 +- src/network/networkd-netdev-gperf.gperf | 6 + src/network/networkd-netdev.c| 240 ++- src/network/networkd-network-gperf.gperf | 1 + src/network/networkd-network.c | 37 + src/network/networkd.h | 38 + 6 files changed, 322 insertions(+), 7 deletions(-) diff --git a/Makefile.am b/Makefile.am index c51f6ae..60c7016 100644 --- a/Makefile.am +++ b/Makefile.am @@ -4144,8 +4144,8 @@ systemd_networkd_SOURCES = \ src/network/networkd.c systemd_networkd_LDADD = \ - libsystemd-networkd-core.la - + libsystemd-networkd-core.la \ + -lkmod noinst_LTLIBRARIES += \ libsystemd-networkd-core.la @@ -4189,7 +4189,8 @@ test_network_SOURCES = \ src/network/test-network.c test_network_LDADD = \ - libsystemd-networkd-core.la + libsystemd-networkd-core.la \ + -lkmod tests += \ test-network diff --git a/src/network/networkd-netdev-gperf.gperf b/src/network/networkd-netdev-gperf.gperf index ea7ba57..ecca2bd 100644 --- a/src/network/networkd-netdev-gperf.gperf +++ b/src/network/networkd-netdev-gperf.gperf @@ -24,3 +24,9 @@ NetDev.Name, config_parse_ifname, 0, NetDev.Kind, config_parse_netdev_kind, 0, offsetof(NetDev, kind) VLAN.Id, config_parse_uint64,0, offsetof(NetDev, vlanid) MACVLAN.Mode,config_parse_macvlan_mode, 0, offsetof(NetDev, macvlan_mode) +Tunnel.Kind, config_parse_tunnel_kind, 0, offsetof(NetDev, tunnel_kind) +Tunnel.Dev, config_parse_ifname,0, offsetof(NetDev, tunnel_dev) +Tunnel.Ttl, config_parse_int, 0, offsetof(NetDev, tunnel_ttl) +Tunnel.Mtu, config_parse_int, 0, offsetof(NetDev, tunnel_mtu) +Tunnel.Local,config_parse_tunnel_address,0, offsetof(NetDev, tunnel_local) +Tunnel.Remote, config_parse_tunnel_address,0, offsetof(NetDev, tunnel_remote) diff --git a/src/network/networkd-netdev.c b/src/network/networkd-netdev.c index 762eff2..6abaf12 100644 --- a/src/network/networkd-netdev.c +++ b/src/network/networkd-netdev.c @@ -18,6 +18,12 @@ You should have received a copy of the GNU Lesser General Public License along with systemd; If not, see http://www.gnu.org/licenses/. ***/ +#include netinet/ether.h +#include arpa/inet.h +#include net/if.h +#include linux/ip.h +#include linux/if_tunnel.h +#include libkmod.h #include networkd.h #include network-internal.h @@ -33,6 +39,7 @@ static const char*
Re: [systemd-devel] [PATCH] networkd: Introduce ipip tunnel
On 04/07/2014 10:05 AM, Susant Sahani wrote:
On 04/04/2014 10:00 PM, Tom Gundersen wrote:
Hi Susant,
Hi Tom,
+ log_error_netdev(netdev,
+ Could not append IFLA_IPTUN_LINK
attribute: %s,
+ strerror(-r));
+return r;
+}
+
+r = sd_rtnl_message_append_u32(m, IFLA_IPTUN_LOCAL,
netdev-tunnel_local.s_addr);
+if (r 0) {
+log_error_netdev(netdev,
+ Could not append IFLA_IPTUN_LOCAL
attribute: %s,
+ strerror(-r));
+return r;
+}
+
+r = sd_rtnl_message_append_u32(m, IFLA_IPTUN_REMOTE,
netdev-tunnel_remote.s_addr);
+if (r 0) {
+log_error_netdev(netdev,
+ Could not append
IFLA_IPTUN_REMOTE attribute: %s,
+ strerror(-r));
+return r;
+}
Hm, I guess these should be _append_in_addr() to get the typesafety
right (might need to verify that we are using the right types for this
in rtnl-types.c.
I am missing something in the code . with the current rtnl code
it does not get appended. Could you please give a example.
r= sd_rtnl_message_append_in_addr(m, IFLA_IPTUN_LOCAL, (const struct
in_addr *)
netdev-tunnel_local.s_addr);
Could not append IFLA_IPTUN_LOCAL attribute: Invalid argument
I just figured out this should do .
git diff rtnl-types.c
diff --git a/src/libsystemd/sd-rtnl/rtnl-types.c
b/src/libsystemd/sd-rtnl/rtnl-types.c
index 27b7d04..585edc6 100644
--- a/src/libsystemd/sd-rtnl/rtnl-types.c
+++ b/src/libsystemd/sd-rtnl/rtnl-types.c
@@ -103,8 +103,8 @@ static const NLType
rtnl_link_info_data_bond_types[IFLA_BOND_MAX + 1] = {
static const NLType rtnl_link_info_data_iptun_types[IFLA_IPTUN_MAX +
1] = {
[IFLA_IPTUN_LINK]= { .type = NLA_U32 },
-[IFLA_IPTUN_LOCAL] = { .type = NLA_U32 },
-[IFLA_IPTUN_REMOTE] = { .type = NLA_U32 },
+[IFLA_IPTUN_LOCAL] = { .type = NLA_IN_ADDR },
+[IFLA_IPTUN_REMOTE] = { .type = NLA_IN_ADDR },
[IFLA_IPTUN_TTL] = { .type = NLA_U8 },
[IFLA_IPTUN_TOS] = { .type = NLA_U8 },
[IFLA_IPTUN_PMTUDISC]= { .type = NLA_U8 },
Thanks
Susant
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] [PATCH] networkd: Introduce ipip tunnel
Hi Susant,
Thanks for this, looking forward getting this merged!
I have some comments below though.
On Fri, Apr 4, 2014 at 11:25 AM, Susant Sahani sus...@redhat.com wrote:
This patch enables basic ipip tunnel support.
It works with kernel module ipip
Example configuration
File : ipip.netdev
[NetDev]
Name=ipip-tun
Kind=tunnel
[Tunnel]
Kind=ipip
Maybe we should simply have
[NetDev]
Kind=ipip
We can still use the same [Tunnel] section for each of the tunnel
kinds though. This way we are closer to the rtnl interface, and it
seems a bit simpler to me.
Local=192.168.8.102
Remote=10.4.4.4
Dev=em1
I don't think we should be using the interface name (anywhere, unless
we really must). I suggest we do the same with tunnel devices as with
other netdev devices. Simply add a Tunnel=ipip-tun to the [Network]
section of the corresponding interface and match in this way.
Ttl=64
Mtu=1480
I guess these should be upper-case, and MTUBytes should be used as in
.link files.
So to sum up, I suggest replacing your example with:
/
ipip.netdev:
[NetDev]
Name=ipip-tun
Kind=tunnel
[Tunnel]
Local=192.168.8.102
Remote=10.4.4.4
TTL=64
MTUBytes=1480
foo.network:
[Match]
Name=em1
[Network]
Tunnel=ipip-tun
//
Also, we need to make sure that we only start setting up the tunnel
when the underlying device (em1) has reached the correct state, so we
really want to initiate the tunnel creation from networkd-link.c (so
hooking into this from the .network file is the most convenient).
In the future, we may want to allow a short-hand, where separate
.network and .netdev files are not necessarily in some cases, but
let's delay that for now.
Makefile.am | 7 +-
src/network/networkd-netdev-gperf.gperf | 6 +
src/network/networkd-netdev.c| 240
++-
src/network/networkd-network-gperf.gperf | 1 +
src/network/networkd-network.c | 37 +
src/network/networkd.h | 38 +
6 files changed, 322 insertions(+), 7 deletions(-)
diff --git a/Makefile.am b/Makefile.am
index c51f6ae..60c7016 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -4144,8 +4144,8 @@ systemd_networkd_SOURCES = \
src/network/networkd.c
systemd_networkd_LDADD = \
- libsystemd-networkd-core.la
-
+ libsystemd-networkd-core.la \
+ -lkmod
noinst_LTLIBRARIES += \
libsystemd-networkd-core.la
@@ -4189,7 +4189,8 @@ test_network_SOURCES = \
src/network/test-network.c
test_network_LDADD = \
- libsystemd-networkd-core.la
+ libsystemd-networkd-core.la \
+ -lkmod
tests += \
test-network
diff --git a/src/network/networkd-netdev-gperf.gperf
b/src/network/networkd-netdev-gperf.gperf
index ea7ba57..ecca2bd 100644
--- a/src/network/networkd-netdev-gperf.gperf
+++ b/src/network/networkd-netdev-gperf.gperf
@@ -24,3 +24,9 @@ NetDev.Name, config_parse_ifname,
0,
NetDev.Kind, config_parse_netdev_kind, 0,
offsetof(NetDev, kind)
VLAN.Id, config_parse_uint64,0,
offsetof(NetDev, vlanid)
MACVLAN.Mode,config_parse_macvlan_mode, 0,
offsetof(NetDev, macvlan_mode)
+Tunnel.Kind, config_parse_tunnel_kind, 0,
offsetof(NetDev, tunnel_kind)
+Tunnel.Dev, config_parse_ifname,0,
offsetof(NetDev, tunnel_dev)
+Tunnel.Ttl, config_parse_int, 0,
offsetof(NetDev, tunnel_ttl)
+Tunnel.Mtu, config_parse_int, 0,
offsetof(NetDev, tunnel_mtu)
+Tunnel.Local,config_parse_tunnel_address,0,
offsetof(NetDev, tunnel_local)
+Tunnel.Remote, config_parse_tunnel_address,0,
offsetof(NetDev, tunnel_remote)
diff --git a/src/network/networkd-netdev.c b/src/network/networkd-netdev.c
index 762eff2..6abaf12 100644
--- a/src/network/networkd-netdev.c
+++ b/src/network/networkd-netdev.c
@@ -18,6 +18,12 @@
You should have received a copy of the GNU Lesser General Public License
along with systemd; If not, see http://www.gnu.org/licenses/.
***/
+#include netinet/ether.h
+#include arpa/inet.h
+#include net/if.h
+#include linux/ip.h
+#include linux/if_tunnel.h
+#include libkmod.h
#include networkd.h
#include network-internal.h
@@ -33,6 +39,7 @@ static const char* const
netdev_kind_table[_NETDEV_KIND_MAX] = {
[NETDEV_KIND_BOND] = bond,
[NETDEV_KIND_VLAN] = vlan,
[NETDEV_KIND_MACVLAN] = macvlan,
+[NETDEV_KIND_TUNNEL] = tunnel,
};
DEFINE_STRING_TABLE_LOOKUP(netdev_kind,
