On Wed, Jun 21, 2006 at 02:49:23PM -0700, Jonathan Stone wrote:
> 
> In message <20060621205606.GC8444@NetBSD.org>Brian Ginsbach writes
> >It should be as simple as changing the above call to
> >_passwdcompat_pwscan().
> >
> >                                rv = _passwdcompat_pwscan(&cpw,
> >                                    cbuf, sizeof(cbuf),
> >                                    search, name, uid);
> 
> Nope (again, no value-judgment implied). See the comment one line
> above your fragment which says
> 
> 	/* get next user */ 

Alright, then the comment is wrong for two of the possible three
cases for compat.  It will get the next user if _compat_pwscan()
was called via getpwent/getpwent_r.  Otherwise it should look up
by key -- the other two, i.e. getpwuid/getpwuid_r and getpwnam/getpwnam_r.

I'm going to change the comment to:

	/* get next user or lookup by key */

> 
> 
> For a request where we have the key, we should *not* "get the next user":
> we should do a lookup by key, rather than   iterate over all
> users in the compat map.

Agreed.  I'll give Luke credit that he had everything set up correctly --
recall it did work, just slowly -- but forgot that the compat backends
maybe able to do a direct key lookup.  The compat "database" is really
a superset of the "files" database.

> 
> Your suggested fix probably does cure the immediate problem; but (on
> several aspects) it leaves the code in a state well below our par for
> code quality. One aspect is that that the comments don't match
> reality. Another is that we're relying on an (undocumented/uncommented) implementation
> artifact of the internal helper functions.  Yet another is that the (single) loop
> in _compat_pwscan() is used to both loop the local file with +/= entries;
> and also (mis)used to loop over the entries in
> the compat source, for non-key lookups.

I disagree about the quality.  Again, I'll give Luke credit.  I
will agree that the addition of the state machinery did muck up
the works.  I maintain that _compat_pwscan() is not misused to loop
over the entries for non-key lookups.  Why duplicate logic that
would need to be the same for both the iterator (getpwent) and key
lookups (getpwuid or getpwnam).  The iterator needs to apply the
same +/- (and by extension @, netgroup) overrides as the key lookups.

> 
> I submit that those flaws have contributed to the bug under discussion.
> 

I submit that Luke may have been a bit over zealous in smashing 
functions together.  The problem is that he used _compat_getpwent()
as the template for _compat_pwscan() rather than _compat_getpw().

> I also submit that (given this bug!) the control-flow inside
> _compat_pwscan(), and the coupling between it and
> _passwdcompat_pwscan(), is too hairy (or too hairy for the available
> time) of the average NetBSD developer.  Therefore the flaws should be
> remedied, if possible. I had thought that went without saying, but
> (given off-list comments), apparently it doesn't.  Brian, that's not
> in any way aimed at you, and I'm sorry to be so long-winded, but
> apparently that all *does* need to be said.

I'm not taking it that way.  The control flow isn't the easiest to
follow but I'm not convinced that the cure will be any better than
the disease...

> 
> I also agree with Christos' subsequent observation that a deeper
> rework is riskier than your approach.  So perhaps your suggested fix
> is actually the best candidate for a pullup to NetBSD-3.

I agree.  Fix the problem at hand before creating new problems. :-)

> 
> Brian: if you can verify that your fix works, please commit it,
> co-ordinate pullups, and let me know when it's safe to replace with a
> reworked version.

I've finally verified that my fix works as a stand-alone.  I'm in
process of doing a full build.  When that is complete I'll commit.

Brian