Subject: Re: NetBSD-3 NIS-compat getpwnam()/getpwuid iterate entire map [was Re: 3.0 YP lookup latency]
To: Jonathan Stone <jonathan@Pescadero.dsg.stanford.edu>
From: Brian Ginsbach <ginsbach@NetBSD.org>
Date: 06/23/2006 17:05:30
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
> > 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
> 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.