| Message ID | 1421666124-16055-1-git-send-email-johannes@sipsolutions.net |
|---|---|
| State | Accepted, archived |
| Delegated to: | David Miller |
| Headers | show |
Le lundi 19 janvier 2015, 12:15:24 Johannes Berg a écrit : > From: Johannes Berg <johannes.berg@intel.com> > > My previous patch to this file changed the code to be bug-compatible > towards userspace. Unless userspace (which I wasn't able to find) > implements the dump reader by hand in a wrong way, this isn't needed. The canonical userspace is there (specifically src/pnroute.c): https://gitorious.org/meego-cellular/phonet-utils/ AFAICT, it should work either way. By now, I expect what's left of the Maemo/Meego/Mer/whatever community has forked or rewritten it though. > If it uses libnl or similar code putting multiple messages into a > single SKB is far more efficient. > > Change the code to do this. While at it, also clean it up and don't > use so many variables - just store the address in the callback args > directly. > > Signed-off-by: Johannes Berg <johannes.berg@intel.com> > --- > net/phonet/pn_netlink.c | 22 +++++++--------------- > 1 file changed, 7 insertions(+), 15 deletions(-) > > diff --git a/net/phonet/pn_netlink.c b/net/phonet/pn_netlink.c > index 54d766842c2b..bc5ee5fbe6ae 100644 > --- a/net/phonet/pn_netlink.c > +++ b/net/phonet/pn_netlink.c > @@ -272,31 +272,23 @@ static int route_doit(struct sk_buff *skb, struct > nlmsghdr *nlh) static int route_dumpit(struct sk_buff *skb, struct > netlink_callback *cb) { > struct net *net = sock_net(skb->sk); > - u8 addr, addr_idx = 0, addr_start_idx = cb->args[0]; > + u8 addr; > > rcu_read_lock(); > - for (addr = 0; addr < 64; addr++) { > - struct net_device *dev; > + for (addr = cb->args[0]; addr < 64; addr++) { > + struct net_device *dev = phonet_route_get_rcu(net, addr << 2); > > - dev = phonet_route_get_rcu(net, addr << 2); > if (!dev) > continue; > > - if (addr_idx++ < addr_start_idx) > - continue; > - fill_route(skb, dev, addr << 2, NETLINK_CB(cb->skb).portid, > - cb->nlh->nlmsg_seq, RTM_NEWROUTE); > - /* fill_route() used to return > 0 (or negative errors) but > - * never 0 - ignore the return value and just go out to > - * call dumpit again from outside to preserve the behavior > - */ > - goto out; > + if (fill_route(skb, dev, addr << 2, NETLINK_CB(cb->skb).portid, > + cb->nlh->nlmsg_seq, RTM_NEWROUTE) < 0) > + goto out; > } > > out: > rcu_read_unlock(); > - cb->args[0] = addr_idx; > - cb->args[1] = 0; > + cb->args[0] = addr; > > return skb->len; > }
On Mon, 2015-01-19 at 22:11 +0200, Rémi Denis-Courmont wrote: > Le lundi 19 janvier 2015, 12:15:24 Johannes Berg a écrit : > > From: Johannes Berg <johannes.berg@intel.com> > > > > My previous patch to this file changed the code to be bug-compatible > > towards userspace. Unless userspace (which I wasn't able to find) > > implements the dump reader by hand in a wrong way, this isn't needed. > > The canonical userspace is there (specifically src/pnroute.c): > https://gitorious.org/meego-cellular/phonet-utils/ Ah, cool. > AFAICT, it should work either way. By now, I expect what's left of the > Maemo/Meego/Mer/whatever community has forked or rewritten it though. The loop for (struct nlmsghdr *nlh = (struct nlmsghdr *)&req; NLMSG_OK (nlh, (size_t)ret); nlh = NLMSG_NEXT (nlh, ret)) should make it do the right thing, I agree. johannes -- 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: Johannes Berg <johannes@sipsolutions.net> Date: Mon, 19 Jan 2015 12:15:24 +0100 > From: Johannes Berg <johannes.berg@intel.com> > > My previous patch to this file changed the code to be bug-compatible > towards userspace. Unless userspace (which I wasn't able to find) > implements the dump reader by hand in a wrong way, this isn't needed. > If it uses libnl or similar code putting multiple messages into a > single SKB is far more efficient. > > Change the code to do this. While at it, also clean it up and don't > use so many variables - just store the address in the callback args > directly. > > Signed-off-by: Johannes Berg <johannes.berg@intel.com> Applied, 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
diff --git a/net/phonet/pn_netlink.c b/net/phonet/pn_netlink.c index 54d766842c2b..bc5ee5fbe6ae 100644 --- a/net/phonet/pn_netlink.c +++ b/net/phonet/pn_netlink.c @@ -272,31 +272,23 @@ static int route_doit(struct sk_buff *skb, struct nlmsghdr *nlh) static int route_dumpit(struct sk_buff *skb, struct netlink_callback *cb) { struct net *net = sock_net(skb->sk); - u8 addr, addr_idx = 0, addr_start_idx = cb->args[0]; + u8 addr; rcu_read_lock(); - for (addr = 0; addr < 64; addr++) { - struct net_device *dev; + for (addr = cb->args[0]; addr < 64; addr++) { + struct net_device *dev = phonet_route_get_rcu(net, addr << 2); - dev = phonet_route_get_rcu(net, addr << 2); if (!dev) continue; - if (addr_idx++ < addr_start_idx) - continue; - fill_route(skb, dev, addr << 2, NETLINK_CB(cb->skb).portid, - cb->nlh->nlmsg_seq, RTM_NEWROUTE); - /* fill_route() used to return > 0 (or negative errors) but - * never 0 - ignore the return value and just go out to - * call dumpit again from outside to preserve the behavior - */ - goto out; + if (fill_route(skb, dev, addr << 2, NETLINK_CB(cb->skb).portid, + cb->nlh->nlmsg_seq, RTM_NEWROUTE) < 0) + goto out; } out: rcu_read_unlock(); - cb->args[0] = addr_idx; - cb->args[1] = 0; + cb->args[0] = addr; return skb->len; }