| Message ID | 4A02921A.1090200@cn.fujitsu.com |
|---|---|
| State | Rejected, archived |
| Delegated to: | David Miller |
| Headers | show |
From: Yang Hongyang <yanghy@cn.fujitsu.com> Date: Thu, 07 May 2009 15:47:38 +0800 > Although the function of these two functions are the same,we should use the > encapsulation function of __ipv6_addr_type > > Signed-off-by: Yang Hongyang<yanghy@cn.fujitsu.com> Are you sure? ipv6_addr_type() masks out the upper 16-bits of __ipv6_addr_type()'s return value, which is the scope. I'm not convinced that is what we want to do here. -- 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
David Miller wrote: > From: Yang Hongyang <yanghy@cn.fujitsu.com> > Date: Thu, 07 May 2009 15:47:38 +0800 > >> Although the function of these two functions are the same,we should use the >> encapsulation function of __ipv6_addr_type >> >> Signed-off-by: Yang Hongyang<yanghy@cn.fujitsu.com> > > Are you sure? ipv6_addr_type() masks out the upper 16-bits > of __ipv6_addr_type()'s return value, which is the scope. > > I'm not convinced that is what we want to do here. My advice here is that the function which has __ maybe shouldn't be directly used. Since the return of __ipv6_addr_type is needed by __ipv6_addr_src_scope,there should comes another patch to covert use of __ipv6_addr_src_scope to ipv6_addr_src_scope.To make this serie of the patch shouldn't be a problem because there are not so much use of these, two or three places. That's my advice.:)
From: Yang Hongyang <yanghy@cn.fujitsu.com> Date: Fri, 08 May 2009 09:18:11 +0800 > David Miller wrote: >> From: Yang Hongyang <yanghy@cn.fujitsu.com> >> Date: Thu, 07 May 2009 15:47:38 +0800 >> >>> Although the function of these two functions are the same,we should use the >>> encapsulation function of __ipv6_addr_type >>> >>> Signed-off-by: Yang Hongyang<yanghy@cn.fujitsu.com> >> >> Are you sure? ipv6_addr_type() masks out the upper 16-bits >> of __ipv6_addr_type()'s return value, which is the scope. >> >> I'm not convinced that is what we want to do here. > > My advice here is that the function which has __ maybe shouldn't be directly used. You're using vague terms like "advice" and "maybe" and this only decreases my confidence in your change. You're also not addressing my questions at all. Two "_" characters at the beginning of the function's name says nothing about whether these call sites want the scope bits present in the return value they receive. This seems like a completely pointless and mindless change, and in fact might introduce bugs. There is no way I'm applying this. You can't even explain if the patch is correct or not. In fact, I doubt you even know. -- 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: David Miller <davem@davemloft.net> Date: Fri, 08 May 2009 00:44:20 -0700 (PDT) > From: Yang Hongyang <yanghy@cn.fujitsu.com> > Date: Fri, 08 May 2009 09:18:11 +0800 > >> David Miller wrote: >>> From: Yang Hongyang <yanghy@cn.fujitsu.com> >>> Date: Thu, 07 May 2009 15:47:38 +0800 >>> >>>> Although the function of these two functions are the same,we should use the >>>> encapsulation function of __ipv6_addr_type >>>> >>>> Signed-off-by: Yang Hongyang<yanghy@cn.fujitsu.com> >>> >>> Are you sure? ipv6_addr_type() masks out the upper 16-bits >>> of __ipv6_addr_type()'s return value, which is the scope. >>> >>> I'm not convinced that is what we want to do here. >> >> My advice here is that the function which has __ maybe shouldn't be directly used. I also want to address this statement on it's own, it's simply not true. "__" means "think carefully before using this function", or "this function requires a special environment for proper usage" It DOES NOT mean "do not use directly." It's never meant this. So ever basis for your patch is fundamentally flawed. -- 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
David Miller wrote: > From: Yang Hongyang <yanghy@cn.fujitsu.com> > Date: Fri, 08 May 2009 09:18:11 +0800 > >> David Miller wrote: >>> From: Yang Hongyang <yanghy@cn.fujitsu.com> >>> Date: Thu, 07 May 2009 15:47:38 +0800 >>> >>>> Although the function of these two functions are the same,we should use the >>>> encapsulation function of __ipv6_addr_type >>>> >>>> Signed-off-by: Yang Hongyang<yanghy@cn.fujitsu.com> >>> Are you sure? ipv6_addr_type() masks out the upper 16-bits >>> of __ipv6_addr_type()'s return value, which is the scope. >>> >>> I'm not convinced that is what we want to do here. >> My advice here is that the function which has __ maybe shouldn't be directly used. > > You're using vague terms like "advice" and "maybe" and this > only decreases my confidence in your change. Yes,I said maybe because I judge this change by search the code.There are many places that use ipv6_addr_type() but only three places out of ipv6.h use __ipv6_addr_type().Now, there are two methods can to do the right things, one is ipv6_addr_type(),another is __ipv6_addr_type().There's no doult that These three places can be converted to use ipv6_addr_type().I said converted because it's not just change __ipv6_addr_type() to ipv6_addr_type(). Of course you can say that this dose not imply that we *should* do this change. I agree,so I just said this is a advice:) > > You're also not addressing my questions at all. > > Two "_" characters at the beginning of the function's name says > nothing about whether these call sites want the scope bits present in > the return value they receive. I used to think Two "_" means the funtion shouldn't(or should better not) be called outside of the file.You teach me,thank you! > > This seems like a completely pointless and mindless change, > and in fact might introduce bugs. If you think this is a pointless change,you can just ingore it, So sorry for the inconvenience:(But I don't think it's mindless,Because before you teach me, I really misunderstand Two "_" means . > > There is no way I'm applying this. You can't even explain if the > patch is correct or not. In fact, I doubt you even know. > Yes,this single patch is not correct.As I said,this patch should together with another patch to covert use of __ipv6_addr_src_scope to ipv6_addr_src_scope.
David Miller wrote: > From: David Miller <davem@davemloft.net> > Date: Fri, 08 May 2009 00:44:20 -0700 (PDT) > >> From: Yang Hongyang <yanghy@cn.fujitsu.com> >> Date: Fri, 08 May 2009 09:18:11 +0800 >> >>> David Miller wrote: >>>> From: Yang Hongyang <yanghy@cn.fujitsu.com> >>>> Date: Thu, 07 May 2009 15:47:38 +0800 >>>> >>>>> Although the function of these two functions are the same,we should use the >>>>> encapsulation function of __ipv6_addr_type >>>>> >>>>> Signed-off-by: Yang Hongyang<yanghy@cn.fujitsu.com> >>>> Are you sure? ipv6_addr_type() masks out the upper 16-bits >>>> of __ipv6_addr_type()'s return value, which is the scope. >>>> >>>> I'm not convinced that is what we want to do here. >>> My advice here is that the function which has __ maybe shouldn't be directly used. > > I also want to address this statement on it's own, it's simply > not true. > > "__" means "think carefully before using this function", or "this > function requires a special environment for proper usage" > > It DOES NOT mean "do not use directly." It's never meant this. I was wrong,thank you for your patience to explain this. > > So ever basis for your patch is fundamentally flawed. > >
diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c index a8218bc..01b7a83 100644 --- a/net/ipv6/addrconf.c +++ b/net/ipv6/addrconf.c @@ -1126,7 +1126,7 @@ int ipv6_dev_get_saddr(struct net *net, struct net_device *dst_dev, struct net_device *dev; int dst_type; - dst_type = __ipv6_addr_type(daddr); + dst_type = ipv6_addr_type(daddr); dst.addr = daddr; dst.ifindex = dst_dev ? dst_dev->ifindex : 0; dst.scope = __ipv6_addr_src_scope(dst_type); @@ -1181,7 +1181,7 @@ int ipv6_dev_get_saddr(struct net *net, struct net_device *dst_dev, (!(score->ifa->flags & IFA_F_OPTIMISTIC))) continue; - score->addr_type = __ipv6_addr_type(&score->ifa->addr); + score->addr_type = ipv6_addr_type(&score->ifa->addr); if (unlikely(score->addr_type == IPV6_ADDR_ANY || score->addr_type & IPV6_ADDR_MULTICAST)) { diff --git a/net/ipv6/datagram.c b/net/ipv6/datagram.c index e2bdc6d..7ea5c77 100644 --- a/net/ipv6/datagram.c +++ b/net/ipv6/datagram.c @@ -537,7 +537,7 @@ int datagram_send_ctl(struct net *net, fl->oif = src_info->ipi6_ifindex; } - addr_type = __ipv6_addr_type(&src_info->ipi6_addr); + addr_type = ipv6_addr_type(&src_info->ipi6_addr); if (fl->oif) { dev = dev_get_by_index(net, fl->oif);
Although the function of these two functions are the same,we should use the encapsulation function of __ipv6_addr_type Signed-off-by: Yang Hongyang<yanghy@cn.fujitsu.com> --- net/ipv6/addrconf.c | 4 ++-- net/ipv6/datagram.c | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-)