SUNRPC: svc_register error overwritten in next iteration

The break is in the inner loop, the svc_register() error is overwritten in the next iteration. Only the error in the last iteration is returned.

Signed-off-by Roel Kluin <[email protected]>

---
net/sunrpc/svc.c | 2 ++

1 files changed, 2 insertions(+), 0 deletions(-)

Is this needed?

net/sunrpc/svc.c +2 -0

index 08e05a8..5fd08c0 100644
@@ -889,6 +889,8 @@ int svc_register(const struct svc_serv *serv, const int family,
 			if (error < 0)
 				break;
 		}
+		if (error < 0)
+			break;
 	}
 
 	return error;

The break is in the inner loop, the svc_register() error is overwritten in the next iteration. Only the error in the last iteration is returned.

Signed-off-by Roel Kluin <[email protected]>

---
 net/sunrpc/svc.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

Is this needed?

net/sunrpc/svc.c +0 -0

index 08e05a8..5fd08c0 100644

net/sunrpc/svc.c +2 -0

 		}
+		if (error < 0)
+			break;
 	}
 
 	return error;

Quoted 15 lines · roel

The break is in the inner loop, the svc_register() error is overwritten in the next iteration. Only the error in the last iteration is returned.

Signed-off-by Roel Kluin <[email protected]>

---
net/sunrpc/svc.c | 2 ++

1 files changed, 2 insertions(+), 0 deletions(-)

Is this needed?

net/sunrpc/svc.c +0 -0

index 08e05a8..5fd08c0 100644

net/sunrpc/svc.c int svc_register(const struct svc_serv *serv, const int family, · line 890

Quoted 2 lines

net/sunrpc/svc.c +0 -0

@@ -889,6 +889,8 @@ int svc_register(const struct svc_serv *serv, const int family,
 			if (error < 0)
 				break;

May as well just "goto out" or "return error" here?

But: aren't we missing some cleanup? If we succesfully register one program then fail at a second one, don't we need to unregister the first?

--b.

net/sunrpc/svc.c int svc_register(const struct svc_serv *serv, const int family, · line 896

Quoted 6 lines

net/sunrpc/svc.c +2 -0

 		}
+		if (error < 0)
+			break;
 	}
 
 	return error;

--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 21 lines · J. Bruce Fields

Quoted 15 lines · roel

The break is in the inner loop, the svc_register() error is overwritten in the next iteration. Only the error in the last iteration is returned.

Signed-off-by Roel Kluin <[email protected]>

---
net/sunrpc/svc.c | 2 ++

1 files changed, 2 insertions(+), 0 deletions(-)

Is this needed?

net/sunrpc/svc.c +0 -0

index 08e05a8..5fd08c0 100644

net/sunrpc/svc.c int svc_register(const struct svc_serv *serv, const int family, · line 890

Quoted 2 lines

net/sunrpc/svc.c +0 -0

@@ -889,6 +889,8 @@ int svc_register(const struct svc_serv *serv, const int family,
 			if (error < 0)
 				break;

May as well just "goto out" or "return error" here?

Quoted 3 lines

But: aren't we missing some cleanup? If we succesfully register one program then fail at a second one, don't we need to unregister the first?

Right. I don't understand what is the intended effect here (of the original code): Best effort registration, or "all or none"?

Quoted 24 lines · Chuck Lever

Quoted 21 lines · J. Bruce Fields

Quoted 15 lines · roel

The break is in the inner loop, the svc_register() error is overwritten in the next iteration. Only the error in the last iteration is returned.

Signed-off-by Roel Kluin <[email protected]>

---
net/sunrpc/svc.c | 2 ++

1 files changed, 2 insertions(+), 0 deletions(-)

Is this needed?

net/sunrpc/svc.c +0 -0

index 08e05a8..5fd08c0 100644

net/sunrpc/svc.c int svc_register(const struct svc_serv *serv, const int family, · line 890

Quoted 2 lines

net/sunrpc/svc.c +0 -0

@@ -889,6 +889,8 @@ int svc_register(const struct svc_serv *serv, const int family,
 			if (error < 0)
 				break;

May as well just "goto out" or "return error" here?

Quoted 5 lines

Quoted 3 lines

But: aren't we missing some cleanup? If we succesfully register one program then fail at a second one, don't we need to unregister the first?

Right. I don't understand what is the intended effect here (of the original code): Best effort registration, or "all or none"?

The current code was failing iff the last registration returns an error. We list the nfs program before the acl program in this list, so nfsd registration was failing iff the acl program failed, which makes no sense whatsoever.

I think "all or none" would be cleanest.

If people start complaining that they don't want to run rpcbind/portmap then we could give them some way of requesting that instead of just depending on allowing the registration to fail.

For cleanup, we can just unregister everything, right? (No harm in possibly unregistering something who's registration just failed?)

Quoted 5 lines · J. Bruce Fields

The current code was failing iff the last registration returns an error. We list the nfs program before the acl program in this list, so nfsd registration was failing iff the acl program failed, which makes no sense whatsoever.

Quoted 5 lines

I think "all or none" would be cleanest.

If people start complaining that they don't want to run rpcbind/portmap then we could give them some way of requesting that instead of just depending on allowing the registration to fail.

I thought vs_hidden was set for NFSACL... but maybe I was wrong about that.

Quoted 2 lines

For cleanup, we can just unregister everything, right? (No harm in possibly unregistering something who's registration just failed?)

Yes. As a simple hard-headed approach, probably you should walk the passed-in sv_program list again and unregister each item in the list. The downside to this is if the upcall is taking a long time (for instance, if networking is not available). It would double the amount of time for svc_register() to return a failure.

However, be prepared: I bet such a change could expose bugs in the NFSD start up stack.

:-( Maybe it deserves some soak-time in linux-next.

Quoted 7 lines · Chuck Lever

Quoted 5 lines · J. Bruce Fields

The current code was failing iff the last registration returns an error. We list the nfs program before the acl program in this list, so nfsd registration was failing iff the acl program failed, which makes no sense whatsoever.

Quoted 7 lines

Quoted 5 lines

I think "all or none" would be cleanest.

If people start complaining that they don't want to run rpcbind/portmap then we could give them some way of requesting that instead of just depending on allowing the registration to fail.

I thought vs_hidden was set for NFSACL... but maybe I was wrong about that.

Oh, I forgot about that.

But, checking.... Actually, it looks like it's not set for NFSACL--from a quick grep, it appears that only the callback server sets it.

Quoted 5 lines

Quoted 2 lines

For cleanup, we can just unregister everything, right? (No harm in possibly unregistering something who's registration just failed?)

Yes. As a simple hard-headed approach, probably you should walk the passed-in sv_program list again and unregister each item in the list. The downside to this is if the upcall is taking a long time (for instance, if networking is not available). It would double the amount of time for svc_register() to return a failure.

Quoted 1 line

However, be prepared: I bet such a change could expose bugs in the NFSD start up stack.

There are so many to expose.

Quoted 1 line

:-( Maybe it deserves some soak-time in linux-next.

Sure.

--b.
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html