On Tuesday March 27, [email protected] wrote:
> On Tuesday 27 March 2007 17:10, Trond Myklebust wrote:
> > Could you please rather move those assignments out of the while()? Good
> > coding style dictates that the above should be avoided. One of the
> > benefits is precisely to avoid the above ambiguities.
>
> Okay, here we go:
> ----------------------
> Fix a typo in svc_udp_recvfrom
Thanks...
This is already in -mm : net-sunrpc-svcsockc-fix-a-check.patch
Hopefully it will get to -linus soon.
Apparently coverty found it.
While I appreciate that having assignments inside conditionals can
hurt readability, I don't find the new version particularly
readable... The logic seems to be obscured. Maybe it is just obscure
logic?
What about this (on top of current -mm).
NeilBrown
Signed-off-by: Neil Brown <[email protected]>
### Diffstat output
./net/sunrpc/svcsock.c | 21 +++++++++++++--------
1 file changed, 13 insertions(+), 8 deletions(-)
diff .prev/net/sunrpc/svcsock.c ./net/sunrpc/svcsock.c
--- .prev/net/sunrpc/svcsock.c 2007-03-28 12:06:59.000000000 +1000
+++ ./net/sunrpc/svcsock.c 2007-03-28 12:12:50.000000000 +1000
@@ -779,15 +779,20 @@ svc_udp_recvfrom(struct svc_rqst *rqstp)
}
clear_bit(SK_DATA, &svsk->sk_flags);
- while ((err = kernel_recvmsg(svsk->sk_sock, &msg, NULL,
- 0, 0, MSG_PEEK | MSG_DONTWAIT)) < 0 ||
- (skb = skb_recv_datagram(svsk->sk_sk, 0, 1, &err)) == NULL) {
- if (err == -EAGAIN) {
- svc_sock_received(svsk);
- return err;
+ skb = NULL;
+ err = kernel_recvmsg(svsk->sk_sock, &msg, NULL,
+ 0, 0, MSG_PEEK | MSG_DONTWAIT);
+ if (err >= 0)
+ skb = skb_recv_datagram(svsk->sk_sk, 0, 1, &err);
+
+ if (skb == NULL) {
+ if (err != -EAGAIN) {
+ /* possibly an icmp error */
+ dprintk("svc: recvfrom returned error %d\n", -err);
+ set_bit(SK_DATA, &svsk->sk_flags);
}
- /* possibly an icmp error */
- dprintk("svc: recvfrom returned error %d\n", -err);
+ svc_sock_received(svsk);
+ return -EAGAIN;
}
rqstp->rq_addrlen = sizeof(rqstp->rq_addr);
if (skb->tstamp.off_sec == 0) {
-------------------------------------------------------------------------
Take Surveys. Earn Cash. Influence the Future of IT
Join SourceForge.net's Techsay panel and you'll get the chance to share your
opinions on IT & business topics through brief surveys-and earn cash
http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV
_______________________________________________
NFS maillist - [email protected]
https://lists.sourceforge.net/lists/listinfo/nfs
Hi Neil,
On Wednesday 28 March 2007 04:16, Neil Brown wrote:
> What about this (on top of current -mm).
Well, you just converted a while loop to an if. The old
code would pull any ICMP signalled error condition out
of the socket, and then try to get the data. The new
code will return if it hits an ICMP error, without
reasserting SK_DATA. Any pending UDP request will
be stuck until the client retransmits.
So I think you really need the loop here.
Olaf
>
> Signed-off-by: Neil Brown <[email protected]>
>
> ### Diffstat output
> ./net/sunrpc/svcsock.c | 21 +++++++++++++--------
> 1 file changed, 13 insertions(+), 8 deletions(-)
>
> diff .prev/net/sunrpc/svcsock.c ./net/sunrpc/svcsock.c
> --- .prev/net/sunrpc/svcsock.c 2007-03-28 12:06:59.000000000 +1000
> +++ ./net/sunrpc/svcsock.c 2007-03-28 12:12:50.000000000 +1000
> @@ -779,15 +779,20 @@ svc_udp_recvfrom(struct svc_rqst *rqstp)
> }
>
> clear_bit(SK_DATA, &svsk->sk_flags);
> - while ((err = kernel_recvmsg(svsk->sk_sock, &msg, NULL,
> - 0, 0, MSG_PEEK | MSG_DONTWAIT)) < 0 ||
> - (skb = skb_recv_datagram(svsk->sk_sk, 0, 1, &err)) == NULL) {
> - if (err == -EAGAIN) {
> - svc_sock_received(svsk);
> - return err;
> + skb = NULL;
> + err = kernel_recvmsg(svsk->sk_sock, &msg, NULL,
> + 0, 0, MSG_PEEK | MSG_DONTWAIT);
> + if (err >= 0)
> + skb = skb_recv_datagram(svsk->sk_sk, 0, 1, &err);
> +
> + if (skb == NULL) {
> + if (err != -EAGAIN) {
> + /* possibly an icmp error */
> + dprintk("svc: recvfrom returned error %d\n", -err);
> + set_bit(SK_DATA, &svsk->sk_flags);
> }
> - /* possibly an icmp error */
> - dprintk("svc: recvfrom returned error %d\n", -err);
> + svc_sock_received(svsk);
> + return -EAGAIN;
> }
> rqstp->rq_addrlen = sizeof(rqstp->rq_addr);
> if (skb->tstamp.off_sec == 0) {
>
--
Olaf Kirch | --- o --- Nous sommes du soleil we love when we play
[email protected] | / | \ sol.dhoop.naytheet.ah kin.ir.samse.qurax
-------------------------------------------------------------------------
Take Surveys. Earn Cash. Influence the Future of IT
Join SourceForge.net's Techsay panel and you'll get the chance to share your
opinions on IT & business topics through brief surveys-and earn cash
http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV
_______________________________________________
NFS maillist - [email protected]
https://lists.sourceforge.net/lists/listinfo/nfs