[Duckwater-discuss] [sparks-discuss] Code-review request for Duckwater Phase 0

[Nicolas Williams]

On Thu, May 29, 2008 at 04:29:45PM -0500, Nicolas Williams wrote:
> > > http://cr.opensolaris.org/~the/duckwater_phase0/

Batch 3:

 - usr/src/cmd/ldap/*.c

   Please see to it that passwords are scrubbed before exiting (e.g.,
   with memset()).


 - usr/src/cmd/ldapcachemgr/cachemgr.h:58

   I don't understand the comment, please clarify.


 - cachmgr.h:50

   What is MAX_CHG_THREADS for?

       50 +#define MAX_CHG_THREADS 65535

   If we can have that many threads then why set a limit at all?

   In cachemgr.c we see:


 122  121  #define LDAP_TABLES             1       /* ldap */
 123  122  #define TABLE_THREADS           10
 124  123  #define COMMON_THREADS          20
 125  124  #define CACHE_MISS_THREADS      (COMMON_THREADS + LDAP_TABLES * TABLE_THREADS)
 126  125  #define CACHE_HIT_THREADS       20
      126 +#define MAX_SERVER_THREADS      (CACHE_HIT_THREADS + CACHE_MISS_THREADS + \
      127 +                                MAX_CHG_THREADS)

   Hmm, why are we defining *_THREADS in different places?

   I'll figure out what change threads are in a minute, but if it's what
   I think it is then I think 65535 is a bit too large.

   Also, if I understand correctly we could have practically every
   process blocked waiting for ldap_cachemgr to do something, is that
   correct?  If so, then I think this is a bit sub-optimal.  I'd much,
   much rather we find a way to avoid such waits.  I don't mind lots of
   processes blocking on nscd, for example, if they are calling getXbyYs
   and the name service backends are blocking.

   A *big* comment block at the top of the file would greatly help a
   reviewer understand what's going on.  As it is I don't know where to
   start; I'm tempted to bringover so I can build a cscope DB, which
   would slow down the review.


 - usr/src/cmd/ldapcachemgr/cachemgr_change.c:70

   cstyle: please put a blank line between local variable declarations
   and the first statement of a function.


 - cachemgr_change.c:chg_get_statusChange() and cachemgr.c:switcher()

   I'm rather concerned at the apparent lack of authorization checks for
   some things here.

   Item (1) in cachemgr_change.c:234-238, for example, doesn't consider
   the possibility that a NS_STATUS_CHANGE_OP_START request came from an
   attacker, leading to "cleanup ...".


More later.

Nico
--