| Message ID | 1470261735-2977-1-git-send-email-denkenz@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Johannes Berg |
| Headers | show |
On Wed, 2016-08-03 at 17:02 -0500, Denis Kenzior wrote: > > +static int nl80211_dump_interface_parse(struct sk_buff *skb, > + struct netlink_callback *cb, > + int *filter_wiphy) Wrong indentation :) > static int nl80211_dump_interface(struct sk_buff *skb, struct > netlink_callback *cb) > { > int wp_idx = 0; > int if_idx = 0; > int wp_start = cb->args[0]; > int if_start = cb->args[1]; > + int filter_wiphy = cb->args[2]; > struct cfg80211_registered_device *rdev; > struct wireless_dev *wdev; > > + if (!wp_start && !if_start && !filter_wiphy) { This seems incorrect - you're setting > + int ret; > + > + filter_wiphy = -1; > + > + ret = nl80211_dump_interface_parse(skb, cb, > &filter_wiphy); it here, but it can take the value 0, so !filter_wiphy seems wrong? johannes -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Johannes, On 08/11/2016 07:47 AM, Johannes Berg wrote: > On Wed, 2016-08-03 at 17:02 -0500, Denis Kenzior wrote: >> >> +static int nl80211_dump_interface_parse(struct sk_buff *skb, >> + struct netlink_callback *cb, >> + int *filter_wiphy) > > Wrong indentation :) Sorry :) Speaking of indentation, can you point me to a doc of the rules I should follow? > >> static int nl80211_dump_interface(struct sk_buff *skb, struct >> netlink_callback *cb) >> { >> int wp_idx = 0; >> int if_idx = 0; >> int wp_start = cb->args[0]; >> int if_start = cb->args[1]; >> + int filter_wiphy = cb->args[2]; >> struct cfg80211_registered_device *rdev; >> struct wireless_dev *wdev; >> >> + if (!wp_start && !if_start && !filter_wiphy) { > > This seems incorrect - you're setting > >> + int ret; >> + >> + filter_wiphy = -1; >> + >> + ret = nl80211_dump_interface_parse(skb, cb, >> &filter_wiphy); > > it here, but it can take the value 0, so !filter_wiphy seems wrong? > I can confirm that I sanity checked this patch. Both ATTR_WIPHY, ATTR_WDEV and wildcard dumps seemed to produce expected results. I noticed you applied this patch. Is there a particular scenario where it goes wrong or did you convince yourself it is correct? Regards, -Denis -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, 2016-08-11 at 11:38 -0500, Denis Kenzior wrote: > Hi Johannes, > > On 08/11/2016 07:47 AM, Johannes Berg wrote: > > > > On Wed, 2016-08-03 at 17:02 -0500, Denis Kenzior wrote: > > > > > > > > > +static int nl80211_dump_interface_parse(struct sk_buff *skb, > > > + struct netlink_callback *cb, > > > + int *filter_wiphy) > > > > Wrong indentation :) > > Sorry :) > > Speaking of indentation, can you point me to a doc of the rules I > should follow? You've seen Documentation/CodingStyle? > I can confirm that I sanity checked this patch. Both ATTR_WIPHY, > ATTR_WDEV and wildcard dumps seemed to produce expected results. I think it probably works due to the other conditions? > I noticed you applied this patch. Is there a particular scenario > where it goes wrong or did you convince yourself it is correct? > No, that was a mistake :( I've removed it now. johannes -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Johannes, >> Speaking of indentation, can you point me to a doc of the rules I >> should follow? > > You've seen Documentation/CodingStyle? Of course. But that one doesn't discuss that you want your function parameters to be aligned to the opening '('. Is there a dialect document specific to linux-wireless? > >> I can confirm that I sanity checked this patch. Both ATTR_WIPHY, >> ATTR_WDEV and wildcard dumps seemed to produce expected results. > > I think it probably works due to the other conditions? The initial conditions are that: cb->args[0..2] == 0. So on the first iteration we set filter_wiphy == -1 and check the filter attributes. If set, we modify filter_wiphy accordingly. Even if filter_wiphy is set to 0, the if statement should still never be entered afterwards since wp_start and if_start are incremented. Is this what you're worried about? Do you see a fault in my logic? Regards, -Denis -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 11-8-2016 20:20, Denis Kenzior wrote: > Hi Johannes, > >>> Speaking of indentation, can you point me to a doc of the rules I >>> should follow? >> >> You've seen Documentation/CodingStyle? > > Of course. But that one doesn't discuss that you want your function > parameters to be aligned to the opening '('. Is there a dialect > document specific to linux-wireless? You can find it in checkpatch.pl [1] :-p Regards, Arend [1] http://lxr.free-electrons.com/source/scripts/checkpatch.pl#L2855 -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Arend, > > You can find it in checkpatch.pl [1] :-p > Aha! Will use that one next time. Thanks. Regards, -Denis -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 11-8-2016 21:05, Denis Kenzior wrote: > Hi Arend, > >> >> You can find it in checkpatch.pl [1] :-p >> > > Aha! Will use that one next time. Thanks. There is a '--strict' command line option. Not sure if this indentation one false into that category. Regards, Arend > Regards, > -Denis -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, 2016-08-11 at 13:20 -0500, Denis Kenzior wrote: > Hi Johannes, > > >> Speaking of indentation, can you point me to a doc of the rules I > > > > > > > > should follow? > > > > You've seen Documentation/CodingStyle? > > Of course. But that one doesn't discuss that you want your function > parameters to be aligned to the opening '('. Is there a dialect > document specific to linux-wireless? Sorry. I don't think this is specific to our part of the tree, and I'm surprised it's not in there. But I see Arend also pointed you to checkpatch.pl, which, I might add, you shouldn't always take as authoritative since sometimes "fixing" things for it makes the code look worse. > The initial conditions are that: > cb->args[0..2] == 0. > > So on the first iteration we set filter_wiphy == -1 and check the > filter attributes. If set, we modify filter_wiphy accordingly. > > Even if filter_wiphy is set to 0, the if statement should still never > be entered afterwards since wp_start and if_start are incremented. > > Is this what you're worried about? Do you see a fault in my logic? No, I don't see a fault in the logic. I just think it's misleading. You make the code look like it relies on filter_wiphy != 0, but then you go and treat filter_wiphy==0 as a valid case. In other places, like nl80211_prepare_wdev_dump(), we add 1 to the wiphy and subtract it again later to avoid exactly this. Perhaps you could do the same, and rely only on filter_wiphy instead of really relying only on wp_start/if_start. johannes -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Johannes, > > No, I don't see a fault in the logic. I just think it's misleading. You > make the code look like it relies on filter_wiphy != 0, but then you go > and treat filter_wiphy==0 as a valid case. It relies on a sentry condition with all 3 variables being zero, not just filter_wiphy. The original patch is about the most non-invasive one I can come up with. Maybe a comment will alleviate any concerns? Anyway, if you feel this is misleading, fair enough. > > In other places, like nl80211_prepare_wdev_dump(), we add 1 to the > wiphy and subtract it again later to avoid exactly this. Perhaps you > could do the same, and rely only on filter_wiphy instead of really > relying only on wp_start/if_start. Having looked at that particular piece of code, I ran away scared with the conclusion that the cure is probably much worse than the disease :) I posted a slightly different solution in v2. It is a bit more invasive, but is more explicit in what is going on. I ran sanity checks on it and it works as expected. Regards, -Denis -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" 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/wireless/nl80211.c b/net/wireless/nl80211.c index 46417f9..ac19eb8 100644 --- a/net/wireless/nl80211.c +++ b/net/wireless/nl80211.c @@ -2519,15 +2519,47 @@ static int nl80211_send_iface(struct sk_buff *msg, u32 portid, u32 seq, int flag return -EMSGSIZE; } +static int nl80211_dump_interface_parse(struct sk_buff *skb, + struct netlink_callback *cb, + int *filter_wiphy) +{ + struct nlattr **tb = nl80211_fam.attrbuf; + int ret = nlmsg_parse(cb->nlh, GENL_HDRLEN + nl80211_fam.hdrsize, + tb, nl80211_fam.maxattr, nl80211_policy); + /* ignore parse errors for backward compatibility */ + if (ret) + return 0; + + if (tb[NL80211_ATTR_WIPHY]) + *filter_wiphy = nla_get_u32(tb[NL80211_ATTR_WIPHY]); + if (tb[NL80211_ATTR_WDEV]) + *filter_wiphy = nla_get_u64(tb[NL80211_ATTR_WDEV]) >> 32; + + return 0; +} + static int nl80211_dump_interface(struct sk_buff *skb, struct netlink_callback *cb) { int wp_idx = 0; int if_idx = 0; int wp_start = cb->args[0]; int if_start = cb->args[1]; + int filter_wiphy = cb->args[2]; struct cfg80211_registered_device *rdev; struct wireless_dev *wdev; + if (!wp_start && !if_start && !filter_wiphy) { + int ret; + + filter_wiphy = -1; + + ret = nl80211_dump_interface_parse(skb, cb, &filter_wiphy); + if (ret) + return ret; + + cb->args[2] = filter_wiphy; + } + rtnl_lock(); list_for_each_entry(rdev, &cfg80211_rdev_list, list) { if (!net_eq(wiphy_net(&rdev->wiphy), sock_net(skb->sk))) @@ -2536,6 +2568,10 @@ static int nl80211_dump_interface(struct sk_buff *skb, struct netlink_callback * wp_idx++; continue; } + + if (filter_wiphy != -1 && filter_wiphy != rdev->wiphy_idx) + continue; + if_idx = 0; list_for_each_entry(wdev, &rdev->wiphy.wdev_list, list) {
This patch allows GET_INTERFACE dumps to be filtered based on NL80211_ATTR_WIPHY or NL80211_ATTR_WDEV. The documentation for GET_INTERFACE mentions that this is possible: "Request an interface's configuration; either a dump request on a %NL80211_ATTR_WIPHY or ..." However, this behavior has not been implemented until now. Signed-off-by: Denis Kenzior <denkenz@gmail.com> --- net/wireless/nl80211.c | 36 ++++++++++++++++++++++++++++++++++++ 1 file changed, 36 insertions(+)