* Re: [PATCH 03/14] ipv4, ipv6: use list_for_each_entry* [not found] <201512190031.S65NXNNd%fengguang.wu@intel.com> @ 2015-12-18 16:59 ` Julia Lawall 2015-12-19 14:45 ` [PATCH 03/14 v2] " Geliang Tang 0 siblings, 1 reply; 3+ messages in thread From: Julia Lawall @ 2015-12-18 16:59 UTC (permalink / raw) To: Geliang Tang Cc: David S. Miller, Alexey Kuznetsov, James Morris, Hideaki YOSHIFUJI, Patrick McHardy, Geliang Tang, netdev, linux-kernel, kbuild-all I don't think this code can work. list_for_each_entry uses answer to get from one element to the next. julia On Sat, 19 Dec 2015, kbuild test robot wrote: > CC: kbuild-all@01.org > In-Reply-To: <0167bba2ecf8c4fcb6b0b3135a4e957309986498.1450451516.git.geliangtang@163.com> > TO: Geliang Tang <geliangtang@163.com> > CC: "David S. Miller" <davem@davemloft.net>, Alexey Kuznetsov <kuznet@ms2.inr.ac.ru>, James Morris <jmorris@namei.org>, Hideaki YOSHIFUJI <yoshfuji@linux-ipv6.org>, Patrick McHardy <kaber@trash.net> > CC: Geliang Tang <geliangtang@163.com>, netdev@vger.kernel.org, linux-kernel@vger.kernel.org > > Hi Geliang, > > [auto build test WARNING on net/master] > [also build test WARNING on v4.4-rc5 next-20151218] > > url: https://github.com/0day-ci/linux/commits/Geliang-Tang/Bluetooth-use-list_for_each_entry/20151218-234306 > :::::: branch date: 69 minutes ago > :::::: commit date: 69 minutes ago > > >> net/ipv6/af_inet6.c:589:1-20: iterator with update on line 597 > > git remote add linux-review https://github.com/0day-ci/linux > git remote update linux-review > git checkout c5e8d791cacac62eeec48e00a1a14a6a350670f4 > vim +589 net/ipv6/af_inet6.c > > ^1da177e Linus Torvalds 2005-04-16 583 goto out_illegal; > ^1da177e Linus Torvalds 2005-04-16 584 > ^1da177e Linus Torvalds 2005-04-16 585 /* If we are trying to override a permanent protocol, bail. */ > ^1da177e Linus Torvalds 2005-04-16 586 answer = NULL; > 87c3efbf Daniel Lezcano 2007-12-11 587 ret = -EPERM; > ^1da177e Linus Torvalds 2005-04-16 588 last_perm = &inetsw6[p->type]; > c5e8d791 Geliang Tang 2015-12-18 @589 list_for_each_entry(answer, &inetsw6[p->type], list) { > ^1da177e Linus Torvalds 2005-04-16 590 /* Check only the non-wild match. */ > ^1da177e Linus Torvalds 2005-04-16 591 if (INET_PROTOSW_PERMANENT & answer->flags) { > ^1da177e Linus Torvalds 2005-04-16 592 if (protocol == answer->protocol) > ^1da177e Linus Torvalds 2005-04-16 593 break; > c5e8d791 Geliang Tang 2015-12-18 594 last_perm = &answer->list; > ^1da177e Linus Torvalds 2005-04-16 595 } > ^1da177e Linus Torvalds 2005-04-16 596 > ^1da177e Linus Torvalds 2005-04-16 @597 answer = NULL; > ^1da177e Linus Torvalds 2005-04-16 598 } > ^1da177e Linus Torvalds 2005-04-16 599 if (answer) > ^1da177e Linus Torvalds 2005-04-16 600 goto out_permanent; > > --- > 0-DAY kernel test infrastructure Open Source Technology Center > https://lists.01.org/pipermail/kbuild-all Intel Corporation > ^ permalink raw reply [flat|nested] 3+ messages in thread
* [PATCH 03/14 v2] ipv4, ipv6: use list_for_each_entry* 2015-12-18 16:59 ` [PATCH 03/14] ipv4, ipv6: use list_for_each_entry* Julia Lawall @ 2015-12-19 14:45 ` Geliang Tang 2015-12-19 14:58 ` Julia Lawall 0 siblings, 1 reply; 3+ messages in thread From: Geliang Tang @ 2015-12-19 14:45 UTC (permalink / raw) To: Julia Lawall, David S. Miller, Alexey Kuznetsov, James Morris, Hideaki YOSHIFUJI, Patrick McHardy Cc: Geliang Tang, netdev, linux-kernel Use list_for_each_entry*() instead of list_for_each*() to simplify the code. Signed-off-by: Geliang Tang <geliangtang@163.com> --- Changes in v2: - bugfix for inet6_register_protosw in v1. --- net/ipv4/af_inet.c | 11 ++++------- net/ipv4/tcp_output.c | 6 ++---- net/ipv6/addrconf.c | 8 +++----- net/ipv6/af_inet6.c | 19 +++++-------------- 4 files changed, 14 insertions(+), 30 deletions(-) diff --git a/net/ipv4/af_inet.c b/net/ipv4/af_inet.c index 11c4ca1..eedf814 100644 --- a/net/ipv4/af_inet.c +++ b/net/ipv4/af_inet.c @@ -1028,10 +1028,8 @@ static struct inet_protosw inetsw_array[] = void inet_register_protosw(struct inet_protosw *p) { - struct list_head *lh; - struct inet_protosw *answer; + struct inet_protosw *answer, *last_perm; int protocol = p->protocol; - struct list_head *last_perm; spin_lock_bh(&inetsw_lock); @@ -1040,14 +1038,13 @@ void inet_register_protosw(struct inet_protosw *p) /* If we are trying to override a permanent protocol, bail. */ last_perm = &inetsw[p->type]; - list_for_each(lh, &inetsw[p->type]) { - answer = list_entry(lh, struct inet_protosw, list); + list_for_each_entry(answer, &inetsw[p->type], list) { /* Check only the non-wild match. */ if ((INET_PROTOSW_PERMANENT & answer->flags) == 0) break; if (protocol == answer->protocol) goto out_permanent; - last_perm = lh; + last_perm = answer; } /* Add the new entry after the last permanent entry if any, so that @@ -1056,7 +1053,7 @@ void inet_register_protosw(struct inet_protosw *p) * non-permanent entry. This means that when we remove this entry, the * system automatically returns to the old behavior. */ - list_add_rcu(&p->list, last_perm); + list_add_rcu(&p->list, &last_perm->list); out: spin_unlock_bh(&inetsw_lock); diff --git a/net/ipv4/tcp_output.c b/net/ipv4/tcp_output.c index a800cee..8810694 100644 --- a/net/ipv4/tcp_output.c +++ b/net/ipv4/tcp_output.c @@ -751,16 +751,14 @@ static void tcp_tasklet_func(unsigned long data) struct tsq_tasklet *tsq = (struct tsq_tasklet *)data; LIST_HEAD(list); unsigned long flags; - struct list_head *q, *n; - struct tcp_sock *tp; + struct tcp_sock *tp, *n; struct sock *sk; local_irq_save(flags); list_splice_init(&tsq->head, &list); local_irq_restore(flags); - list_for_each_safe(q, n, &list) { - tp = list_entry(q, struct tcp_sock, tsq_node); + list_for_each_entry_safe(tp, n, &list, tsq_node) { list_del(&tp->tsq_node); sk = (struct sock *)tp; diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c index 7082fb7..e293647 100644 --- a/net/ipv6/addrconf.c +++ b/net/ipv6/addrconf.c @@ -865,21 +865,19 @@ void inet6_ifa_finish_destroy(struct inet6_ifaddr *ifp) static void ipv6_link_dev_addr(struct inet6_dev *idev, struct inet6_ifaddr *ifp) { - struct list_head *p; + struct inet6_ifaddr *ifa; int ifp_scope = ipv6_addr_src_scope(&ifp->addr); /* * Each device address list is sorted in order of scope - * global before linklocal. */ - list_for_each(p, &idev->addr_list) { - struct inet6_ifaddr *ifa - = list_entry(p, struct inet6_ifaddr, if_list); + list_for_each_entry(ifa, &idev->addr_list, if_list) { if (ifp_scope >= ipv6_addr_src_scope(&ifa->addr)) break; } - list_add_tail(&ifp->if_list, p); + list_add_tail(&ifp->if_list, &ifa->if_list); } static u32 inet6_addr_hash(const struct in6_addr *addr) diff --git a/net/ipv6/af_inet6.c b/net/ipv6/af_inet6.c index 8ec0df7..437a500 100644 --- a/net/ipv6/af_inet6.c +++ b/net/ipv6/af_inet6.c @@ -568,9 +568,7 @@ static const struct net_proto_family inet6_family_ops = { int inet6_register_protosw(struct inet_protosw *p) { - struct list_head *lh; - struct inet_protosw *answer; - struct list_head *last_perm; + struct inet_protosw *answer, *last_perm; int protocol = p->protocol; int ret; @@ -581,23 +579,16 @@ int inet6_register_protosw(struct inet_protosw *p) goto out_illegal; /* If we are trying to override a permanent protocol, bail. */ - answer = NULL; ret = -EPERM; last_perm = &inetsw6[p->type]; - list_for_each(lh, &inetsw6[p->type]) { - answer = list_entry(lh, struct inet_protosw, list); - + list_for_each_entry(answer, &inetsw6[p->type], list) { /* Check only the non-wild match. */ if (INET_PROTOSW_PERMANENT & answer->flags) { if (protocol == answer->protocol) - break; - last_perm = lh; + goto out_permanent; + last_perm = answer; } - - answer = NULL; } - if (answer) - goto out_permanent; /* Add the new entry after the last permanent entry if any, so that * the new entry does not override a permanent entry when matched with @@ -605,7 +596,7 @@ int inet6_register_protosw(struct inet_protosw *p) * non-permanent entry. This means that when we remove this entry, the * system automatically returns to the old behavior. */ - list_add_rcu(&p->list, last_perm); + list_add_rcu(&p->list, &last_perm->list); ret = 0; out: spin_unlock_bh(&inetsw6_lock); -- 2.5.0 ^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH 03/14 v2] ipv4, ipv6: use list_for_each_entry* 2015-12-19 14:45 ` [PATCH 03/14 v2] " Geliang Tang @ 2015-12-19 14:58 ` Julia Lawall 0 siblings, 0 replies; 3+ messages in thread From: Julia Lawall @ 2015-12-19 14:58 UTC (permalink / raw) To: Geliang Tang Cc: David S. Miller, Alexey Kuznetsov, James Morris, Hideaki YOSHIFUJI, Patrick McHardy, netdev, linux-kernel On Sat, 19 Dec 2015, Geliang Tang wrote: > Use list_for_each_entry*() instead of list_for_each*() to simplify > the code. Is the code really simpler? At least in the case of net/ipv6/addrconf.c, I have the impression that there can be an access from a pointer that is not really pointing to the thing indicated by its type. The code seems to work, but this doesn't seem like a good property for it to have. In these cases, you need access to both the list and to the entry. The original code seemed like a reasonable way to express that. julia > Signed-off-by: Geliang Tang <geliangtang@163.com> > --- > Changes in v2: > - bugfix for inet6_register_protosw in v1. > --- > net/ipv4/af_inet.c | 11 ++++------- > net/ipv4/tcp_output.c | 6 ++---- > net/ipv6/addrconf.c | 8 +++----- > net/ipv6/af_inet6.c | 19 +++++-------------- > 4 files changed, 14 insertions(+), 30 deletions(-) > > diff --git a/net/ipv4/af_inet.c b/net/ipv4/af_inet.c > index 11c4ca1..eedf814 100644 > --- a/net/ipv4/af_inet.c > +++ b/net/ipv4/af_inet.c > @@ -1028,10 +1028,8 @@ static struct inet_protosw inetsw_array[] = > > void inet_register_protosw(struct inet_protosw *p) > { > - struct list_head *lh; > - struct inet_protosw *answer; > + struct inet_protosw *answer, *last_perm; > int protocol = p->protocol; > - struct list_head *last_perm; > > spin_lock_bh(&inetsw_lock); > > @@ -1040,14 +1038,13 @@ void inet_register_protosw(struct inet_protosw *p) > > /* If we are trying to override a permanent protocol, bail. */ > last_perm = &inetsw[p->type]; > - list_for_each(lh, &inetsw[p->type]) { > - answer = list_entry(lh, struct inet_protosw, list); > + list_for_each_entry(answer, &inetsw[p->type], list) { > /* Check only the non-wild match. */ > if ((INET_PROTOSW_PERMANENT & answer->flags) == 0) > break; > if (protocol == answer->protocol) > goto out_permanent; > - last_perm = lh; > + last_perm = answer; > } > > /* Add the new entry after the last permanent entry if any, so that > @@ -1056,7 +1053,7 @@ void inet_register_protosw(struct inet_protosw *p) > * non-permanent entry. This means that when we remove this entry, the > * system automatically returns to the old behavior. > */ > - list_add_rcu(&p->list, last_perm); > + list_add_rcu(&p->list, &last_perm->list); > out: > spin_unlock_bh(&inetsw_lock); > > diff --git a/net/ipv4/tcp_output.c b/net/ipv4/tcp_output.c > index a800cee..8810694 100644 > --- a/net/ipv4/tcp_output.c > +++ b/net/ipv4/tcp_output.c > @@ -751,16 +751,14 @@ static void tcp_tasklet_func(unsigned long data) > struct tsq_tasklet *tsq = (struct tsq_tasklet *)data; > LIST_HEAD(list); > unsigned long flags; > - struct list_head *q, *n; > - struct tcp_sock *tp; > + struct tcp_sock *tp, *n; > struct sock *sk; > > local_irq_save(flags); > list_splice_init(&tsq->head, &list); > local_irq_restore(flags); > > - list_for_each_safe(q, n, &list) { > - tp = list_entry(q, struct tcp_sock, tsq_node); > + list_for_each_entry_safe(tp, n, &list, tsq_node) { > list_del(&tp->tsq_node); > > sk = (struct sock *)tp; > diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c > index 7082fb7..e293647 100644 > --- a/net/ipv6/addrconf.c > +++ b/net/ipv6/addrconf.c > @@ -865,21 +865,19 @@ void inet6_ifa_finish_destroy(struct inet6_ifaddr *ifp) > static void > ipv6_link_dev_addr(struct inet6_dev *idev, struct inet6_ifaddr *ifp) > { > - struct list_head *p; > + struct inet6_ifaddr *ifa; > int ifp_scope = ipv6_addr_src_scope(&ifp->addr); > > /* > * Each device address list is sorted in order of scope - > * global before linklocal. > */ > - list_for_each(p, &idev->addr_list) { > - struct inet6_ifaddr *ifa > - = list_entry(p, struct inet6_ifaddr, if_list); > + list_for_each_entry(ifa, &idev->addr_list, if_list) { > if (ifp_scope >= ipv6_addr_src_scope(&ifa->addr)) > break; > } > > - list_add_tail(&ifp->if_list, p); > + list_add_tail(&ifp->if_list, &ifa->if_list); > } > > static u32 inet6_addr_hash(const struct in6_addr *addr) > diff --git a/net/ipv6/af_inet6.c b/net/ipv6/af_inet6.c > index 8ec0df7..437a500 100644 > --- a/net/ipv6/af_inet6.c > +++ b/net/ipv6/af_inet6.c > @@ -568,9 +568,7 @@ static const struct net_proto_family inet6_family_ops = { > > int inet6_register_protosw(struct inet_protosw *p) > { > - struct list_head *lh; > - struct inet_protosw *answer; > - struct list_head *last_perm; > + struct inet_protosw *answer, *last_perm; > int protocol = p->protocol; > int ret; > > @@ -581,23 +579,16 @@ int inet6_register_protosw(struct inet_protosw *p) > goto out_illegal; > > /* If we are trying to override a permanent protocol, bail. */ > - answer = NULL; > ret = -EPERM; > last_perm = &inetsw6[p->type]; > - list_for_each(lh, &inetsw6[p->type]) { > - answer = list_entry(lh, struct inet_protosw, list); > - > + list_for_each_entry(answer, &inetsw6[p->type], list) { > /* Check only the non-wild match. */ > if (INET_PROTOSW_PERMANENT & answer->flags) { > if (protocol == answer->protocol) > - break; > - last_perm = lh; > + goto out_permanent; > + last_perm = answer; > } > - > - answer = NULL; > } > - if (answer) > - goto out_permanent; > > /* Add the new entry after the last permanent entry if any, so that > * the new entry does not override a permanent entry when matched with > @@ -605,7 +596,7 @@ int inet6_register_protosw(struct inet_protosw *p) > * non-permanent entry. This means that when we remove this entry, the > * system automatically returns to the old behavior. > */ > - list_add_rcu(&p->list, last_perm); > + list_add_rcu(&p->list, &last_perm->list); > ret = 0; > out: > spin_unlock_bh(&inetsw6_lock); > -- > 2.5.0 > > > ^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2015-12-19 14:58 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <201512190031.S65NXNNd%fengguang.wu@intel.com>
2015-12-18 16:59 ` [PATCH 03/14] ipv4, ipv6: use list_for_each_entry* Julia Lawall
2015-12-19 14:45 ` [PATCH 03/14 v2] " Geliang Tang
2015-12-19 14:58 ` Julia Lawall
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox