* [PATCH] nfs: fix dead code of ipv6_addr_scope @ 2013-12-21 4:39 ` Alexander Aring 0 siblings, 0 replies; 13+ messages in thread From: Alexander Aring @ 2013-12-21 4:39 UTC (permalink / raw) To: Trond.Myklebust; +Cc: linux-nfs, netdev, werner, Alexander Aring The correct way to check on IPV6_ADDR_SCOPE_LINKLOCAL is to check with the ipv6_addr_src_scope function. Currently this can't be work, because ipv6_addr_scope returns a int with a mask of IPV6_ADDR_SCOPE_MASK (0x00f0U) and IPV6_ADDR_SCOPE_LINKLOCAL is 0x02. So the condition is always false. Signed-off-by: Alexander Aring <alex.aring@gmail.com> --- I think ipv6_addr_src_scope should be correct, can somebody from netdev ml confirm this please? I stumple over that and I did not compile and test it. Maybe this is something for stable? fs/nfs/nfs4filelayoutdev.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/fs/nfs/nfs4filelayoutdev.c b/fs/nfs/nfs4filelayoutdev.c index c7c295e5..efac602 100644 --- a/fs/nfs/nfs4filelayoutdev.c +++ b/fs/nfs/nfs4filelayoutdev.c @@ -95,7 +95,7 @@ same_sockaddr(struct sockaddr *addr1, struct sockaddr *addr2) b6 = (struct sockaddr_in6 *)addr2; /* LINKLOCAL addresses must have matching scope_id */ - if (ipv6_addr_scope(&a6->sin6_addr) == + if (ipv6_addr_src_scope(&a6->sin6_addr) == IPV6_ADDR_SCOPE_LINKLOCAL && a6->sin6_scope_id != b6->sin6_scope_id) return false; -- 1.8.5.2 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH] nfs: fix dead code of ipv6_addr_scope @ 2013-12-21 4:39 ` Alexander Aring 0 siblings, 0 replies; 13+ messages in thread From: Alexander Aring @ 2013-12-21 4:39 UTC (permalink / raw) To: Trond.Myklebust-HgOvQuBEEgTQT0dZR+AlfA Cc: linux-nfs-u79uwXL29TY76Z2rM5mHXA, netdev-u79uwXL29TY76Z2rM5mHXA, werner-SEdMjqphH88wryQfseakQg, Alexander Aring The correct way to check on IPV6_ADDR_SCOPE_LINKLOCAL is to check with the ipv6_addr_src_scope function. Currently this can't be work, because ipv6_addr_scope returns a int with a mask of IPV6_ADDR_SCOPE_MASK (0x00f0U) and IPV6_ADDR_SCOPE_LINKLOCAL is 0x02. So the condition is always false. Signed-off-by: Alexander Aring <alex.aring-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> --- I think ipv6_addr_src_scope should be correct, can somebody from netdev ml confirm this please? I stumple over that and I did not compile and test it. Maybe this is something for stable? fs/nfs/nfs4filelayoutdev.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/fs/nfs/nfs4filelayoutdev.c b/fs/nfs/nfs4filelayoutdev.c index c7c295e5..efac602 100644 --- a/fs/nfs/nfs4filelayoutdev.c +++ b/fs/nfs/nfs4filelayoutdev.c @@ -95,7 +95,7 @@ same_sockaddr(struct sockaddr *addr1, struct sockaddr *addr2) b6 = (struct sockaddr_in6 *)addr2; /* LINKLOCAL addresses must have matching scope_id */ - if (ipv6_addr_scope(&a6->sin6_addr) == + if (ipv6_addr_src_scope(&a6->sin6_addr) == IPV6_ADDR_SCOPE_LINKLOCAL && a6->sin6_scope_id != b6->sin6_scope_id) return false; -- 1.8.5.2 -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH] nfs: fix dead code of ipv6_addr_scope 2013-12-21 4:39 ` Alexander Aring (?) @ 2013-12-21 8:17 ` Alexander Aring -1 siblings, 0 replies; 13+ messages in thread From: Alexander Aring @ 2013-12-21 8:17 UTC (permalink / raw) To: Trond.Myklebust; +Cc: linux-nfs, netdev, werner Hi, I saw right now file "./net/sctp/ipv6.c" in function sctp_v6_scope has the same issue. - Alex ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] nfs: fix dead code of ipv6_addr_scope @ 2013-12-21 12:44 ` Hannes Frederic Sowa 0 siblings, 0 replies; 13+ messages in thread From: Hannes Frederic Sowa @ 2013-12-21 12:44 UTC (permalink / raw) To: Alexander Aring; +Cc: Trond.Myklebust, linux-nfs, netdev, werner On Sat, Dec 21, 2013 at 05:39:04AM +0100, Alexander Aring wrote: > The correct way to check on IPV6_ADDR_SCOPE_LINKLOCAL is to check with > the ipv6_addr_src_scope function. > > Currently this can't be work, because ipv6_addr_scope returns a int with > a mask of IPV6_ADDR_SCOPE_MASK (0x00f0U) and IPV6_ADDR_SCOPE_LINKLOCAL > is 0x02. So the condition is always false. > > Signed-off-by: Alexander Aring <alex.aring@gmail.com> > --- > I think ipv6_addr_src_scope should be correct, can somebody from netdev ml > confirm this please? > I stumple over that and I did not compile and test it. Maybe this is something > for stable? > > fs/nfs/nfs4filelayoutdev.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/fs/nfs/nfs4filelayoutdev.c b/fs/nfs/nfs4filelayoutdev.c > index c7c295e5..efac602 100644 > --- a/fs/nfs/nfs4filelayoutdev.c > +++ b/fs/nfs/nfs4filelayoutdev.c > @@ -95,7 +95,7 @@ same_sockaddr(struct sockaddr *addr1, struct sockaddr *addr2) > b6 = (struct sockaddr_in6 *)addr2; > > /* LINKLOCAL addresses must have matching scope_id */ > - if (ipv6_addr_scope(&a6->sin6_addr) == > + if (ipv6_addr_src_scope(&a6->sin6_addr) == > IPV6_ADDR_SCOPE_LINKLOCAL && > a6->sin6_scope_id != b6->sin6_scope_id) > return false; Good catch! SCOPE_TYPE also can be compared and is no bitfield, so the patch is good. Do you mind also proposing a patch for sctp? Thanks, Hannes ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] nfs: fix dead code of ipv6_addr_scope @ 2013-12-21 12:44 ` Hannes Frederic Sowa 0 siblings, 0 replies; 13+ messages in thread From: Hannes Frederic Sowa @ 2013-12-21 12:44 UTC (permalink / raw) To: Alexander Aring Cc: Trond.Myklebust-HgOvQuBEEgTQT0dZR+AlfA, linux-nfs-u79uwXL29TY76Z2rM5mHXA, netdev-u79uwXL29TY76Z2rM5mHXA, werner-SEdMjqphH88wryQfseakQg On Sat, Dec 21, 2013 at 05:39:04AM +0100, Alexander Aring wrote: > The correct way to check on IPV6_ADDR_SCOPE_LINKLOCAL is to check with > the ipv6_addr_src_scope function. > > Currently this can't be work, because ipv6_addr_scope returns a int with > a mask of IPV6_ADDR_SCOPE_MASK (0x00f0U) and IPV6_ADDR_SCOPE_LINKLOCAL > is 0x02. So the condition is always false. > > Signed-off-by: Alexander Aring <alex.aring-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> > --- > I think ipv6_addr_src_scope should be correct, can somebody from netdev ml > confirm this please? > I stumple over that and I did not compile and test it. Maybe this is something > for stable? > > fs/nfs/nfs4filelayoutdev.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/fs/nfs/nfs4filelayoutdev.c b/fs/nfs/nfs4filelayoutdev.c > index c7c295e5..efac602 100644 > --- a/fs/nfs/nfs4filelayoutdev.c > +++ b/fs/nfs/nfs4filelayoutdev.c > @@ -95,7 +95,7 @@ same_sockaddr(struct sockaddr *addr1, struct sockaddr *addr2) > b6 = (struct sockaddr_in6 *)addr2; > > /* LINKLOCAL addresses must have matching scope_id */ > - if (ipv6_addr_scope(&a6->sin6_addr) == > + if (ipv6_addr_src_scope(&a6->sin6_addr) == > IPV6_ADDR_SCOPE_LINKLOCAL && > a6->sin6_scope_id != b6->sin6_scope_id) > return false; Good catch! SCOPE_TYPE also can be compared and is no bitfield, so the patch is good. Do you mind also proposing a patch for sctp? Thanks, Hannes -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] nfs: fix dead code of ipv6_addr_scope @ 2013-12-21 13:32 ` Alexander Aring 0 siblings, 0 replies; 13+ messages in thread From: Alexander Aring @ 2013-12-21 13:32 UTC (permalink / raw) To: linux-nfs, netdev, werner Hi Hannes, On Sat, Dec 21, 2013 at 01:44:40PM +0100, Hannes Frederic Sowa wrote: > On Sat, Dec 21, 2013 at 05:39:04AM +0100, Alexander Aring wrote: > > The correct way to check on IPV6_ADDR_SCOPE_LINKLOCAL is to check with > > the ipv6_addr_src_scope function. > > > > Currently this can't be work, because ipv6_addr_scope returns a int with > > a mask of IPV6_ADDR_SCOPE_MASK (0x00f0U) and IPV6_ADDR_SCOPE_LINKLOCAL > > is 0x02. So the condition is always false. > > > > Signed-off-by: Alexander Aring <alex.aring@gmail.com> > > --- > > I think ipv6_addr_src_scope should be correct, can somebody from netdev ml > > confirm this please? > > I stumple over that and I did not compile and test it. Maybe this is something > > for stable? > > > > fs/nfs/nfs4filelayoutdev.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/fs/nfs/nfs4filelayoutdev.c b/fs/nfs/nfs4filelayoutdev.c > > index c7c295e5..efac602 100644 > > --- a/fs/nfs/nfs4filelayoutdev.c > > +++ b/fs/nfs/nfs4filelayoutdev.c > > @@ -95,7 +95,7 @@ same_sockaddr(struct sockaddr *addr1, struct sockaddr *addr2) > > b6 = (struct sockaddr_in6 *)addr2; > > > > /* LINKLOCAL addresses must have matching scope_id */ > > - if (ipv6_addr_scope(&a6->sin6_addr) == > > + if (ipv6_addr_src_scope(&a6->sin6_addr) == > > IPV6_ADDR_SCOPE_LINKLOCAL && > > a6->sin6_scope_id != b6->sin6_scope_id) > > return false; > > Good catch! > thanks. I am still unsure if sctp is correct or not, I think it isn't correct. Because we compare and don't check if any bit is set. We don't use IPV6_ADDR_SCOPE_TYPE here. We use IPV6_ADDR_TYPE. But we can't compare it. Current implementation is: v6scope = ipv6_addr_scope(&addr->v6.sin6_addr); switch (v6scope) { case IFA_HOST: retval = SCTP_SCOPE_LOOPBACK; break; case IFA_LINK: retval = SCTP_SCOPE_LINK; break; case IFA_SITE: retval = SCTP_SCOPE_PRIVATE; break; default: retval = SCTP_SCOPE_GLOBAL; break; } and should be something like: v6scope = ipv6_addr_src_scope(&addr->v6.sin6_addr); switch (v6scope) { case IPV6_ADDR_SCOPE_NODELOCAL: retval = SCTP_SCOPE_LOOPBACK; break; case IPV6_ADDR_SCOPE_LINKLOCAL: retval = SCTP_SCOPE_LINK; break; case IPV6_ADDR_SCOPE_SITELOCAL: retval = SCTP_SCOPE_PRIVATE; break; case IPV6_ADDR_SCOPE_GLOBAL: retval = SCTP_SCOPE_GLOBAL; break; default: retval = SCTP_SCOPE_UNUSABLE; break; } Looks this okay for you? Then we can handle SCTP_SCOPE_UNUSABLE, too. - Alex ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] nfs: fix dead code of ipv6_addr_scope @ 2013-12-21 13:32 ` Alexander Aring 0 siblings, 0 replies; 13+ messages in thread From: Alexander Aring @ 2013-12-21 13:32 UTC (permalink / raw) To: linux-nfs-u79uwXL29TY76Z2rM5mHXA, netdev-u79uwXL29TY76Z2rM5mHXA, werner-SEdMjqphH88wryQfseakQg Hi Hannes, On Sat, Dec 21, 2013 at 01:44:40PM +0100, Hannes Frederic Sowa wrote: > On Sat, Dec 21, 2013 at 05:39:04AM +0100, Alexander Aring wrote: > > The correct way to check on IPV6_ADDR_SCOPE_LINKLOCAL is to check with > > the ipv6_addr_src_scope function. > > > > Currently this can't be work, because ipv6_addr_scope returns a int with > > a mask of IPV6_ADDR_SCOPE_MASK (0x00f0U) and IPV6_ADDR_SCOPE_LINKLOCAL > > is 0x02. So the condition is always false. > > > > Signed-off-by: Alexander Aring <alex.aring-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> > > --- > > I think ipv6_addr_src_scope should be correct, can somebody from netdev ml > > confirm this please? > > I stumple over that and I did not compile and test it. Maybe this is something > > for stable? > > > > fs/nfs/nfs4filelayoutdev.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/fs/nfs/nfs4filelayoutdev.c b/fs/nfs/nfs4filelayoutdev.c > > index c7c295e5..efac602 100644 > > --- a/fs/nfs/nfs4filelayoutdev.c > > +++ b/fs/nfs/nfs4filelayoutdev.c > > @@ -95,7 +95,7 @@ same_sockaddr(struct sockaddr *addr1, struct sockaddr *addr2) > > b6 = (struct sockaddr_in6 *)addr2; > > > > /* LINKLOCAL addresses must have matching scope_id */ > > - if (ipv6_addr_scope(&a6->sin6_addr) == > > + if (ipv6_addr_src_scope(&a6->sin6_addr) == > > IPV6_ADDR_SCOPE_LINKLOCAL && > > a6->sin6_scope_id != b6->sin6_scope_id) > > return false; > > Good catch! > thanks. I am still unsure if sctp is correct or not, I think it isn't correct. Because we compare and don't check if any bit is set. We don't use IPV6_ADDR_SCOPE_TYPE here. We use IPV6_ADDR_TYPE. But we can't compare it. Current implementation is: v6scope = ipv6_addr_scope(&addr->v6.sin6_addr); switch (v6scope) { case IFA_HOST: retval = SCTP_SCOPE_LOOPBACK; break; case IFA_LINK: retval = SCTP_SCOPE_LINK; break; case IFA_SITE: retval = SCTP_SCOPE_PRIVATE; break; default: retval = SCTP_SCOPE_GLOBAL; break; } and should be something like: v6scope = ipv6_addr_src_scope(&addr->v6.sin6_addr); switch (v6scope) { case IPV6_ADDR_SCOPE_NODELOCAL: retval = SCTP_SCOPE_LOOPBACK; break; case IPV6_ADDR_SCOPE_LINKLOCAL: retval = SCTP_SCOPE_LINK; break; case IPV6_ADDR_SCOPE_SITELOCAL: retval = SCTP_SCOPE_PRIVATE; break; case IPV6_ADDR_SCOPE_GLOBAL: retval = SCTP_SCOPE_GLOBAL; break; default: retval = SCTP_SCOPE_UNUSABLE; break; } Looks this okay for you? Then we can handle SCTP_SCOPE_UNUSABLE, too. - Alex -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] nfs: fix dead code of ipv6_addr_scope 2013-12-21 13:32 ` Alexander Aring @ 2013-12-22 2:30 ` Hannes Frederic Sowa -1 siblings, 0 replies; 13+ messages in thread From: Hannes Frederic Sowa @ 2013-12-22 2:30 UTC (permalink / raw) To: Alexander Aring; +Cc: linux-nfs, netdev, werner On Sat, Dec 21, 2013 at 02:32:54PM +0100, Alexander Aring wrote: > Hi Hannes, > > On Sat, Dec 21, 2013 at 01:44:40PM +0100, Hannes Frederic Sowa wrote: > > On Sat, Dec 21, 2013 at 05:39:04AM +0100, Alexander Aring wrote: > > > The correct way to check on IPV6_ADDR_SCOPE_LINKLOCAL is to check with > > > the ipv6_addr_src_scope function. > > > > > > Currently this can't be work, because ipv6_addr_scope returns a int with > > > a mask of IPV6_ADDR_SCOPE_MASK (0x00f0U) and IPV6_ADDR_SCOPE_LINKLOCAL > > > is 0x02. So the condition is always false. > > > > > > Signed-off-by: Alexander Aring <alex.aring@gmail.com> > > > --- > > > I think ipv6_addr_src_scope should be correct, can somebody from netdev ml > > > confirm this please? > > > I stumple over that and I did not compile and test it. Maybe this is something > > > for stable? > > > > > > fs/nfs/nfs4filelayoutdev.c | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > diff --git a/fs/nfs/nfs4filelayoutdev.c b/fs/nfs/nfs4filelayoutdev.c > > > index c7c295e5..efac602 100644 > > > --- a/fs/nfs/nfs4filelayoutdev.c > > > +++ b/fs/nfs/nfs4filelayoutdev.c > > > @@ -95,7 +95,7 @@ same_sockaddr(struct sockaddr *addr1, struct sockaddr *addr2) > > > b6 = (struct sockaddr_in6 *)addr2; > > > > > > /* LINKLOCAL addresses must have matching scope_id */ > > > - if (ipv6_addr_scope(&a6->sin6_addr) == > > > + if (ipv6_addr_src_scope(&a6->sin6_addr) == > > > IPV6_ADDR_SCOPE_LINKLOCAL && > > > a6->sin6_scope_id != b6->sin6_scope_id) > > > return false; > > > > Good catch! > > > thanks. > > I am still unsure if sctp is correct or not, I think it isn't correct. > Because we compare and don't check if any bit is set. > > We don't use IPV6_ADDR_SCOPE_TYPE here. We use IPV6_ADDR_TYPE. But we can't > compare it. Actually, this is fine, too. ipv6_addr_scope does mask the addr_type with IPV6_ADDR_SCOPE_MASK (which is 0x00f0U). If you look at addrconf_core.c you see that the 4 bits stand by itself each time. Actually it seems ipv6_addr_src_scope is better suitable for multicast scope handling and ipv6_addr_scope with IFA_{HOST,LINK,SITE} is fine for non-multicast. In this case there is no difference. Maybe an int ipv6_cmp_sockaddr(struct in6_addr *a1, int scope1, struct in6_addr *a2, int scope2) or int ipv6_cmp_sockaddr(struct sockaddr_in6 *s1, struct sockaddr_in6 *s2) would be nice so we don't need to open code it everywhere. Greetings, Hannes ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] nfs: fix dead code of ipv6_addr_scope @ 2013-12-22 2:30 ` Hannes Frederic Sowa 0 siblings, 0 replies; 13+ messages in thread From: Hannes Frederic Sowa @ 2013-12-22 2:30 UTC (permalink / raw) To: Alexander Aring Cc: linux-nfs-u79uwXL29TY76Z2rM5mHXA, netdev-u79uwXL29TY76Z2rM5mHXA, werner-SEdMjqphH88wryQfseakQg On Sat, Dec 21, 2013 at 02:32:54PM +0100, Alexander Aring wrote: > Hi Hannes, > > On Sat, Dec 21, 2013 at 01:44:40PM +0100, Hannes Frederic Sowa wrote: > > On Sat, Dec 21, 2013 at 05:39:04AM +0100, Alexander Aring wrote: > > > The correct way to check on IPV6_ADDR_SCOPE_LINKLOCAL is to check with > > > the ipv6_addr_src_scope function. > > > > > > Currently this can't be work, because ipv6_addr_scope returns a int with > > > a mask of IPV6_ADDR_SCOPE_MASK (0x00f0U) and IPV6_ADDR_SCOPE_LINKLOCAL > > > is 0x02. So the condition is always false. > > > > > > Signed-off-by: Alexander Aring <alex.aring-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> > > > --- > > > I think ipv6_addr_src_scope should be correct, can somebody from netdev ml > > > confirm this please? > > > I stumple over that and I did not compile and test it. Maybe this is something > > > for stable? > > > > > > fs/nfs/nfs4filelayoutdev.c | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > diff --git a/fs/nfs/nfs4filelayoutdev.c b/fs/nfs/nfs4filelayoutdev.c > > > index c7c295e5..efac602 100644 > > > --- a/fs/nfs/nfs4filelayoutdev.c > > > +++ b/fs/nfs/nfs4filelayoutdev.c > > > @@ -95,7 +95,7 @@ same_sockaddr(struct sockaddr *addr1, struct sockaddr *addr2) > > > b6 = (struct sockaddr_in6 *)addr2; > > > > > > /* LINKLOCAL addresses must have matching scope_id */ > > > - if (ipv6_addr_scope(&a6->sin6_addr) == > > > + if (ipv6_addr_src_scope(&a6->sin6_addr) == > > > IPV6_ADDR_SCOPE_LINKLOCAL && > > > a6->sin6_scope_id != b6->sin6_scope_id) > > > return false; > > > > Good catch! > > > thanks. > > I am still unsure if sctp is correct or not, I think it isn't correct. > Because we compare and don't check if any bit is set. > > We don't use IPV6_ADDR_SCOPE_TYPE here. We use IPV6_ADDR_TYPE. But we can't > compare it. Actually, this is fine, too. ipv6_addr_scope does mask the addr_type with IPV6_ADDR_SCOPE_MASK (which is 0x00f0U). If you look at addrconf_core.c you see that the 4 bits stand by itself each time. Actually it seems ipv6_addr_src_scope is better suitable for multicast scope handling and ipv6_addr_scope with IFA_{HOST,LINK,SITE} is fine for non-multicast. In this case there is no difference. Maybe an int ipv6_cmp_sockaddr(struct in6_addr *a1, int scope1, struct in6_addr *a2, int scope2) or int ipv6_cmp_sockaddr(struct sockaddr_in6 *s1, struct sockaddr_in6 *s2) would be nice so we don't need to open code it everywhere. Greetings, Hannes -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] nfs: fix dead code of ipv6_addr_scope @ 2013-12-22 12:30 ` Alexander Aring 0 siblings, 0 replies; 13+ messages in thread From: Alexander Aring @ 2013-12-22 12:30 UTC (permalink / raw) To: linux-nfs, netdev, werner Hi Hannes, On Sun, Dec 22, 2013 at 03:30:54AM +0100, Hannes Frederic Sowa wrote: > On Sat, Dec 21, 2013 at 02:32:54PM +0100, Alexander Aring wrote: > > Hi Hannes, > > > > On Sat, Dec 21, 2013 at 01:44:40PM +0100, Hannes Frederic Sowa wrote: > > > On Sat, Dec 21, 2013 at 05:39:04AM +0100, Alexander Aring wrote: > > > > The correct way to check on IPV6_ADDR_SCOPE_LINKLOCAL is to check with > > > > the ipv6_addr_src_scope function. > > > > > > > > Currently this can't be work, because ipv6_addr_scope returns a int with > > > > a mask of IPV6_ADDR_SCOPE_MASK (0x00f0U) and IPV6_ADDR_SCOPE_LINKLOCAL > > > > is 0x02. So the condition is always false. > > > > > > > > Signed-off-by: Alexander Aring <alex.aring@gmail.com> > > > > --- > > > > I think ipv6_addr_src_scope should be correct, can somebody from netdev ml > > > > confirm this please? > > > > I stumple over that and I did not compile and test it. Maybe this is something > > > > for stable? > > > > > > > > fs/nfs/nfs4filelayoutdev.c | 2 +- > > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > > > diff --git a/fs/nfs/nfs4filelayoutdev.c b/fs/nfs/nfs4filelayoutdev.c > > > > index c7c295e5..efac602 100644 > > > > --- a/fs/nfs/nfs4filelayoutdev.c > > > > +++ b/fs/nfs/nfs4filelayoutdev.c > > > > @@ -95,7 +95,7 @@ same_sockaddr(struct sockaddr *addr1, struct sockaddr *addr2) > > > > b6 = (struct sockaddr_in6 *)addr2; > > > > > > > > /* LINKLOCAL addresses must have matching scope_id */ > > > > - if (ipv6_addr_scope(&a6->sin6_addr) == > > > > + if (ipv6_addr_src_scope(&a6->sin6_addr) == > > > > IPV6_ADDR_SCOPE_LINKLOCAL && > > > > a6->sin6_scope_id != b6->sin6_scope_id) > > > > return false; > > > > > > Good catch! > > > > > thanks. > > > > I am still unsure if sctp is correct or not, I think it isn't correct. > > Because we compare and don't check if any bit is set. > > > > We don't use IPV6_ADDR_SCOPE_TYPE here. We use IPV6_ADDR_TYPE. But we can't > > compare it. > > Actually, this is fine, too. ipv6_addr_scope does mask the addr_type with > IPV6_ADDR_SCOPE_MASK (which is 0x00f0U). If you look at addrconf_core.c you > see that the 4 bits stand by itself each time. > > Actually it seems ipv6_addr_src_scope is better suitable for multicast scope > handling and ipv6_addr_scope with IFA_{HOST,LINK,SITE} is fine for > non-multicast. In this case there is no difference. > ah thanks, now I understand it! so an alternative would be: if (ipv6_addr_scope(&a6->sin6_addr) & IPV6_ADDR_LINKLOCAL && a6->sin6_scope_id != b6->sin6_scope_id) ... maybe this is a little bit faster instead of ipv6_addr_src_scope. Should I resend a v2 with the faster solution? > Maybe an int ipv6_cmp_sockaddr(struct in6_addr *a1, int scope1, > struct in6_addr *a2, int scope2) > or > int ipv6_cmp_sockaddr(struct sockaddr_in6 *s1, > struct sockaddr_in6 *s2) > I don't understand why we need such a function here. We only check if "a6" is linklocal and has a different sin6_scope_id than "b6" sin6_scope_id and we don't compare "a6" and "b6" here (then "b6" should be a linklocal, too). I think it's too abstract for me what exactly "compare" means in this case. :-) - Alex ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] nfs: fix dead code of ipv6_addr_scope @ 2013-12-22 12:30 ` Alexander Aring 0 siblings, 0 replies; 13+ messages in thread From: Alexander Aring @ 2013-12-22 12:30 UTC (permalink / raw) To: linux-nfs-u79uwXL29TY76Z2rM5mHXA, netdev-u79uwXL29TY76Z2rM5mHXA, werner-SEdMjqphH88wryQfseakQg Hi Hannes, On Sun, Dec 22, 2013 at 03:30:54AM +0100, Hannes Frederic Sowa wrote: > On Sat, Dec 21, 2013 at 02:32:54PM +0100, Alexander Aring wrote: > > Hi Hannes, > > > > On Sat, Dec 21, 2013 at 01:44:40PM +0100, Hannes Frederic Sowa wrote: > > > On Sat, Dec 21, 2013 at 05:39:04AM +0100, Alexander Aring wrote: > > > > The correct way to check on IPV6_ADDR_SCOPE_LINKLOCAL is to check with > > > > the ipv6_addr_src_scope function. > > > > > > > > Currently this can't be work, because ipv6_addr_scope returns a int with > > > > a mask of IPV6_ADDR_SCOPE_MASK (0x00f0U) and IPV6_ADDR_SCOPE_LINKLOCAL > > > > is 0x02. So the condition is always false. > > > > > > > > Signed-off-by: Alexander Aring <alex.aring-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> > > > > --- > > > > I think ipv6_addr_src_scope should be correct, can somebody from netdev ml > > > > confirm this please? > > > > I stumple over that and I did not compile and test it. Maybe this is something > > > > for stable? > > > > > > > > fs/nfs/nfs4filelayoutdev.c | 2 +- > > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > > > diff --git a/fs/nfs/nfs4filelayoutdev.c b/fs/nfs/nfs4filelayoutdev.c > > > > index c7c295e5..efac602 100644 > > > > --- a/fs/nfs/nfs4filelayoutdev.c > > > > +++ b/fs/nfs/nfs4filelayoutdev.c > > > > @@ -95,7 +95,7 @@ same_sockaddr(struct sockaddr *addr1, struct sockaddr *addr2) > > > > b6 = (struct sockaddr_in6 *)addr2; > > > > > > > > /* LINKLOCAL addresses must have matching scope_id */ > > > > - if (ipv6_addr_scope(&a6->sin6_addr) == > > > > + if (ipv6_addr_src_scope(&a6->sin6_addr) == > > > > IPV6_ADDR_SCOPE_LINKLOCAL && > > > > a6->sin6_scope_id != b6->sin6_scope_id) > > > > return false; > > > > > > Good catch! > > > > > thanks. > > > > I am still unsure if sctp is correct or not, I think it isn't correct. > > Because we compare and don't check if any bit is set. > > > > We don't use IPV6_ADDR_SCOPE_TYPE here. We use IPV6_ADDR_TYPE. But we can't > > compare it. > > Actually, this is fine, too. ipv6_addr_scope does mask the addr_type with > IPV6_ADDR_SCOPE_MASK (which is 0x00f0U). If you look at addrconf_core.c you > see that the 4 bits stand by itself each time. > > Actually it seems ipv6_addr_src_scope is better suitable for multicast scope > handling and ipv6_addr_scope with IFA_{HOST,LINK,SITE} is fine for > non-multicast. In this case there is no difference. > ah thanks, now I understand it! so an alternative would be: if (ipv6_addr_scope(&a6->sin6_addr) & IPV6_ADDR_LINKLOCAL && a6->sin6_scope_id != b6->sin6_scope_id) ... maybe this is a little bit faster instead of ipv6_addr_src_scope. Should I resend a v2 with the faster solution? > Maybe an int ipv6_cmp_sockaddr(struct in6_addr *a1, int scope1, > struct in6_addr *a2, int scope2) > or > int ipv6_cmp_sockaddr(struct sockaddr_in6 *s1, > struct sockaddr_in6 *s2) > I don't understand why we need such a function here. We only check if "a6" is linklocal and has a different sin6_scope_id than "b6" sin6_scope_id and we don't compare "a6" and "b6" here (then "b6" should be a linklocal, too). I think it's too abstract for me what exactly "compare" means in this case. :-) - Alex -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] nfs: fix dead code of ipv6_addr_scope 2013-12-22 12:30 ` Alexander Aring @ 2013-12-22 12:38 ` Hannes Frederic Sowa -1 siblings, 0 replies; 13+ messages in thread From: Hannes Frederic Sowa @ 2013-12-22 12:38 UTC (permalink / raw) To: Alexander Aring; +Cc: linux-nfs, netdev, werner On Sun, Dec 22, 2013 at 01:30:14PM +0100, Alexander Aring wrote: > On Sun, Dec 22, 2013 at 03:30:54AM +0100, Hannes Frederic Sowa wrote: > > On Sat, Dec 21, 2013 at 02:32:54PM +0100, Alexander Aring wrote: > > > Hi Hannes, > > > > > > On Sat, Dec 21, 2013 at 01:44:40PM +0100, Hannes Frederic Sowa wrote: > > > > On Sat, Dec 21, 2013 at 05:39:04AM +0100, Alexander Aring wrote: > > > > > The correct way to check on IPV6_ADDR_SCOPE_LINKLOCAL is to check with > > > > > the ipv6_addr_src_scope function. > > > > > > > > > > Currently this can't be work, because ipv6_addr_scope returns a int with > > > > > a mask of IPV6_ADDR_SCOPE_MASK (0x00f0U) and IPV6_ADDR_SCOPE_LINKLOCAL > > > > > is 0x02. So the condition is always false. > > > > > > > > > > Signed-off-by: Alexander Aring <alex.aring@gmail.com> > > > > > --- > > > > > I think ipv6_addr_src_scope should be correct, can somebody from netdev ml > > > > > confirm this please? > > > > > I stumple over that and I did not compile and test it. Maybe this is something > > > > > for stable? > > > > > > > > > > fs/nfs/nfs4filelayoutdev.c | 2 +- > > > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > > > > > diff --git a/fs/nfs/nfs4filelayoutdev.c b/fs/nfs/nfs4filelayoutdev.c > > > > > index c7c295e5..efac602 100644 > > > > > --- a/fs/nfs/nfs4filelayoutdev.c > > > > > +++ b/fs/nfs/nfs4filelayoutdev.c > > > > > @@ -95,7 +95,7 @@ same_sockaddr(struct sockaddr *addr1, struct sockaddr *addr2) > > > > > b6 = (struct sockaddr_in6 *)addr2; > > > > > > > > > > /* LINKLOCAL addresses must have matching scope_id */ > > > > > - if (ipv6_addr_scope(&a6->sin6_addr) == > > > > > + if (ipv6_addr_src_scope(&a6->sin6_addr) == > > > > > IPV6_ADDR_SCOPE_LINKLOCAL && > > > > > a6->sin6_scope_id != b6->sin6_scope_id) > > > > > return false; > > > > > > > > Good catch! > > > > > > > thanks. > > > > > > I am still unsure if sctp is correct or not, I think it isn't correct. > > > Because we compare and don't check if any bit is set. > > > > > > We don't use IPV6_ADDR_SCOPE_TYPE here. We use IPV6_ADDR_TYPE. But we can't > > > compare it. > > > > Actually, this is fine, too. ipv6_addr_scope does mask the addr_type with > > IPV6_ADDR_SCOPE_MASK (which is 0x00f0U). If you look at addrconf_core.c you > > see that the 4 bits stand by itself each time. > > > > Actually it seems ipv6_addr_src_scope is better suitable for multicast scope > > handling and ipv6_addr_scope with IFA_{HOST,LINK,SITE} is fine for > > non-multicast. In this case there is no difference. > > > ah thanks, now I understand it! > > so an alternative would be: > > if (ipv6_addr_scope(&a6->sin6_addr) & IPV6_ADDR_LINKLOCAL && > a6->sin6_scope_id != b6->sin6_scope_id) > ... > > maybe this is a little bit faster instead of ipv6_addr_src_scope. > Should I resend a v2 with the faster solution? Yes, please do so. Thanks! > > Maybe an int ipv6_cmp_sockaddr(struct in6_addr *a1, int scope1, > > struct in6_addr *a2, int scope2) > > or > > int ipv6_cmp_sockaddr(struct sockaddr_in6 *s1, > > struct sockaddr_in6 *s2) > > > > I don't understand why we need such a function here. We only check if > "a6" is linklocal and has a different sin6_scope_id than "b6" sin6_scope_id > and we don't compare "a6" and "b6" here (then "b6" should be a > linklocal, too). I think it's too abstract for me what exactly "compare" > means in this case. :-) That were exactly the semantics I had in mind. Something like ipv6_equal_sockaddr would be a better name, you are right. Trying to sort ipv6 addresses depends on the specific code and I would leave that open-coded in the specific case. Greetings, Hannes ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] nfs: fix dead code of ipv6_addr_scope @ 2013-12-22 12:38 ` Hannes Frederic Sowa 0 siblings, 0 replies; 13+ messages in thread From: Hannes Frederic Sowa @ 2013-12-22 12:38 UTC (permalink / raw) To: Alexander Aring Cc: linux-nfs-u79uwXL29TY76Z2rM5mHXA, netdev-u79uwXL29TY76Z2rM5mHXA, werner-SEdMjqphH88wryQfseakQg On Sun, Dec 22, 2013 at 01:30:14PM +0100, Alexander Aring wrote: > On Sun, Dec 22, 2013 at 03:30:54AM +0100, Hannes Frederic Sowa wrote: > > On Sat, Dec 21, 2013 at 02:32:54PM +0100, Alexander Aring wrote: > > > Hi Hannes, > > > > > > On Sat, Dec 21, 2013 at 01:44:40PM +0100, Hannes Frederic Sowa wrote: > > > > On Sat, Dec 21, 2013 at 05:39:04AM +0100, Alexander Aring wrote: > > > > > The correct way to check on IPV6_ADDR_SCOPE_LINKLOCAL is to check with > > > > > the ipv6_addr_src_scope function. > > > > > > > > > > Currently this can't be work, because ipv6_addr_scope returns a int with > > > > > a mask of IPV6_ADDR_SCOPE_MASK (0x00f0U) and IPV6_ADDR_SCOPE_LINKLOCAL > > > > > is 0x02. So the condition is always false. > > > > > > > > > > Signed-off-by: Alexander Aring <alex.aring-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> > > > > > --- > > > > > I think ipv6_addr_src_scope should be correct, can somebody from netdev ml > > > > > confirm this please? > > > > > I stumple over that and I did not compile and test it. Maybe this is something > > > > > for stable? > > > > > > > > > > fs/nfs/nfs4filelayoutdev.c | 2 +- > > > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > > > > > diff --git a/fs/nfs/nfs4filelayoutdev.c b/fs/nfs/nfs4filelayoutdev.c > > > > > index c7c295e5..efac602 100644 > > > > > --- a/fs/nfs/nfs4filelayoutdev.c > > > > > +++ b/fs/nfs/nfs4filelayoutdev.c > > > > > @@ -95,7 +95,7 @@ same_sockaddr(struct sockaddr *addr1, struct sockaddr *addr2) > > > > > b6 = (struct sockaddr_in6 *)addr2; > > > > > > > > > > /* LINKLOCAL addresses must have matching scope_id */ > > > > > - if (ipv6_addr_scope(&a6->sin6_addr) == > > > > > + if (ipv6_addr_src_scope(&a6->sin6_addr) == > > > > > IPV6_ADDR_SCOPE_LINKLOCAL && > > > > > a6->sin6_scope_id != b6->sin6_scope_id) > > > > > return false; > > > > > > > > Good catch! > > > > > > > thanks. > > > > > > I am still unsure if sctp is correct or not, I think it isn't correct. > > > Because we compare and don't check if any bit is set. > > > > > > We don't use IPV6_ADDR_SCOPE_TYPE here. We use IPV6_ADDR_TYPE. But we can't > > > compare it. > > > > Actually, this is fine, too. ipv6_addr_scope does mask the addr_type with > > IPV6_ADDR_SCOPE_MASK (which is 0x00f0U). If you look at addrconf_core.c you > > see that the 4 bits stand by itself each time. > > > > Actually it seems ipv6_addr_src_scope is better suitable for multicast scope > > handling and ipv6_addr_scope with IFA_{HOST,LINK,SITE} is fine for > > non-multicast. In this case there is no difference. > > > ah thanks, now I understand it! > > so an alternative would be: > > if (ipv6_addr_scope(&a6->sin6_addr) & IPV6_ADDR_LINKLOCAL && > a6->sin6_scope_id != b6->sin6_scope_id) > ... > > maybe this is a little bit faster instead of ipv6_addr_src_scope. > Should I resend a v2 with the faster solution? Yes, please do so. Thanks! > > Maybe an int ipv6_cmp_sockaddr(struct in6_addr *a1, int scope1, > > struct in6_addr *a2, int scope2) > > or > > int ipv6_cmp_sockaddr(struct sockaddr_in6 *s1, > > struct sockaddr_in6 *s2) > > > > I don't understand why we need such a function here. We only check if > "a6" is linklocal and has a different sin6_scope_id than "b6" sin6_scope_id > and we don't compare "a6" and "b6" here (then "b6" should be a > linklocal, too). I think it's too abstract for me what exactly "compare" > means in this case. :-) That were exactly the semantics I had in mind. Something like ipv6_equal_sockaddr would be a better name, you are right. Trying to sort ipv6 addresses depends on the specific code and I would leave that open-coded in the specific case. Greetings, Hannes -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2013-12-22 12:38 UTC | newest] Thread overview: 13+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2013-12-21 4:39 [PATCH] nfs: fix dead code of ipv6_addr_scope Alexander Aring 2013-12-21 4:39 ` Alexander Aring 2013-12-21 8:17 ` Alexander Aring 2013-12-21 12:44 ` Hannes Frederic Sowa 2013-12-21 12:44 ` Hannes Frederic Sowa 2013-12-21 13:32 ` Alexander Aring 2013-12-21 13:32 ` Alexander Aring 2013-12-22 2:30 ` Hannes Frederic Sowa 2013-12-22 2:30 ` Hannes Frederic Sowa 2013-12-22 12:30 ` Alexander Aring 2013-12-22 12:30 ` Alexander Aring 2013-12-22 12:38 ` Hannes Frederic Sowa 2013-12-22 12:38 ` Hannes Frederic Sowa
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.