| Message ID | 1353082456-21234-2-git-send-email-nicolas.dichtel@6wind.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
On Fri, 16 Nov 2012 17:14:13 +0100 Nicolas Dichtel <nicolas.dichtel@6wind.com> wrote: > Now that tunnels can be configured via rtnetlink, this device is not mandatory. > The default is conservative. > > Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com> Although I am in favor of reducing clutter, and we even have to put in special case code to ignore these stub devices in the Vyatta scripts. Module parameters are bit of a nuisance to deal with, but maybe the only way for this kind of thing and keep the required ABI. Not sure if I can fully endorse this. The device may still have uses. It is still useful for capturing "none of the above" packets and is used to auto-load module via module aliases. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Le 16/11/2012 17:29, Stephen Hemminger a écrit : > On Fri, 16 Nov 2012 17:14:13 +0100 > Nicolas Dichtel <nicolas.dichtel@6wind.com> wrote: > >> Now that tunnels can be configured via rtnetlink, this device is not mandatory. >> The default is conservative. >> >> Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com> > > > Although I am in favor of reducing clutter, and we even have to put in special case > code to ignore these stub devices in the Vyatta scripts. Module parameters are bit of a nuisance to deal with, but maybe > the only way for this kind of thing and keep the required ABI. > > Not sure if I can fully endorse this. The device may still have uses. > It is still useful for capturing "none of the above" packets If you need to capture these packets, you can still create a tunnel with local any and remote any, even if the fb_device has not been created. > and is used to auto-load module via module aliases. Right, but if user uses netlink, the problem exists without these patches too. By default, the fb device is created, so there is no change if you don't set explicitly setup_fb to 0. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
From: Nicolas Dichtel <nicolas.dichtel@6wind.com> Date: Fri, 16 Nov 2012 17:14:13 +0100 > Now that tunnels can be configured via rtnetlink, this device is not mandatory. > The default is conservative. > > Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com> I'm not too thrilled about this change, mostly because of my dislike of module parameters in general. But in this case there appears to be real bugs in the two sets of changes where you add this setup_fb thing. > @@ -1057,7 +1066,8 @@ static void __net_exit ipip_exit_net(struct net *net) > > rtnl_lock(); > ipip_destroy_tunnels(ipn, &list); > - unregister_netdevice_queue(ipn->fb_tunnel_dev, &list); > + if (setup_fb) > + unregister_netdevice_queue(ipn->fb_tunnel_dev, &list); > unregister_netdevice_many(&list); > rtnl_unlock(); > } Users can modify module parameter values via sysfs after the module is loaded, so you need a more internal and protected state to use to decide whether you really need to unregister the thing or not. But to me it's just symptomatic of what a bad idea this is in the first place. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
From: Nicolas Dichtel <nicolas.dichtel@6wind.com> Date: Fri, 16 Nov 2012 17:46:11 +0100 > By default, the fb device is created, so there is no change if you > don't set explicitly setup_fb to 0. Nicolas, this idea is contentous to me too. Why not put this aside and submit the other parts of your patch set on their own, since those looked fine to me? Thanks. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Le 20/11/2012 01:07, David Miller a écrit : > From: Nicolas Dichtel <nicolas.dichtel@6wind.com> > Date: Fri, 16 Nov 2012 17:46:11 +0100 > >> By default, the fb device is created, so there is no change if you >> don't set explicitly setup_fb to 0. > > > Nicolas, this idea is contentous to me too. > > Why not put this aside and submit the other parts of your > patch set on their own, since those looked fine to me? Ok, no problem. I wanted to get feedback about this kind of idea, I understand the point. When you said "the other parts", you mean only patch 2/4 (sit: allow to configure 6rd tunnels via netlink) or 3/4 and 4/4 too? Because 3/4 and 4/4 are equivalent to this one. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
From: Nicolas Dichtel <nicolas.dichtel@6wind.com> Date: Tue, 20 Nov 2012 09:30:17 +0100 > Le 20/11/2012 01:07, David Miller a écrit : >> Why not put this aside and submit the other parts of your >> patch set on their own, since those looked fine to me? > Ok, no problem. I wanted to get feedback about this kind of idea, I > understand the point. > > When you said "the other parts", you mean only patch 2/4 (sit: allow > to configure 6rd tunnels via netlink) or 3/4 and 4/4 too? > Because 3/4 and 4/4 are equivalent to this one. I meant patch #2. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/net/ipv4/ipip.c b/net/ipv4/ipip.c index c26c171..9e11633 100644 --- a/net/ipv4/ipip.c +++ b/net/ipv4/ipip.c @@ -124,6 +124,11 @@ static bool log_ecn_error = true; module_param(log_ecn_error, bool, 0644); MODULE_PARM_DESC(log_ecn_error, "Log packets received with corrupted ECN"); +static bool setup_fb = true; +module_param(setup_fb, bool, 0644); +MODULE_PARM_DESC(setup_fb, + "Setup the fb device to configure tunnel via IOCTL"); + static int ipip_net_id __read_mostly; struct ipip_net { struct ip_tunnel __rcu *tunnels_r_l[HASH_SIZE]; @@ -1022,25 +1027,29 @@ static int __net_init ipip_init_net(struct net *net) ipn->tunnels[2] = ipn->tunnels_r; ipn->tunnels[3] = ipn->tunnels_r_l; - ipn->fb_tunnel_dev = alloc_netdev(sizeof(struct ip_tunnel), - "tunl0", - ipip_tunnel_setup); - if (!ipn->fb_tunnel_dev) { - err = -ENOMEM; - goto err_alloc_dev; - } - dev_net_set(ipn->fb_tunnel_dev, net); + if (setup_fb) { + ipn->fb_tunnel_dev = alloc_netdev(sizeof(struct ip_tunnel), + "tunl0", + ipip_tunnel_setup); + if (!ipn->fb_tunnel_dev) { + err = -ENOMEM; + goto err_alloc_dev; + } + dev_net_set(ipn->fb_tunnel_dev, net); - err = ipip_fb_tunnel_init(ipn->fb_tunnel_dev); - if (err) - goto err_reg_dev; + err = ipip_fb_tunnel_init(ipn->fb_tunnel_dev); + if (err) + goto err_reg_dev; - if ((err = register_netdev(ipn->fb_tunnel_dev))) - goto err_reg_dev; + err = register_netdev(ipn->fb_tunnel_dev); + if (err) + goto err_reg_dev; - t = netdev_priv(ipn->fb_tunnel_dev); + t = netdev_priv(ipn->fb_tunnel_dev); - strcpy(t->parms.name, ipn->fb_tunnel_dev->name); + strcpy(t->parms.name, ipn->fb_tunnel_dev->name); + } else + ipn->fb_tunnel_dev = NULL; return 0; err_reg_dev: @@ -1057,7 +1066,8 @@ static void __net_exit ipip_exit_net(struct net *net) rtnl_lock(); ipip_destroy_tunnels(ipn, &list); - unregister_netdevice_queue(ipn->fb_tunnel_dev, &list); + if (setup_fb) + unregister_netdevice_queue(ipn->fb_tunnel_dev, &list); unregister_netdevice_many(&list); rtnl_unlock(); }
Now that tunnels can be configured via rtnetlink, this device is not mandatory. The default is conservative. Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com> --- net/ipv4/ipip.c | 42 ++++++++++++++++++++++++++---------------- 1 file changed, 26 insertions(+), 16 deletions(-)