Re: 2.6.2-rc2-mm2

[Tim Hockin <thockin@sun.com>]

On Fri, Jan 30, 2004 at 03:08:19PM -0800, Andrew Morton wrote:
> I think this is right - the NFSEXP_ALLSQUASH case appears to be clearing
> all groups.  When this settles down we need to run it all by Neil.
> 
> Do we need to handle the return value from set_current_groups(), or should
> that guy be simply returning void?

set_current_groups() can fail if security_task_setgroups() fails.

> +	struct group_info *group_info = NULL;

Why init to NULL?

> +	ngroups = 0;
> +	if (!(exp->ex_flags & NFSEXP_ALLSQUASH)) {
> +		for (i = 0; i < SVC_CRED_NGROUPS; i++) {
> +			if (cred->cr_groups[i])
> +				ngroups++;
> +		}
> +	}

I though of doing this, but passed in favor of simplicity of patch :)

The original made a specific point of doing
	gid_t group = cred->cr_groups[i];
	if (group == (gid_t) NOGROUP)
		break;

So the count loop should probably be
	ngroups = 0;
	if (!(exp->ex_flags & NFSEXP_ALLSQUASH)) {
		for (i = 0; i < SVC_CRED_NGROUPS; i++) {
			gid_t group = cred->cr_groups[i];
			if (group == (gid_t) NOGROUP)
				break;
			ngroups++;
		}
	}
So that we don't assume anything about NOGROUP.

> +	return ret;

The caller in fs/nfsd/nfsfh.c still needs to check the return value and do
something with it, or all this is just dumb.

-- 
Tim Hockin
Sun Microsystems, Linux Software Engineering
thockin@sun.com
All opinions are my own, not Sun's