| Message ID | 1353715332-4284-6-git-send-email-sven@narfation.org |
|---|---|
| State | Awaiting Upstream, archived |
| Delegated to: | David Miller |
| Headers | show |
Please don't remove netdev from this discussion without a reason. On Saturday 01 December 2012 14:11:08 Antonio Quartulli wrote: > On Sat, Nov 24, 2012 at 01:02:11AM +0100, Sven Eckelmann wrote: > > The sysfs configuration interface of batman-adv to add/remove > > soft-interfaces is not deadlock free and doesn't follow the currently > > common way to create new virtual interfaces. > > > > An additional interface though rtnl_link is introduced which provides easy > > device creation/deletion with tools like "ip": > > > > $ ip link add dev bat0 type batadv > > $ ip link del dev bat0 > > > > Signed-off-by: Sven Eckelmann <sven@narfation.org> > > Hello Sven, > > why are we adding yet another API? What's the purpose? Is this intended to > fix the lock bug we get while using sysfs? Because this is the normal way to create virtual network devices (please feel free to correct me). And no, it doesn't fix the sysfs problem because the sysfs stuff isn't removed. Kind regards, Sven
On Sat, Dec 01, 2012 at 02:16:38PM +0100, Sven Eckelmann wrote: > Please don't remove netdev from this discussion without a reason. > > On Saturday 01 December 2012 14:11:08 Antonio Quartulli wrote: > > On Sat, Nov 24, 2012 at 01:02:11AM +0100, Sven Eckelmann wrote: > > > The sysfs configuration interface of batman-adv to add/remove > > > soft-interfaces is not deadlock free and doesn't follow the currently > > > common way to create new virtual interfaces. > > > > > > An additional interface though rtnl_link is introduced which provides easy > > > device creation/deletion with tools like "ip": > > > > > > $ ip link add dev bat0 type batadv > > > $ ip link del dev bat0 > > > > > > Signed-off-by: Sven Eckelmann <sven@narfation.org> > > > > Hello Sven, > > > > why are we adding yet another API? What's the purpose? Is this intended to > > fix the lock bug we get while using sysfs? > > Because this is the normal way to create virtual network devices (please feel > free to correct me). Well, I've seen different iface types using many tools, e.g. vconfig, tunctl, brctl.. Not that this justifies the fact that we should do the same (imho having a standard and unified way for creating interfaces would be the best option). But, to be honest, I think it should better discuss how to entirely moving/changing the existent API to a "better one" or to a "new one", instead of starting to maintain two of them from now on with no plan, don't you think so? > And no, it doesn't fix the sysfs problem because the > sysfs stuff isn't removed. Yeah, I had the same in mind, that's why I was asking for the purpose of this. Cheers,
On Saturday 01 December 2012 14:28:02 Antonio Quartulli wrote: > > Because this is the normal way to create virtual network devices (please > > feel free to correct me). > > Well, I've seen different iface types using many tools, e.g. vconfig, > tunctl, brctl.. > Not that this justifies the fact that we should do the same (imho having a > standard and unified way for creating interfaces would be the best option). The device creation and enslaving using vconfig, tunctl and brctl can be replaced using ip. > But, to be honest, I think it should better discuss how to entirely > moving/changing the existent API to a "better one" or to a "new one", > instead of starting to maintain two of them from now on with no plan, don't > you think so? I leave this discussion to the maintainers of batman-adv. Btw. removing the old one without a time of coexistence sounds like a bad move. And therefore maintaining of both interfaces like it is done in other network devices seems to be necessary. Kind regards, Sven
On Sat, Dec 01, 2012 at 02:39:26PM +0100, Sven Eckelmann wrote: > On Saturday 01 December 2012 14:28:02 Antonio Quartulli wrote: > > > Because this is the normal way to create virtual network devices (please > > > feel free to correct me). > > > > Well, I've seen different iface types using many tools, e.g. vconfig, > > tunctl, brctl.. > > Not that this justifies the fact that we should do the same (imho having a > > standard and unified way for creating interfaces would be the best option). > > The device creation and enslaving using vconfig, tunctl and brctl can be > replaced using ip. True. > > > But, to be honest, I think it should better discuss how to entirely > > moving/changing the existent API to a "better one" or to a "new one", > > instead of starting to maintain two of them from now on with no plan, don't > > you think so? > > I leave this discussion to the maintainers of batman-adv. I started this discussion here because you are still part of them (even if not listed in MAINTAINERS). > Btw. removing the > old one without a time of coexistence sounds like a bad move. And therefore > maintaining of both interfaces like it is done in other network devices seems > to be necessary. > Exactly, this is what I wanted to discuss as "a plan". Anyway, I discussed about this together with the others and it seems that a proper solution now is to wait before merging this patchset and fix the current sysfs/rtnl_lock problem first. What do you think? Adding a new API without fixing the current one doesn't sound like a good move. Cheers,
diff --git a/net/batman-adv/main.c b/net/batman-adv/main.c index f65a222..9b180a1 100644 --- a/net/batman-adv/main.c +++ b/net/batman-adv/main.c @@ -70,6 +70,7 @@ static int __init batadv_init(void) batadv_debugfs_init(); register_netdevice_notifier(&batadv_hard_if_notifier); + rtnl_link_register(&batadv_link_ops); pr_info("B.A.T.M.A.N. advanced %s (compatibility version %i) loaded\n", BATADV_SOURCE_VERSION, BATADV_COMPAT_VERSION); @@ -80,6 +81,7 @@ static int __init batadv_init(void) static void __exit batadv_exit(void) { batadv_debugfs_destroy(); + rtnl_link_unregister(&batadv_link_ops); unregister_netdevice_notifier(&batadv_hard_if_notifier); batadv_hardif_remove_interfaces(); diff --git a/net/batman-adv/main.h b/net/batman-adv/main.h index 2f85577..605b125 100644 --- a/net/batman-adv/main.h +++ b/net/batman-adv/main.h @@ -148,6 +148,7 @@ enum batadv_uev_type { #include <linux/percpu.h> #include <linux/slab.h> #include <net/sock.h> /* struct sock */ +#include <net/rtnetlink.h> #include <linux/jiffies.h> #include <linux/seq_file.h> #include "types.h" diff --git a/net/batman-adv/soft-interface.c b/net/batman-adv/soft-interface.c index 68832f5..fcb510e 100644 --- a/net/batman-adv/soft-interface.c +++ b/net/batman-adv/soft-interface.c @@ -533,6 +533,8 @@ struct net_device *batadv_softif_create(const char *name) if (!soft_iface) return NULL; + soft_iface->rtnl_link_ops = &batadv_link_ops; + ret = register_netdevice(soft_iface); if (ret < 0) { pr_err("Unable to register the batman interface '%s': %i\n", @@ -567,6 +569,13 @@ int batadv_softif_is_valid(const struct net_device *net_dev) return 0; } +struct rtnl_link_ops batadv_link_ops __read_mostly = { + .kind = "batadv", + .priv_size = sizeof(struct batadv_priv), + .setup = batadv_interface_setup, + .dellink = batadv_softif_destroy, +}; + /* ethtool */ static int batadv_get_settings(struct net_device *dev, struct ethtool_cmd *cmd) { diff --git a/net/batman-adv/soft-interface.h b/net/batman-adv/soft-interface.h index fe9b0f7..5eecc6e 100644 --- a/net/batman-adv/soft-interface.h +++ b/net/batman-adv/soft-interface.h @@ -28,5 +28,6 @@ struct net_device *batadv_softif_create(const char *name); void batadv_softif_destroy(struct net_device *soft_iface, struct list_head *head); int batadv_softif_is_valid(const struct net_device *net_dev); +extern struct rtnl_link_ops batadv_link_ops; #endif /* _NET_BATMAN_ADV_SOFT_INTERFACE_H_ */
The sysfs configuration interface of batman-adv to add/remove soft-interfaces is not deadlock free and doesn't follow the currently common way to create new virtual interfaces. An additional interface though rtnl_link is introduced which provides easy device creation/deletion with tools like "ip": $ ip link add dev bat0 type batadv $ ip link del dev bat0 Signed-off-by: Sven Eckelmann <sven@narfation.org> --- net/batman-adv/main.c | 2 ++ net/batman-adv/main.h | 1 + net/batman-adv/soft-interface.c | 9 +++++++++ net/batman-adv/soft-interface.h | 1 + 4 files changed, 13 insertions(+)