Sorry for the delayed response--I read over these offline and them
forgot about them....:
On Thu, Mar 13, 2008 at 01:48:07PM -0400, Trond Myklebust wrote:
> The hash bucket is for some reason always being set to zero.
The changelog seems out of sync with the code; nr is normally set a
little lower to the low bits of acred->uid:
...
> @@ -280,7 +281,9 @@ rpcauth_lookup_credcache(struct rpc_auth *auth, struct auth_cred * acred,
> struct hlist_node *pos;
> struct rpc_cred *cred = NULL,
> *entry, *new;
> - int nr = 0;
> + unsigned int nr;
> +
> + nr = hash_long(acred->uid, RPC_CREDCACHE_HASHBITS);
>
> if (!(flags & RPCAUTH_LOOKUP_ROOTCREDS))
> nr = acred->uid & RPC_CREDCACHE_MASK;
So what you're really doing is switching to a better hash fn. (Except
that the result is being overwritten immediately afterwards in the
!RPCAUTH_LOOKUP_ROOTRCREDS case.)
Wouldn't it have made more sense to replace the existing assignment,
then remove just make it unconditional in next patch?
--b.
On Fri, 2008-03-28 at 11:12 -0400, J. Bruce Fields wrote:
> Sorry for the delayed response--I read over these offline and them
> forgot about them....:
>
> On Thu, Mar 13, 2008 at 01:48:07PM -0400, Trond Myklebust wrote:
> > The hash bucket is for some reason always being set to zero.
>
> The changelog seems out of sync with the code; nr is normally set a
> little lower to the low bits of acred->uid:
>
> ...
> > @@ -280,7 +281,9 @@ rpcauth_lookup_credcache(struct rpc_auth *auth, struct auth_cred * acred,
> > struct hlist_node *pos;
> > struct rpc_cred *cred = NULL,
> > *entry, *new;
> > - int nr = 0;
> > + unsigned int nr;
> > +
> > + nr = hash_long(acred->uid, RPC_CREDCACHE_HASHBITS);
> >
> > if (!(flags & RPCAUTH_LOOKUP_ROOTCREDS))
> > nr = acred->uid & RPC_CREDCACHE_MASK;
>
> So what you're really doing is switching to a better hash fn. (Except
> that the result is being overwritten immediately afterwards in the
> !RPCAUTH_LOOKUP_ROOTRCREDS case.)
>
> Wouldn't it have made more sense to replace the existing assignment,
> then remove just make it unconditional in next patch?
Originally I developed the second patch first, then added this one.
After a spot of re-ordering, I got confused.
The end result is correct, though, and git bisection will not be broken,
so I figure we might as well keep it rather than rebuilding the git
trees...
--
Trond Myklebust
Linux NFS client maintainer
NetApp
[email protected]
http://www.netapp.com