[PATCH] SUNRPC: svc_register error overwritten in next iteration

[PATCH] SUNRPC: svc_register error overwritten in next iteration

[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 <roel.kluin@gmail.com>
---
 net/sunrpc/svc.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

Is this needed?

diff --git a/net/sunrpc/svc.c b/net/sunrpc/svc.c
index 08e05a8..5fd08c0 100644
--- a/net/sunrpc/svc.c
+++ b/net/sunrpc/svc.c
@@ -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;

Re: [PATCH] SUNRPC: svc_register error overwritten in next iteration

[Chuck Lever]

On Mar 12, 2011, at 8:27 AM, roel wrote:

> 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 <roel.kluin@gmail.com>
> ---
> net/sunrpc/svc.c |    2 ++
> 1 files changed, 2 insertions(+), 0 deletions(-)
> 
> Is this needed?

Maybe.  Did you encounter a problem here?

> diff --git a/net/sunrpc/svc.c b/net/sunrpc/svc.c
> index 08e05a8..5fd08c0 100644
> --- a/net/sunrpc/svc.c
> +++ b/net/sunrpc/svc.c
> @@ -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;

-- 
Chuck Lever
chuck[dot]lever[at]oracle[dot]com

Re: [PATCH] SUNRPC: svc_register error overwritten in next iteration

[J. Bruce Fields]

On Sat, Mar 12, 2011 at 02:27:35PM +0100, roel wrote:
> 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 <roel.kluin@gmail.com>
> ---
>  net/sunrpc/svc.c |    2 ++
>  1 files changed, 2 insertions(+), 0 deletions(-)
> 
> Is this needed?
> 
> diff --git a/net/sunrpc/svc.c b/net/sunrpc/svc.c
> index 08e05a8..5fd08c0 100644
> --- a/net/sunrpc/svc.c
> +++ b/net/sunrpc/svc.c
> @@ -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.

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

Re: [PATCH] SUNRPC: svc_register error overwritten in next iteration

[Chuck Lever]

On Mar 14, 2011, at 6:36 PM, J. Bruce Fields wrote:

> On Sat, Mar 12, 2011 at 02:27:35PM +0100, roel wrote:
>> 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 <roel.kluin@gmail.com>
>> ---
>> net/sunrpc/svc.c |    2 ++
>> 1 files changed, 2 insertions(+), 0 deletions(-)
>> 
>> Is this needed?
>> 
>> diff --git a/net/sunrpc/svc.c b/net/sunrpc/svc.c
>> index 08e05a8..5fd08c0 100644
>> --- a/net/sunrpc/svc.c
>> +++ b/net/sunrpc/svc.c
>> @@ -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?

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

> --b.
> 
>> 		}
>> +		if (error < 0)
>> +			break;
>> 	}
>> 
>> 	return error;
> --
> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

-- 
Chuck Lever
chuck[dot]lever[at]oracle[dot]com

Re: [PATCH] SUNRPC: svc_register error overwritten in next iteration

[J. Bruce Fields]

On Tue, Mar 15, 2011 at 11:43:32AM -0400, Chuck Lever wrote:
> 
> On Mar 14, 2011, at 6:36 PM, J. Bruce Fields wrote:
> 
> > On Sat, Mar 12, 2011 at 02:27:35PM +0100, roel wrote:
> >> 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 <roel.kluin@gmail.com>
> >> ---
> >> net/sunrpc/svc.c |    2 ++
> >> 1 files changed, 2 insertions(+), 0 deletions(-)
> >> 
> >> Is this needed?
> >> 
> >> diff --git a/net/sunrpc/svc.c b/net/sunrpc/svc.c
> >> index 08e05a8..5fd08c0 100644
> >> --- a/net/sunrpc/svc.c
> >> +++ b/net/sunrpc/svc.c
> >> @@ -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?
> 
> 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?)

--b.

Re: [PATCH] SUNRPC: svc_register error overwritten in next iteration

[Chuck Lever]

On Mar 15, 2011, at 12:13 PM, J. Bruce Fields wrote:

> On Tue, Mar 15, 2011 at 11:43:32AM -0400, Chuck Lever wrote:
>> 
>> On Mar 14, 2011, at 6:36 PM, J. Bruce Fields wrote:
>> 
>>> On Sat, Mar 12, 2011 at 02:27:35PM +0100, roel wrote:
>>>> 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 <roel.kluin@gmail.com>
>>>> ---
>>>> net/sunrpc/svc.c |    2 ++
>>>> 1 files changed, 2 insertions(+), 0 deletions(-)
>>>> 
>>>> Is this needed?
>>>> 
>>>> diff --git a/net/sunrpc/svc.c b/net/sunrpc/svc.c
>>>> index 08e05a8..5fd08c0 100644
>>>> --- a/net/sunrpc/svc.c
>>>> +++ b/net/sunrpc/svc.c
>>>> @@ -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?
>> 
>> 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.

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

> 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.

-- 
Chuck Lever
chuck[dot]lever[at]oracle[dot]com

Re: [PATCH] SUNRPC: svc_register error overwritten in next iteration

[J. Bruce Fields]

On Tue, Mar 15, 2011 at 12:54:01PM -0400, Chuck Lever wrote:
> 
> On Mar 15, 2011, at 12:13 PM, J. Bruce Fields wrote:
> > 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.
> 
> 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.

> > 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.

There are so many to expose.

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

Sure.

--b.

Re: [PATCH] SUNRPC: svc_register error overwritten in next iteration

[J. Bruce Fields]

On Tue, Mar 15, 2011 at 12:57:39PM -0400, J. Bruce Fields wrote:
> On Tue, Mar 15, 2011 at 12:54:01PM -0400, Chuck Lever wrote:
> > 
> > On Mar 15, 2011, at 12:13 PM, J. Bruce Fields wrote:
> > > 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.
> > 
> > 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.
> 
> > > 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.
> 
> There are so many to expose.
> 
> > :-(  Maybe it deserves some soak-time in linux-next.
> 
> Sure.

(Roel Kluin, could you revise and resubmit that?  I'd do it in two
patches: first just fix the bug you found, but just use a "goto out" or
a "return" instead; then fix the lack of cleanup if you can.)

--b.

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