Serge

Serge's comments for usr/src/lib/libsldap/ - New Connection Management

 Last update: 2008-06-05
Batch 1

usr/src/lib/libsldap/common/ns_connmgmt.h

SD-00: l-88, 2 typos:
    - 'one' -> 'One'
    - 'an' ~--> 'a'

mc> Comments changed. This no longer applied.

SD-01: l-89/90; clarification needed ? 
    I guess this is saying that getent users are likely
    to share the same connection amongst different
    threads (as opposed to each threads for the getent users
    to handle different connections). 

    Also, is it saying that only the getent users can
    share connections between threads, or any type of
    users can (but only getent users are likely to ?)
    Could be more explicit IMO.

mc> Comment changed to clarify.

SD-02: l-147
    why int32_t and not uint_t as for cu_cnt ?

mc> Did you mean cu_max ? Because cu_max could have a value of -1
mc> (NS_CONN_MT_USER_MAX).

SD-03: l-169, typo ?
    'one the way' -> 'on the way' ?

mc> changed.

SD-04: l-227/246, also l-215
    I don't really get what's specific to nscd in there.
    Is that the persistent connection ? maybe that should
    be clarified.
    Also, if this is about persistent connection, is that
    specific to nscd or is it also applicable to connection
    that use NS_LDAP_KEEP_CONN flag ? 

mc> comments added to clarify.

usr/src/lib/libsldap/common/ns_connmgmt.c

SD-05: l-48/51
    shouldn't those be in ns_connmgmt.h ?

mc> These are not used by the structures defined in ns_connmgmt. So will
mc> stay here.

SD-06: l-105
    worth initiallizing tsd to NULL ? 

mc> Yes. changed.

SD-07: l-182, why not LOG_ERR as today ?

mc> Sure. Changed.

SD-08: l-189/190
    worth having a LOG_INFO/LOG_DEBUG that nothing
    was set ?

mc> OK. LOG_INFO added.

SD-09: l-225, same as SD-07

mc> Changed.

SD-10: l-224/229, should errno be set to 0 first ?

mc> man page says the error number is the return value, so the code is changed
mc> accordingly.

SD-11: l-258, typo
    coonection -> connection

mc> changed.

SD-12: l-287/290, also l-308/311
    so failing to setup the per-thread functions is not
    considered an error any more ? what's the rationale ?
    See SD-13 as well

mc> It was never a fatal error. In sparks code, a LOG_ERR syslog message
mc> is logged as we assumed that the libldap used would be our libsldap.so.5.
mc> But now we assume that libsldap could be using a libldap that does not
mc> support multi-threaded connection. In such case, libsldap should still be
mc> functional, but just works like pre-sparks ~-- no multi-threaded connection.

SD-13: l-330
    setup_mt_conn() never returns -1, only 0 or 1. Bug ?

mc> Yes. Fixed.

SD-14: l-317, same as SD-11 .. and same comment as well. 
    One of these 2 comments is wrong ?

mc> setup_mt_conn() is more for setting up the ldap session. Comment changed
mc> to clarify.

SD-15: l-1606 and following
   don't we need to take into account rc < 0 (MT setup failed)
   in all cases and set cmg->ldap_mt to FALSE ?

   We only do that if (cmg->do_mt_conn == B_FALSE), aren't 
   we missing some cases ?
   What if cmg->do_mt_conn = BTRUE and rc < 0 ?

mc> cmg->ldap_mt will only be set once. If cmg->do_mt_conn is TRUE
mc> and rc < 0, then the connection being opened will not be a
mc> shared MT one. This is treating rc < 0 as a temporary error if
mc> setup_mt_ld() succeeded at least once.

SD-16: l-467/513
   shouldn't these definition be somewhere else (ns_connmgmt.h ?
   top of file ?) instead of being in the middle of the file ?

mc> Moved to the beginning of the file. These are not used by the
mc> structures defined in ns_connmgmt.h so stay in this file.

SD-17: l-545/547
   shouldn't do_mt_conn be set as well ?
   will it be done later ?

mc> Yes. done later.

SD-18: l-550/560
   I guess this is related to SD-04 above. But shouldn't this
   apply to any process which is not using standalone API ? 
   shouldn't (cmg->config)->paramList[NS_LDAP_EXP_P].ns_tm = 0
   be the default ? 

mc> No. For non-nscd process, libsldap should try to refresh
mc> configuration when it times out. OTOH, nscd relies on
mc> ldap_cachemgr to know when the configuration changes.

SD-19: l-570
   *cmg (ns_connmgmt) can be NULL I think.

mc> Not likely. mark_shutdown_or_reoladed() wouldn't be called in situation
mc> when it's NULL.

SD-20: l-575, why incrementing ref_cnt here ?

mc> Just to ensure the conn_mgmt won't be destroyed when shutdown or
mc> config-reload is in progress.

SD-21: mark_shutdown_or_reoladed()
   op can be NS_CONN_MGMT_OP_NEW_CONFIG, is that well taken into account ?

mc> Yes.

SD-22: get_current_conn_mgmt()
   could we get comments on what exactly is the purpose of this function ?
   it seems to be doing more that what its name implies.

mc> Comment added.

SD-23: l-636/637 and 470
   What's the use of ns_connmgmt_shutting_down ? it's never set to B_TRUE
   if I am not mistaken.

mc> This is a bug. It should be set between line 642-643.

SD-24: l-659/663
   what happens to cmg_prev if it was set ? aren't we potentially
   leaking memory and fd/sockets ?

mc> cmg_prev is reference counted and when the last user releases it then
mc> refcount reaches zero, and it will be destroyed. Comment added.

SD-25: l-668
   why do we want cmg_prev here and not the current cmg ?

mc> Because that's the previous cmg that still needs to be dealt with by
mc> the caller who needs to release it to decrement the ref count.

SD-26: l-670; can this happen (cmg->config NULL) ? in which case(s) ?

mc> Yes. When init_conn_mgmt() is called during libsldap loading time
mc> before ldap_cachemgr is available to provide the configuration.

Batch 2

usr/src/lib/libsldap/common/ns_connmgmt.c

SD-27: free_conn_mgmt(), l-703
   shouldn't we exit here, or at least make sure not
   to free cmg on line 727 ?

mc> This is the function to destroy the connection management
mc> which has a refcount of zero, so free(cmg) at the end
mc> is always necessary.

SD-28: l-719
   please check return code and behave accordingly (at last
   syslog). We've seen door call failing in the past.

mc> There's not much we can do if the door call failed. Added
mc> a LOG_INFO syslog message for this.

SD-29: free_conn_mgmt()
   what about cm_head/cm_tail ? don't we need to release the conn_mt
   linked list or checked that it's been done ? 

mc> If the conn_mt linked list is not NULL, refcount
mc> won't be zero. free_conn_mgmt() won't be called.

SD-30: free_conn_mgmt()
   shouldn't we take the cmg->lock before playing with cmg and
   freeing it ? it looks to me that there's a potential race
   condition here. 
   What about adding a new arg to know whether or not we
   already have the lock, and release just before calling
   free(cmg) ?

mc> again, if free_conn_mgmt() is called, refcount
mc> should be zero. No two threads will call it at the
mc> same time.

SD-31: general about lock/mutexes. There are several locks
   used in this code. 
   What tools did you use or sanity check did you to to 
   make sure as much as possible that no deadlocks are
   introduced ?

mc> Through a lot of testing. Dead lock is very noticeable.
mc> Please suggest a tool to use.

SD-32: l-737/739
   see SD-30. I'd release the lock much later, and would take
   it if we haven't yet done so.

mc> don't really understand the comment. See comment about
mc> refcount above.

SD-33: l-739/740
   see SD-27; if there's a case where cmg is not null, we 
   would need to be aware of it here.

mc> Not sure what the comment is about. Please elaborate.

SD-34: l-739/743
   please set cmg to NULL explicitly if it's been released;
   I think there's no guarantee that free(cmg)/free_conn_mgmt()
   did it, so 743 might return garbage.

mc> Code added to set cmg to NULL and to log a LOG_WARNING
mc> syslog message.

SD-35: general about return codes
   Those of free_conn_mt()/release_conn_mgmt() are [never ?]
   checked. That looks bad to me.

mc> free_conn_mt() does not have return value ?
mc> Not sure why we need to check the returned conn_mgmt of
mc> release_conn_mgmt() ? It should never reach zero unless
mc> the conn_mgmt itself is explicitly released during unload or
mc> shutdown. Otherwise, once it is created, the refcount is
mc> 1 and always >= 1.

SD-36: __s_api_conn_mgmt_init()
    shouln't we return NULL if a call to thr_keycreate_once() 
    fails ?

mc> yes. Changed.

SD-37: 770-771
    shouldn't we do that only once we're sure we'll get a non
    null cmg ?

mc> ?? Please elaborate. libsldap could be loaded/unloaded/reloaded
mc> in a process more than once. So this is a safety measure.

SD-38: __s_api_conn_user_free()
    don't we need to check or free next (next conn_user in the
    linked list) ? if the caller shall take care of this pointer,
    can we get a comment about it ?

mc> comment added.

SD-39:  __s_api_conn_user_free()
    don't we need to check or free conn_mt ?

mc> Not really necessary.

SD-40: __s_api_conn_user_free()
    don't we need to check or free userinfo ?

mc> Caller needs to do that. Comment added.

SD-41: init_conn_mt(), 835/844
    what's the prupose of this code ? 
    can you add comments ?

mc> comment added.

SD-42: 850-853
    these line do not hurt but shall not be needed since calloc()
    was called.

mc> OK.

SD-43: free_conn_mt, 859
    how is the comment about lock related to unlock_cmg argument 
    of the function ?

mc> comment added.

SD-44: 870/871
    wouldn't it be more appropriate to ldap_unbind() in
    __s_api_freeConnection() ?

mc> I think it's fine here.

SD-45: free_conn_mt()
    what about the following fileds ? shouldn't we checked
    they've been released ?
	- lock (shall mutex_destroy be called ?)
	- next (similar as SD-38)
	- cu_head/cu_tail

mc> These are fine. lock is in the structure and not
mc> dynamically allocated, so I don't think calling
mc> mutex_destroy is necessary. next and cu_head/cu_tail
mc> will be taken care of before free_conn_mt() is called.

SD-46: 899-900
   where do we do garbage collection ? can you expand here ?

mc> comment added to explain. In match_conn_mt() about
mc> WRITE connection.

SD-47: 937/941
   add_cu2cm() adds at the end of the list; here, there's an 
   optimisation to delete at the begining of the list. 
   Which means that we're managing this list as a FIFO, not a
   LIFO. 
   Is that the expected behavior of the list or is that best
   guess (out of curiosity) ?

mc> Best guess.

SD-48: 950/956
   if pu == cm->cu_tail, this means that we haven't found cu.
   Should we log a info in this case ?

mc> LOG_INFO message added.

SD-49: 923
   we're testing on cm == null, but we should have taken cm->lock
   before entreing there right (hence cm not null ?)

mc> Yes. Check removed.

SD-50: 992/997, same as SD-48

mc>  LOG_INFO message added.

SD-51: is_server_cred_matched()
   assuming we know that all parameters are not null, shouldn't
   we check on cp->auth and cp->serverAddr ?

mc> cp->auth and cp->serverAddr won't be null but the input
mc> server and cred need to be checked. Code rearranged.

SD-52: wait_for_conn_mt()
   this routines does:
      - unlock cm->lock
      - unlock (cm->conn_mgmt)->lock
      - lock cm->lock

   which might be confusing. wouldn't it be better for the caller
   to do so, since all what this routines really need is to have
    cm->lock if I am not mistaken.

mc> It does need to do all three before calling conn_wait()
mc> which release conn_mt->lock. We don't want to hold
mc> cm->conn_mgmt->lock too long across the wait.

SD-53: check_and_close_conn() 
   what lock(s) are we suppose to onw when entering this function ?
   can the comment clarify ?

mc> comment added.

SD-54: check_and_close_conn()
   The code in there assumes that a conn_mt can not be shared
   for read and write operations. Could this be clarified
   in the header file, where ns_conn_mt is described ?
   e.g., can a conn_mt have NS_CONN_USER_SEARCH and NS_CONN_USER_GETENT ?
   or other combinations ? or is it specific to a user type ?

mc> comment added in the section for ns_conn_user_t.

SD-55: 1168
   why do we need the extra varaible ep here ? why not using errorp ?

mc> errorp is not passed in. This is for close_conn_mt() to
mc> copy the ns_ldap_error_t into the conn_mt structure for
mc> the affected conn_user to consume.

SD-56: 1184/1187
   I don't get the rationale here. Are we saying the the server that 
   we were given by ldap_cachemgr might be down now ? why is that ?

   Also, is __s_api_removeServer() about checking the status of a
   given server ? maybe the name shall be changed if that's the 
   case.

mc> These lines are not needed anymore. It does not work
mc> the way I thought it should, so removed.

Batch 3

usr/src/lib/libsldap/common/ns_connmgmt.c

SD-57: l195-1197
    could you state explicitly which locks are supposed to be taken
    when entering this function (match_conn_mt)?

mc> comment added.

SD-58: match_conn_mt and unlocking cm->lock
    I am not a fan of unlokcing mutex that were taken by the caller, this makes
    code harder to read IMO.
    In this case match_conn_mt() is called in only 1 place if I am not mistaken
    (in __s_api_conn_mt_get()) and it looks to me that it would be easier and
    safier for the caller to mutex_unlock() when  match_conn_mt() is B_FALSE.

mc> The reason for the unlock is because this function
mc> call free_conn_mt() to free the conn_mt. It could
mc> not be done if the mutex is not unlocked.

SD-59: 1218-1231
    I don't get the logic here:
    - why closing a connn_mt that was for write and that's been idle long
      enough ?
    - what is the test on cu->type != NS_CONN_USER_WRITE for ? according to
      the comment, shall we not just close the conn_mt regardless of what cu 
      type is ?
    - 'long enough' seems to be no user (cu_cnt == 0) and no access for 60s,
      should 60 be configurable ? part of the profile ?

mc> 1. Comment added to explain.
mc> 2. If a conn_user of WRITE is requesting a WRITE connection,
mc>    then the connection should be used (instead of opening
mc>    a new one). On the other hand, if a conn_user of other
mc>    see this WRITE connection and it's idle too long, then
mc>    it should be closed.
mc> 3. 60 is a best guess. I think the current rule is not to add
mc>    any new config parameters to the DUAprofile. Doug's position.

SD-60: 1225
    NS_LDAP_INTERNAL does not seem very appropriate. Could we add a new return
    code ? besides, NS_LDAP_INTERNAL requires details in errorp, which is NULL
    here

mc> It really doesn't matter here. The rc will not be consumed
mc> by any conn_user (since cm->cu_cnt == 0). Comment added
mc> to say it's irrelevant.

SD-61: 1234-1250
   so this means that NS_CONN_USER_AUTH and NS_CONN_USER_TEST can not share
   conn_mt. Worth clarifyning were conn_mt is defined ? see SD-54 as well.

mc> Comment added in ns_connmgmt.h. In the section for
mc> ns_conn_user.

SD-62: 1261
   why don't we check_and_close_conn() for nscd ? 

mc> nscd relies on ldap_cachemgr to provide the server status
mc> change via the GETSTATUSCHANGE door call. See the change
mc> monitor function, get_server_change(). Comments added.

SD-63: 1276 & 1273
   shouldn't the test be (safer) >= instead of == ? 

mc> Yes. Changed.

SD-64: 1256-1278
   should'nt we test also NS_CONN_MT_UNINITED, NS_CONN_MT_CONNECT_ERROR and 
   NS_CONN_MT_CLOSING ? 

mc> It won't get this far in the function if the cm->state
mc> is not NS_CONN_MT_CONNECTED and NS_CONN_MT_CONNECTING.
mc> 1211         if (cm->state != st
mc> makes sure that is the case (where st can only be one
mc> of those two.

SD-65: __s_api_conn_mt_get() (and frineds: _s_api_conn_mt_add(), ...)
   that's an 'API' function, could we get more wording on what the parameters
   are (shall be) and what return code shall be expected ? 

mc> done.

SD-66: 1317-1318
   I find the comment and the following test hard to read. 
   What about something like: we want to reuse MT connection only if 
   keep-connection flag is set or if MT was requested (or active)

mc> Changed as suggested.

SD-67: 1326, typo
   coonection -> connection

mc> Fixed.

SD-68: 1324
   worth adding a comment that we're taking the lock of cmg ? (or changing 
   macro name to make it explicit ?)

mc> Name changed to NS_CONN_CHECK_ABORT_AND_LOCK.

SD-69: 1370-1374
    I don't understadn the logic, can you add a comment ?
    Why NS_CONN_USER_CONNECT_ERROR in 1 case and NS_CONN_USER_FINDING else ?

mc> Comment added to clarify.

SD-70: 1384/1385
    add same comment as 1376

mc> done.

SD-71: 1397-1400
   why do we 'hide' NS_LDAP_MEMORY ?

mc> Yes. No need to hide. Code removed.

SD-72: 1403 and following
   shouldn't we add_cu2cn() ?

mc> It will be added when the cn state reaches CONNECTED
mc> in __s_api_conn_mt_add().

SD-73: 1413
   why NOTFOUND ? why not SUCCESS ?

mc> Because the connection is not established. NOTFOUND tells
mc> the caller to go through the connection establishment
mc> code path.

SD-74: 1438-1442
   why do we do this here ? shouldn't this have been done elsewhere ? 

mc> Because this is the time the conn_mt is ready to serve conn_users.

SD-75: 1483 & 490
   what's the purpose of ns_conn_free ? it's only used here if I am not
   mistaken

mc> It's used to make line 494 if (free == 1) in macro
mc> NS_CONN_UNLOCK_AND_FREE not becoming if (1 == 1)
mc> that lint does not like.

SD-76: err2cm()
   err2cm() assumes its caller will take care of releasing errorp, since
   it copies it into ep (assuming __s_api_copy_error() does what its name
   implies). 

   Have you checked for memory leaks ? it seems to me that at least 1 code
   path would produce some:
	shutdown_all_conn_mt() allocates ep, gives it to:
	    close_conn_mt() as errorp which then might call err2cm().
   If I am not mistaken, neither close_conn_mt() nor shutdown_all_conn_mt()
   release the initial ep. 

mc> It is freed at line 2097 (void) __ns_ldap_freeError(&ep);
mc> in shutdown_all_conn_mt.

Batch 4

usr/src/lib/libsldap/common/ns_connmgmt.c

SD-77: err_from_caller() & err_from_cm()
   Both function are called the same place to set cu->ns_error, but behave
   differently: err_from_cm() copies (duplicates) a ns_ldap_error_t whereas
   err_from_caller() just plays with pointer.
   Doesn't that make of difference when it comes to release associated memory ?

mc> Yes. err_from_caller() moves the ns_ldap_error_t into the ns_conn_user_t
mc> and set *errorp to NULL, so the caller does not need to free it. I have
mc> added comment to __s_api_conn_mt_close() to make it clear.

SD-78: 1571
   can't cm->ns_error be set already ? shoudln't we check and release if it is ?
mc> It won't. Because of line 1566 if (cm->state != NS_CONN_MT_CONNECT_ERROR) {

SD-78: 1704 and 1719-1722
   why don't we have the code in both places (check on cu_cnt and set free_cm) ?
mc> Actually line 1704 del_cm4cmg(cm, cmg);
mc> is not necessaey. Removed.

SD-79: close_conn_mt()
   can't cmg be NULL ? if yes, needs to be taken into account in 1651/1652

mc> cmg can't be NULL.

SD-80: related to SD-79
   to conform to comment 1654, don't we just need to check that cmg is not
   NULL ?

mc> Sorry. I don't understand the question.

SD-81: 1664-1666 & 1678-1679
   can't we combine these 2 ?
   also, I don't get what 1664-1666 is really about

mc> Yes. it can be combined. Comment added to explain line 1664-1666.

SD-82: 1669-1675
   are we safe here changing conn_user states ? can't another thread be using
   one of these conn_user ? i.e., don't we need a lock on conn_user for here ?

mc> lock is not really necessary, but overloading the state is not
mc> that safe. So I have changed ns_conn_user_t to have a bad_mt_conn flag
mc> that will be set once and stayed set until the ns_conn_user_t is
mc> deleted. It safer this way.

SD-82: 1848-1855
   I don't get the logic. Why 2 calls to release_conn_mgmt(ocmg, B_FALSE) ?

mc> when ocmg is created, the refcount is 1. When ocmg is referenced by
mc> NS_CONN_MGMT_OP_RELOAD_CONFIG, refcount is one more to prevent the
mc> ocmg from being deleted by someone release the orignal refcount, so
mc> we need two release_conn_mgmt calls.

SD-83: 1872, while loop
   potential for infinite loop ?
   what if close_conn_mt_by_procchg() does not close cm ? this could happen if
   for some reasons close_conn_mt() failed (returned -1), right ?

mc> In normal case, close_conn_mt() should change the state of the cm,
mc> so loop is not possible. In error case, if close_conn_mt() returned
mc> -1, then either the cm state is not valid, or no cm can be found.
mc> both will cause the loop to exit. So no infinite loop.

SD-84: 1895-1899
   another case requiring to start from the head is when we have severals
   conn_mt to the same serverAddr. Is that correct ?

mc> Yes. Comment changed to indicate that.

SD-85: 1978
   does the XXX means you wanted to re-review this ? I like the 'play safe'
  option ;)

mc> Yes. Now XXX is removed.

SD-86: similar as SD-83 for the 'server up' loop

mc> Same answer. Loop not possible. close_conn_mt_when_nouser()
mc> either set close_when_nouser flag to B_TRUE or remove cm
mc> from cmg's cm linked list, so loop is not possible.

SD-87: __s_api_enable_MT_conn()
   is never called/used if I am not mistaken.
   Which also questions the need/use of cmg->mt_requested.

mc> This is for testing purpose. I can remove it or leave it in
mc> and expose it in the future.
mc> OK. Decided to remove. 
mc> Deleted member mt_requested in ns_conn_mgmt_t, deleted
mc> enum for NS_CONN_USER_TEST, removed function
mc> __s_api_enable_MT_conn, and changed code accordingly.

SD-88: shutdown_all_conn_mt() 2241 in new webrev by Michen, typo
   cgange -> change

mc> fixed.

SD-88: __s_api_shutdown_conn_mgmt()
   same as SD-82. I must be missing something ... ;)

mc> same answer.

SD-89: __s_api_setup_retry_search(
   add comment about what is success and what is failure

mc> comment added.

SD-90: ns_conn_waiter_t, conn_wait, conn_signal
   what is the key field of ns_conn_waiter_t used for ?

mc> Mainly a nice debug info to know which
mc> conn_user is waiting for an CONN_MT.

SD-91: conn_wait, conn_signal
   couple of possible concerns:
   - conn_mt->waiter is never used if I am not mistaken (i.e., the initial
     head element). Or (worse ?) is it never used for cond_wait() but used
     for cond_signal() ?

mc> It is used as the anchor of the linked list.
mc> It is used in both cond_wait (line 2237) and cond_signal (2263/2264).

   - conn_wait keeps on adding waiter to conn_mt->waiter, shouldn't conn_signal
     remove them once signal is sent ?
mc> conn_signal wakes up the waiters and waiters remove themself from the
mc> linked list (line 2251 - 2254, in conn_wait)

SD-92: 2303
   what's the point of initializing cmg (it's going to be done on 2309) ?

mc> Yes. no point. Removed.

SD-93: get_server_change()
   don't we need to take cmg->lock before palying with it ?
   or add a comment that we have it ?

mc> Code added to __s_api_conn_mt_add() to ensure get_server_change()
mc> will not be called twice. Before the call, cmg->lock is hold.
mc> comment added.

SD-94: 2326
   what's the purpose of munmap ?

mc> Just in case the door call returns a different buffer that needs to
mc> be freed.

SD-95: 2384-2389
   can't there be any other failure door_rc code than these 2 ? yes, it seems,
   although we're using NS_CACHE_NOTFOUND here and -1 in cachemgr. Can we make
   this more consistent ?

mc> If not those two rc, the door call will be retried. The -1 changes
mc> would involve quite a bit of old code in ldap_cachemgr, we will change
mc> the code we add this time.

SD-96: get_server_change()
   - can we have more syslog in case of errors (e.g. calloc() fails and we
     continue)

mc> done

   - can we have more comments in the for () loop when testinmg is server
     status changed ? it's hard to read IMO, and to understand exactly what
     it's doing (e.g., what are the magic values for 'which' ?)

mc> done

   - this routine is only relevant for FN, right ?

mc> only for the main nscd

   - does this routines keep on sending door call to ldap_cachemgr() ?
     isn't there any pause/sleep type of thing anywhere ?

mc> sleep is there. Added retry a number of times.

usr/src/lib/libsldap/Makefile.com

SD-97: why do we depend on libresolv ?

Tomas: Not needed. Removed.

SD-98: what is C99MODE about ?
   to which CR fix does it apply ?

Tomas: Not needed. Removed.

usr/src/lib/libsldap/common/mapfile-vers

SD-99: wouldn't it be appropriate to have a SUNWprivate_1.2 version ?
   introducing standalone bits is somewhat a major change.

mc> The answer is no. Doug pointed us to usr/src/lib/README.mapfiles.

    4.2 Adding a Private interface

    Private interfaces are the non-ABI interfaces of the library.  Unlike
    introducing a Public interface, a new entry is simply added to the
    SUNWprivate version.  No minor number increment is necessary.

usr/src/lib/libsldap/common/ns_cache_door.c

