sunrpc: fix typo in svc_udp_recvfrom

I'm not sure whether this has been brought up already - I came across this today when poking around the code...

Olaf

--[ cut here ]--
Fix a typo in svc_udp_recvfrom

This patch corrects a small and hamless typo in svc_udp_recvfrom.

Signed-off-by [email protected]

---
 net/sunrpc/svcsock.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Index: linux-2.6/net/sunrpc/svcsock.c
===================================================================

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.

Quoted 3 lines · Trond Myklebust

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

This patch corrects a small and harmless typo in svc_udp_recvfrom.

Signed-off-by [email protected]

---
 net/sunrpc/svcsock.c |   11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

Index: linux-2.6/net/sunrpc/svcsock.c
===================================================================

net/sunrpc/svcsock.c +1 -3

@@ -779,9 +779,14 @@ 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) {
+	while (1) {

Quoted 3 lines · Trond Myklebust

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

What about this (on top of current -mm).

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

net/sunrpc/svcsock.c +13 -8

@@ -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) {

Hi Neil,

Quoted 1 line · Neil Brown

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

Quoted 10 lines

net/sunrpc/svcsock.c +3 -6

 	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);

net/sunrpc/svcsock.c svc_udp_recvfrom(struct svc_rqst *rqstp) · line 792

Quoted 8 lines

net/sunrpc/svcsock.c +8 -0

+	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);

Quoted 9 lines

net/sunrpc/svcsock.c +2 -2

 		}
-		/* 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) {

Signature · 2 lines

Olaf Kirch  |  --- o --- Nous sommes du soleil we love when we play
[email protected] |    / | \   sol.dhoop.naytheet.ah kin.ir.samse.qurax

Quoted 5 lines

net/sunrpc/svcsock.c svc_udp_recvfrom(struct svc_rqst *rqstp) · line 792

Quoted 5 lines

net/sunrpc/svcsock.c +5 -0

+	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);

                        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^