staging: lustre: Fix unneeded byte-ordering cast
PATCH 1/1 staging: lustre: Fix unneeded byte-ordering cast
Fix sparse warning:
CHECK drivers/staging//lustre/lnet/lnet/acceptor.c
drivers/staging//lustre/lnet/lnet/acceptor.c:243:30: warning: cast to restricted __le32
LNET_PROTO_TCP_MAGIC, as a define, is already CPU byte-ordered when compared to 'magic', so no need for a cast.
--- drivers/staging/lustre/lnet/lnet/acceptor.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/staging/lustre/lnet/lnet/acceptor.c +1 -1
@@ -240,7 +240,7 @@ lnet_accept(struct socket *sock, __u32 magic)return -EPROTO ; } -if (magic ==le32_to_cpu (LNET_PROTO_TCP_MAGIC )) +if (magic ==LNET_PROTO_TCP_MAGIC ) str ="'old' socknal/tcpnal" ;else str ="unrecognised" ;
Signature · 6 lines
2.16.2 _______________________________________________ devel mailing list [email protected] http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Quoted 19 lines · Justin Skists
Fix sparse warning:
CHECK drivers/staging//lustre/lnet/lnet/acceptor.c
drivers/staging//lustre/lnet/lnet/acceptor.c:243:30: warning: cast to restricted__le32
LNET_PROTO_TCP_MAGIC, as a define, is already CPU byte-ordered when compared to 'magic', so no need for a cast.Signed-off-by Justin Skists <[email protected]>--- drivers/staging/lustre/lnet/lnet/acceptor.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)drivers/staging/lustre/lnet/lnet/acceptor.c +0
-0
drivers/staging/lustre/lnet/lnet/acceptor.c lnet_accept(struct socket *sock, __u32 magic) · line 246
Quoted 8 lines
drivers/staging/lustre/lnet/lnet/acceptor.c +1
-1@@ -240,7 +240,7 @@ lnet_accept(struct socket *sock, __u32 magic)return -EPROTO ; } -if (magic ==le32_to_cpu (LNET_PROTO_TCP_MAGIC )) +if (magic ==LNET_PROTO_TCP_MAGIC ) str ="'old' socknal/tcpnal" ;else str ="unrecognised" ;
This code is almost completely irrelevant (it just choose which error message to use when failing), but we may as well get it right and I cannot see why your change is a fix.
"magic" was passed as an argument from lnet_acceptor() to lnet_accept(), and lnet_acceptor() got it by reading bytes off the network with lnet_sock_read().
My knowledge is far from complete, but from what I've seen, lustre sends data in host-byte-order on the sender, and expects the receiver to determine which byte-order that is (often by looking at a "magic" word like this) and do any byte-swap that is necessary.
While I agree that LNET_PROTO_TCP_MAGIC is in host-byte-order so calling le32_to_cpu() on it makes no sense, I don't agree that "magic" is also host byte-ordered.
I suspect a more correct fix would be to use
lnet_accept_magic(magic, LNET_PROTO_TCP_MAGIC)
as the condition of the if(). This is consistent with other code that tests magic, and it is consistent with the general understanding that "magic" should be in host-byte-order for the peer which sent the message.
Could you resubmit with that change?
Thanks,
NeilBrown
_______________________________________________
devel mailing list
[email protected]
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Quoted 24 lines · NeilBrown
Quoted 22 lines · Justin Skists
Fix sparse warning:
CHECK drivers/staging//lustre/lnet/lnet/acceptor.c
drivers/staging//lustre/lnet/lnet/acceptor.c:243:30: warning: cast to restricted__le32
LNET_PROTO_TCP_MAGIC, as a define, is already CPU byte-ordered when compared to 'magic', so no need for a cast.Signed-off-by Justin Skists <[email protected]>--- drivers/staging/lustre/lnet/lnet/acceptor.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)drivers/staging/lustre/lnet/lnet/acceptor.c +0
-0@@ -240,7 +240,7 @@ lnet_accept(struct socket *sock, __u32 magic)return -EPROTO ; }
Quoted 9 lines
drivers/staging/lustre/lnet/lnet/acceptor.c lnet_accept(struct socket *sock, __u32 magic) · line 246
Quoted 5 lines
drivers/staging/lustre/lnet/lnet/acceptor.c +1
-1-if (magic ==le32_to_cpu (LNET_PROTO_TCP_MAGIC )) +if (magic ==LNET_PROTO_TCP_MAGIC ) str ="'old' socknal/tcpnal" ;else str ="unrecognised" ;This code is almost completely irrelevant (it just choose which error message to use when failing), but we may as well get it right and I cannot see why your change is a fix.
I admit that the change is trivial, and, in hindsight, the word fix is a little "strong".
The rationale was that the if-statement, as it was, probably wouldn't work as intented on big-endian systems.
I chose this sparse warning to test the waters, as it was an isolated change, before I thought about proposing a bigger change: There are quite a few sparse warning in regards to struct lnet_hdr with regards to __u32 vs. __le32 (etc.) restricted castings.
Quoted 8 lines
I suspect a more correct fix would be to use
lnet_accept_magic(magic, LNET_PROTO_TCP_MAGIC)
as the condition of theif(). This is consistent with other code that tests magic, and it is consistent with the general understanding that "magic" should be in host-byte-order for the peer which sent the message.Could you resubmit with that change?
I agree that your suggestion is a much better fix.
As Greg has already accepted the patch in question into staging-next, would the correct course of action be for me to submit a new patch with a "fixes" tag based on staging-next? Or would Greg prefer to drop the previous one for a fresh v2?
Regards,
Justin.
_______________________________________________
devel mailing list
[email protected]
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel