[Duckwater-discuss] Code-review request for Duckwater Phase 0 (round 3)

[Serge Dussud]

Thanks Michen,

no sure how to follow-up, should I edit
http://opensolaris.org/os/project/duckwater/duckwater_phase0/codereview/serge_libsldap/ 
  ? (how do I do that ?)

Anyway, here are my main follow-up comments for now:

- IIUC, is_nscd is used in several places to identify the main nscd 
(that's what your answers to SD-04 and SD-62 suggest). However, is_nscd 
is B_TRUE as well for per-user NSCD (see init_conn_mgmt()). Am I not 
understanding correctly or is there something wrong ?

- that'd be great to have at some point a write-up/TOI/arch document 
about how configuration changes are handled in different scenarios (MN, 
PUN or non-MN processes, standalone). Will (or has) the NL2 arch doc be 
updated accordingly ? there are also a number of new APIs for connection 
enhancements, will (or do) they appear in the NL2 arch doc ?

- about checking mutexes problem: our main nevada page (tools section) 
refers to warlock. People of these list may have other preferred tools.

Serge




Michen Chang wrote:
> 
> Hi Serge,
> 
> Thanks for the review. I have integrated your comments (batch 1/2/3).
> Please see
> http://opensolaris.org/os/project/duckwater/duckwater_phase0/codereview/serge_libsldap/ 
> 
> 
> Tomas, My webrev is here:
> http://jurassic.eng/net/ns-web.sfbay/tank/panjim-export/ldap/users/michen/ws/sldap-comment-0602/webrev 
> 
> 
> This also address Nico's concern about too many peruser nscds using
> the ldap_cachemgr status change feature. One line change to allow
> only the main nscd to use it.
> 
> -- 
> Michen
> 
> Serge Dussud wrote:
>>
>>
>> round 3 of comments attached.
>>
>> Serge
>> ------------------------------------------------------------------------
>>
>>
>> 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)?
>>
>> 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.
>>
>> 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 ?
>>
>> 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
>>
>> 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.
>>
>> SD-62: 1261
>>    why don't we check_and_close_conn() for nscd ?
>> SD-63: 1276 & 1273
>>    shouldn't the test be (safer) >= instead of == ?
>> SD-64: 1256-1278
>>    should'nt we test also NS_CONN_MT_UNINITED, 
>> NS_CONN_MT_CONNECT_ERROR and    NS_CONN_MT_CLOSING ?
>> 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 ?
>> 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)
>>
>> SD-67: 1326, typo
>>    coonection -> connection
>>
>> SD-68: 1324
>>    worth adding a comment that we're taking the lock of cmg ? (or 
>> changing    macro name to make it explicit ?)
>>
>> 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 ?
>>    SD-70: 1384/1385
>>     add same comment as 1376
>>
>> SD-71: 1397-1400
>>    why do we 'hide' NS_LDAP_MEMORY ?
>>
>> SD-72: 1403 and following
>>    shouldn't we add_cu2cn() ?
>>
>> SD-73: 1413
>>    why NOTFOUND ? why not SUCCESS ?
>>
>> SD-74: 1438-1442
>>    why do we do this here ? shouldn't this have been done elsewhere ?
>> SD-75: 1483 & 490
>>    what's the purpose of ns_conn_free ? it's only used here if I am not
>>    mistaken
>>
>> 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.
>>   
>> ------------------------------------------------------------------------
>>
>> _______________________________________________
>> duckwater-discuss mailing list
>> duckwater-discuss at opensolaris.org
>> http://mail.opensolaris.org/mailman/listinfo/duckwater-discuss
>>   
>