SD-100: 198
   please check return code. No need for calling __ns_ldap_trydoorcall_send()
   if we couldn't get door fd.

mc> Yes indeed. Will change to call __ns_ldap_trydoorcall_send only
mc> if rc is good.

usr/src/lib/libsldap/common/ns_cache_door.h
~---> no comment

usr/src/lib/libsldap/common/ns_common.c

SD-101: 2233-2254
   can we have comments on what exactly is done here in standalone mode ?
   is __s_api_findRootDSE() really appropriate or is it an overkill for
   removing the server ?

mc> Comment added.

SD-102: 2239-2244
   "no info" does not mention that we failed, possibly error->message
   is not more helpful. What about on line 2240 changing to
   "failed removing %s - %s" ?

mc> Message modified.

SD-103: __s_api_make_error()
   so now we have __s_api_make_error() and MKERROR() doing very similar
   things. No way of combining the 2 ? most calls to MKERROR would
   strdup() the message, so why not having MKERROR use __s_api_make_error() ?

mc> __s_api_make_error() and MKERROR() have slightly different semantic.
mc> __s_api_make_error() always strdup() the input error message.
mc> MKERROR() does not. It relies on the caller to allocate the memory
mc> for the error message. At this time, there are too many references to
mc> MKERROR() in the old code. The impact is too big to change
mc> MKERROR() to use __s_api_make_error(). So will leave them
mc> as is.

usr/src/lib/libsldap/common/ns_config.c

SD-104: to follow-up Nico's comment about #pragma init, is that the
   way to go moving forward (more things in pragma fini)

mc> We have to look into this one after the putback. There is more
mc> involved.

SD-105: 880-883 & 910
   comment states it sets the config ofr per connection mgmt, but it
   may set the global config. Is that expected ? if yes, comment shall
   reflect this.

mc> Comment will be added to state that.

Batch 5

usr/src/lib/libsldap/common/ns_confmgr.c

SD-106: what's the point of making it an inline function ? 

Igor>
> That function was initially a macro, but Milan asked to make it 
> function. Since
> it was small and simple, to make it to behave as a macro, I put 
> 'inline' there.
> Remove it, if you do not like it.

SD-107: comments please of what's the purpose of set_attr() (as opposed
   to __ns_ldap_setParamValue()

Igor>
This function is a part of code used by
__s_api_create_config_door_str() and create_config_nsconf()
(for details, please see the OpenSolaris version of ns_confmgr.c).
Its main purpose is to process a pair of an attribute's name and value,
get a valid index (see my next comment), and call
__ns_ldap_setParamValue()

SD-108: 262-263
   __s_api_get_profiletype() checks defconfig[i].profile_name and 
   __s_api_get_versiontype() check defconfig[i].name. 
   So, we're saying here that attr_name can be either of these 2 fields,
   e.g., either "NS_LDAP_SERVER_PREF" or "preferredServerList". Is that
   correct ?

Igor>
Yes, as I wrote in my previous email(SD-110), an LDAP configuration
can be obtained either from a server or from SMF.
The former sends a DUA with attributes' names styled like
"preferredServerList". But local configurations
will have names inherited from the /var/ldap/ldap* files.
So, the standalone bits are able to process
both sets of attributes' names.

SD-109: 256-257
   it doesn't hurt but:
   - why not checking config_struct as well ?
   - attrname and attr_value are already checked by caller if I am not mistaken

cls> All 3 parameters(config_struct, attrname and attr_value) are
cls> checked by the caller so I'll remove all the check here.

SD-110: __s_api_create_config_door_str()
   what is _door_str() in __s_api_create_config_door_str() suppose to
   suggest ?

Igor>
> Initially, it was designed that the standalone bits would
> get input information in two ways: as a set of attributes
> describing an LDAP server (the NS_LDAP_SERVER request type
> - see ns_sldap.h::ns_standalone_request_type_t)
> or as a name of a local back-end configuration (NS_BEC).
>
> In the first case, the __ns_ldap_getConnectionInfo()
> function obtains the corresponding DUAProfile and returns
> it in the ldap_cachemgr door call format.
> Then, __s_api_create_config_door_str() converts the
> profile into an ns_config_t struct.
> That is what _door_str means: create from a string in the 
> ldap_cachemgr door call format.

SD-111: __s_api_create_config_door_str()
   check arguments (290-292, 301-304) before allocating configStruct
   (and avoid destryoing it then)

cls> Will fix it.

SD-112: 311
   remove attr from snprintf

