Re: [RFC RTNETLINK 04/09]: Link creation API
Stephen Hemminger wrote: On Wed, 06 Jun 2007 01:17:11 +0200 Patrick McHardy [EMAIL PROTECTED] wrote: If you want I'll extend existing bridge netlink to use these. Are you talking about brige-port information or bridge device configuration? So far the API is not suitable for anything that currently uses IFLA_PROTINFO because the sender is not the driver which created the device and doesn't use AF_UNSPEC. For bridge device configuration it would certainly be nice to have, but I'm not sure yet how to handle enslave operations. So far my favourite idea is to add enslave/release operations to rtnl_link_ops and call them when IFLA_MASTER is set (so the netlink message would look like this: ifindex: eth0 master: br0 nlmsg_type: RTM_NETLINK). But I haven't really thought this through yet. Was thinking AF_BRIDGE (we have it already so use it), and both add/remove bridge, and enslave/unslave device. I think we should use two seperate families for bridge devices and bridge ports. I'll think about the enslave operation some more and try to add it next week. - To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC RTNETLINK 04/09]: Link creation API
+static int rtnl_newlink(struct sk_buff *skb, struct nlmsghdr *nlh, void *arg)
+{
+ struct rtnl_link_ops *ops;
+ struct net_device *dev;
+ struct ifinfomsg *ifm;
+ char name[MODULE_NAME_LEN];
+ char ifname[IFNAMSIZ];
+ struct nlattr *tb[IFLA_MAX+1];
+ struct nlattr *linkinfo[IFLA_INFO_MAX+1];
+ int err;
+
+ err = nlmsg_parse(nlh, sizeof(*ifm), tb, IFLA_MAX, ifla_policy);
+ if (err 0)
+ return err;
+
+ if (tb[IFLA_IFNAME])
+ nla_strlcpy(ifname, tb[IFLA_IFNAME], IFNAMSIZ);
+ else
+ ifname[0] = '\0';
+
+ ifm = nlmsg_data(nlh);
+ if (ifm-ifi_index 0)
+ dev = __dev_get_by_index(ifm-ifi_index);
+ else if (ifname[0])
+ dev = __dev_get_by_name(ifname);
+ else
+ dev = NULL;
+
+ if (tb[IFLA_LINKINFO]) {
+ err = nla_parse_nested(linkinfo, IFLA_INFO_MAX,
+tb[IFLA_LINKINFO], ifla_info_policy);
+ if (err 0)
+ return err;
+ } else
+ memset(linkinfo, 0, sizeof(linkinfo));
+
+ if (linkinfo[IFLA_INFO_NAME]) {
+ nla_strlcpy(name, linkinfo[IFLA_INFO_NAME], sizeof(name));
+ ops = rtnl_link_ops_get(name);
Ugh. Shouldn't we have the request_module logic here?
Otherwise it looks like we can skip the validate method and
have other weird interactions.
+ } else {
+ name[0] = '\0';
+ ops = NULL;
+ }
+
+ if (1) {
+ struct nlattr *attr[ops ? ops-maxtype + 1 : 0], **data = NULL;
+
+ if (ops) {
+ if (ops-maxtype linkinfo[IFLA_INFO_DATA]) {
+ err = nla_parse_nested(attr, ops-maxtype,
+ linkinfo[IFLA_INFO_DATA],
+ops-policy);
+ if (err 0)
+ return err;
+ data = attr;
+ }
+ if (ops-validate) {
+ err = ops-validate(tb, data);
+ if (err 0)
+ return err;
+ }
+ }
+
+ if (dev) {
+ int modified = 0;
+
+ if (nlh-nlmsg_flags NLM_F_EXCL)
+ return -EEXIST;
+ if (nlh-nlmsg_flags NLM_F_REPLACE)
+ return -EOPNOTSUPP;
+
+ if (linkinfo[IFLA_INFO_DATA]) {
+ if (!ops || ops != dev-rtnl_link_ops ||
+ !ops-changelink)
+ return -EOPNOTSUPP;
+
+ err = ops-changelink(dev, tb, data);
+ if (err 0)
+ return err;
+ modified = 1;
+ }
+
+ return do_setlink(dev, ifm, tb, ifname, modified);
+ }
+
+ if (!(nlh-nlmsg_flags NLM_F_CREATE))
+ return -ENODEV;
+
+ if (ifm-ifi_index)
+ return -EINVAL;
+ if (tb[IFLA_ADDRESS] || tb[IFLA_BROADCAST] || tb[IFLA_MAP])
+ return -EOPNOTSUPP;
+
+#ifdef CONFIG_KMOD
+ if (!ops name[0]) {
+ /* race condition: device may be created while rtnl is
+ * unlocked, final register_netdevice will catch it.
+ */
+ __rtnl_unlock();
+ request_module(rtnl-link-%s, name);
+ rtnl_lock();
+ ops = rtnl_link_ops_get(name);
+ }
+#endif
+ if (!ops)
+ return -EOPNOTSUPP;
+
+ if (!ifname[0])
+ snprintf(ifname, IFNAMSIZ, %s%%d, ops-name);
+ dev = alloc_netdev(ops-priv_size, ifname, ops-setup);
+ if (!dev)
+ return -ENOMEM;
+
+ if (strchr(dev-name, '%')) {
+ err = dev_alloc_name(dev, dev-name);
+ if (err 0)
+ goto err_free;
+ }
+ dev-rtnl_link_ops = ops;
+
+ if (tb[IFLA_MTU])
+ dev-mtu = nla_get_u32(tb[IFLA_MTU]);
+ if (tb[IFLA_TXQLEN])
+ dev-tx_queue_len = nla_get_u32(tb[IFLA_TXQLEN]);
+ if (tb[IFLA_WEIGHT])
+ dev-weight = nla_get_u32(tb[IFLA_WEIGHT]);
+ if (tb[IFLA_OPERSTATE])
+ set_operstate(dev, nla_get_u8(tb[IFLA_OPERSTATE]));
+ if (tb[IFLA_LINKMODE])
+ dev-link_mode = nla_get_u8(tb[IFLA_LINKMODE]);
+
+
Re: [RFC RTNETLINK 04/09]: Link creation API
Eric W. Biederman wrote:
+ if (linkinfo[IFLA_INFO_NAME]) {
+ nla_strlcpy(name, linkinfo[IFLA_INFO_NAME], sizeof(name));
+ ops = rtnl_link_ops_get(name);
Ugh. Shouldn't we have the request_module logic here?
Otherwise it looks like we can skip the validate method and
have other weird interactions.
Good catch. The easiest solution seems be to simply replay the
request after successful module load, which also avoids the
device lookup race.
Something like this (untested).
diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
index 8d2f817..f2868b0 100644
--- a/net/core/rtnetlink.c
+++ b/net/core/rtnetlink.c
@@ -930,6 +930,7 @@ static int rtnl_newlink(struct sk_buff *skb, struct
nlmsghdr *nlh, void *arg)
struct nlattr *linkinfo[IFLA_INFO_MAX+1];
int err;
+replay:
err = nlmsg_parse(nlh, sizeof(*ifm), tb, IFLA_MAX, ifla_policy);
if (err 0)
return err;
@@ -1012,19 +1013,19 @@ static int rtnl_newlink(struct sk_buff *skb, struct
nlmsghdr *nlh, void *arg)
if (tb[IFLA_ADDRESS] || tb[IFLA_BROADCAST] || tb[IFLA_MAP])
return -EOPNOTSUPP;
+ if (!ops) {
#ifdef CONFIG_KMOD
- if (!ops kind[0]) {
- /* race condition: device may be created while rtnl is
-* unlocked, final register_netdevice will catch it.
-*/
- __rtnl_unlock();
- request_module(rtnl-link-%s, kind);
- rtnl_lock();
- ops = rtnl_link_ops_get(kind);
- }
+ if (kind[0]) {
+ __rtnl_unlock();
+ request_module(rtnl-link-%s, kind);
+ rtnl_lock();
+ ops = rtnl_link_ops_get(kind);
+ if (ops)
+ goto replay;
+ }
#endif
- if (!ops)
return -EOPNOTSUPP;
+ }
if (!ifname[0])
snprintf(ifname, IFNAMSIZ, %s%%d, ops-kind);
Re: [RFC RTNETLINK 04/09]: Link creation API
Patrick McHardy [EMAIL PROTECTED] writes:
Eric W. Biederman wrote:
+if (linkinfo[IFLA_INFO_NAME]) {
+nla_strlcpy(name, linkinfo[IFLA_INFO_NAME], sizeof(name));
+ops = rtnl_link_ops_get(name);
Ugh. Shouldn't we have the request_module logic here?
Otherwise it looks like we can skip the validate method and
have other weird interactions.
Good catch. The easiest solution seems be to simply replay the
request after successful module load, which also avoids the
device lookup race.
Looks reasonable to me. There might be a variable or two that
we need to make certain is initialized but at a quick glance it
looks ok.
Eric
Something like this (untested).
diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
index 8d2f817..f2868b0 100644
--- a/net/core/rtnetlink.c
+++ b/net/core/rtnetlink.c
@@ -930,6 +930,7 @@ static int rtnl_newlink(struct sk_buff *skb, struct
nlmsghdr
*nlh, void *arg)
struct nlattr *linkinfo[IFLA_INFO_MAX+1];
int err;
+replay:
err = nlmsg_parse(nlh, sizeof(*ifm), tb, IFLA_MAX, ifla_policy);
if (err 0)
return err;
@@ -1012,19 +1013,19 @@ static int rtnl_newlink(struct sk_buff *skb, struct
nlmsghdr *nlh, void *arg)
if (tb[IFLA_ADDRESS] || tb[IFLA_BROADCAST] || tb[IFLA_MAP])
return -EOPNOTSUPP;
+ if (!ops) {
#ifdef CONFIG_KMOD
- if (!ops kind[0]) {
- /* race condition: device may be created while rtnl is
- * unlocked, final register_netdevice will catch it.
- */
- __rtnl_unlock();
- request_module(rtnl-link-%s, kind);
- rtnl_lock();
- ops = rtnl_link_ops_get(kind);
- }
+ if (kind[0]) {
+ __rtnl_unlock();
+ request_module(rtnl-link-%s, kind);
+ rtnl_lock();
+ ops = rtnl_link_ops_get(kind);
+ if (ops)
+ goto replay;
+ }
#endif
- if (!ops)
return -EOPNOTSUPP;
+ }
if (!ifname[0])
snprintf(ifname, IFNAMSIZ, %s%%d, ops-kind);
-
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC RTNETLINK 04/09]: Link creation API
From: Patrick McHardy [EMAIL PROTECTED] Date: Tue, 5 Jun 2007 16:12:57 +0200 (MEST) [RTNETLINK]: Link creation API Add rtnetlink API for creating, changing and deleting software devices. Signed-off-by: Patrick McHardy [EMAIL PROTECTED] Looks mostly fine, perhaps you can make even more use of 'const' for instances of struct rtnl_link_ops * at least as function arguments deeper in the implementation. - To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC RTNETLINK 04/09]: Link creation API
David Miller wrote: From: Patrick McHardy [EMAIL PROTECTED] Date: Tue, 5 Jun 2007 16:12:57 +0200 (MEST) [RTNETLINK]: Link creation API Add rtnetlink API for creating, changing and deleting software devices. Signed-off-by: Patrick McHardy [EMAIL PROTECTED] Looks mostly fine, perhaps you can make even more use of 'const' for instances of struct rtnl_link_ops * at least as function arguments deeper in the implementation. Good suggestion. I initially had rtnl_link_ops const everywhere (since the lookup was by family I stored it in an array and didn't need the list_head), then changed it to a list and removed all consts. I'll add them back where possible. - To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC RTNETLINK 04/09]: Link creation API
On Tue, 5 Jun 2007 16:12:57 +0200 (MEST) Patrick McHardy [EMAIL PROTECTED] wrote: [RTNETLINK]: Link creation API Add rtnetlink API for creating, changing and deleting software devices. Signed-off-by: Patrick McHardy [EMAIL PROTECTED] If you want I'll extend existing bridge netlink to use these. -- Stephen Hemminger [EMAIL PROTECTED] - To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC RTNETLINK 04/09]: Link creation API
Stephen Hemminger wrote: On Tue, 5 Jun 2007 16:12:57 +0200 (MEST) Patrick McHardy [EMAIL PROTECTED] wrote: [RTNETLINK]: Link creation API Add rtnetlink API for creating, changing and deleting software devices. Signed-off-by: Patrick McHardy [EMAIL PROTECTED] If you want I'll extend existing bridge netlink to use these. Are you talking about brige-port information or bridge device configuration? So far the API is not suitable for anything that currently uses IFLA_PROTINFO because the sender is not the driver which created the device and doesn't use AF_UNSPEC. For bridge device configuration it would certainly be nice to have, but I'm not sure yet how to handle enslave operations. So far my favourite idea is to add enslave/release operations to rtnl_link_ops and call them when IFLA_MASTER is set (so the netlink message would look like this: ifindex: eth0 master: br0 nlmsg_type: RTM_NETLINK). But I haven't really thought this through yet. I would also like to add support for handling secondary device state like bridge port state and others that currently use IFLA_PROTINFO (a lot of the code is very similar to the generic code), but all ideas so far turned out not to work very well. I'm leaving for a short vacation until Sunday tommorrow, so replies may be delayed :) - To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC RTNETLINK 04/09]: Link creation API
On Wed, 06 Jun 2007 01:17:11 +0200 Patrick McHardy [EMAIL PROTECTED] wrote: Stephen Hemminger wrote: On Tue, 5 Jun 2007 16:12:57 +0200 (MEST) Patrick McHardy [EMAIL PROTECTED] wrote: [RTNETLINK]: Link creation API Add rtnetlink API for creating, changing and deleting software devices. Signed-off-by: Patrick McHardy [EMAIL PROTECTED] If you want I'll extend existing bridge netlink to use these. Are you talking about brige-port information or bridge device configuration? So far the API is not suitable for anything that currently uses IFLA_PROTINFO because the sender is not the driver which created the device and doesn't use AF_UNSPEC. For bridge device configuration it would certainly be nice to have, but I'm not sure yet how to handle enslave operations. So far my favourite idea is to add enslave/release operations to rtnl_link_ops and call them when IFLA_MASTER is set (so the netlink message would look like this: ifindex: eth0 master: br0 nlmsg_type: RTM_NETLINK). But I haven't really thought this through yet. Was thinking AF_BRIDGE (we have it already so use it), and both add/remove bridge, and enslave/unslave device. I would also like to add support for handling secondary device state like bridge port state and others that currently use IFLA_PROTINFO (a lot of the code is very similar to the generic code), but all ideas so far turned out not to work very well. I'm leaving for a short vacation until Sunday tommorrow, so replies may be delayed :) -- Stephen Hemminger [EMAIL PROTECTED] - To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