cls> Will fix it.

SD-113: 333-340
   this shall be done before 320

cls> Will fix it.

SD-114: 349
   __s_api_crosscheck() requires lock for conifgStr to be taken, but I
   guess this is OK since we've just created it and didn't give it
   to anyone ?

Igor>
Right, when being checked, the configStruct is just an automatic var.
within __s_api_create_config_door_str().
It's not yet exposed to outside.

SD-115: 478
   comment line started by a comma ?

cls> Will fix it.

SD-116: 520 asnd following
   what are we doing here ? please add comment

cls> Will fix it.

usr/src/lib/libsldap/common/ns_connect.c

SD-117: 175
   test request not null (as on 221-222)

mc> lines 221-222 moved to before line 175 and use 'ireq' instead of
mc> 'request'.

SD-118: 194 and ns_standalone.c
   looking at what __s_api_ip2hostname() does, it seems to be that
   it's not consistent in the way it handles hostname (ret->serverFQDN
   here): it ipAddr is indeed an IP address, __s_api_ip2hostname() will
   allocate memory for hostname; whereas it won't if ipAddr is already
   a host name. Looks like there's a bug somewhere.

mc> I think it's fine. There's no bug here.
mc> __s_api_ip2hostname() does allocate if ipAddr is already a
mc> hostname. see lines:
mc> 535         if ((addr = strdup(ipaddr)) == NULL)
mc> 636         *hostname = addr;
mc> In the non-hostname case,
mc> 535         if ((addr = strdup(ipaddr)) == NULL)
mc> 612         if ((*hostname = malloc(len)) == NULL) {
mc> 623         free(addr);
mc> So either way, memory is allocated for what's returned in *hostname.

SD-119: printCred, printConnection
   outputs now go to fp, which is stderr. What is stderr for nscd ?
   logfile as in nscd.conf(4) ?

mc> If nscd is running with debug enabled and in the foreground,
mc> the output goes to stderr. Otherwise nscd closes all fds before
mc> daemonlized except the logfile fd, which won't be stderr. So the
mc> output won't be seen. For ldaplist or other applications, the
mc> output will go to stderr.

SD-120: findConnection()
   why do we remove threadID from the DEBUG messages ? isn't this useful ?
   we still have it for printCred/printConnection, I'd suggest to keep it
   in findConnection() as well.

mc> threadID is added back.

SD-121: 580-597
   duplicate code with is_server_cred_matched() from ns_connmgmt.c.
   Please reuse one or the other (move it to ns_common.c ?)

mc> New function __s_api_is_auth_matched() created,
mc code in ns_connect.c and ns_connmgmt.c changed accordingly.

SD-121: 579 and usedBit in general
   do we still need usedBit, as defined in ns_internal.h (true if only used by
   one thread and not shared ?)
   Shared connection is not managed at this level anymore, right ? 

mc> Still needed. A non-MT persistent connection can still be shared, just one
mc> thread at a time, not by multiple threads.

Batch 6

usr/src/lib/libsldap/common/ns_connect.c

SD-122: 689
  should it be __s_api_isStandalone() as in other parts of the code ?

mc> NO. It should be __s_api_isInitializing(), which returns 1 if the
mc> standalone mode is in the initialization stage.

SD-123: 694
  check strdup() returning null

mc> Fixed.

SD-124: 961-962
  add a comment to say that we're going in only if MT connection layer
  is not used.

mc> Added.

SD-125: _DropConnection()
  same as a previous comment: why getting rid of threadID in certain
  DEBUG messages ?

mc> ThreadID added.

SD-126: 1022-1023
  why do we get rid of the cehcks on nscd here, and keep them in
  findConnection() ?

mc> The nscd checks is added back.

SD-127: 1031
  do we still use shared ? shouldn't it remove from the structure ?
  if not, where do we set it ?

mc> 'shared' removed.

SD-128: 1345-1346
  what are we doing here ? should this be done before ?

mc> Undo what line 1335 *s = '\0'; did. Put ':' back in the serverAddr
mc> string.

SD-129: createTLSSession()
  syslog message uses the prefix 'openConnection' instead of createTLSSession.

mc> Changed openConnection to createTLSSession.

SD-130: 2041
  anough -> enough

mc> Fixed.

SD-131: 2050
  already done in 2033

mc> Need both. One is to set the buffer to an empty string.
mc> One is to clear the data from __s_api_hostname2ip().