Milan

Milans's comments for usr/src/cmd/ldapcachemgr

 Last update: 2008-05-30
Batch 0

usr/src/cmd/ldapcachemgr/Makefile

{{{=================================}}}
[00]

 65      -LDLIBS +=       -lsldap -lldap -lnsl
      65 +LDLIBS +=       -lumem -lsldap

Just to be sure - is libumem default memory allocator for ldapcachemgr
now?

Yes.

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

{{{===================================}}}
[01]
       58 + * In ldap_cachemgr, it return -99 for some case, start with
-100 here

Where does it return what?

In some edge cases:
Line 849 in cachemgr.c
Line 1087 in cachemgr_getldap.c

[02] lines 69-73 and 88-98

- it would be better to use tabs before comments

Will fix it.

[03] 
      122 +extern int is_nscd(pid_t pid); /* in cachemgr.c */

- is_call_from_nscd(), to not confuse libsldap and ldap_cachemgr
implementations?

Will fix it.

usr/src/cmd/ldapcachemgr/cachemgr.c

{{{===================================}}}
[04]
      579 +         * kick off the thread which cleans ups waiting
threads for

- typo - cleans ups - too much "s"?

Will fix it.

[05]
      583 +        if (thr_create(NULL, NULL,
      584 +            (void *(*)(void*))chg_cleanup_waiting_threads, 0,
0, NULL) != 0) {

- the second parameter should be 0 (type is size_t)

- do you need to retype chg_cleanup_waiting_threads() ? I think it is
correct type already

- the 4th argument should be NULL (type is (void *))

Will fix it.

[06]
      585 +                logit("thr_create()
chg_cleanup_waiting_thread call failed\n");

      587 +                    gettext("ldap_cachemgr: thr_create() "
      588 +                    "chg_cleanup_waiting_threads call failed"));

- thr_create call failed in chg_cleanup_waiting_thread

Will fix it.

usr/src/cmd/ldapcachemgr/cachemgr_getldap.c

{{{===========================================}}}
[07] 
      165 +        int     up; /* TRUE: up, FALSE, down */

- if it is boolean, why not to use boolean_t ?

Will change comment.

[08]
      615 +        exitrc = NS_LDAP_SUCCESS;

- already initialized exitrc for NS_LDAP_SUCCESS, duplicite line

Will fix it

[09]
 880      -                int reset_bindtime)
      771 +                int reset_bindtime, info_op_t op)

- double tab?

Will fix it.

[10]
1123      -        int             matched = FALSE, len, rc = 0;
1124      -        char            *ret_addr = NULL, *ret_addrFQDN = NULL;
      893 +        int             matched = FALSE, len, rc = 0, main_nscd;
      894 +        char            *ret_addr = NULL, *ret_addrFQDN = NULL, *new_addr;

- don't increase mix of initialized and uninitialized variables, it decresase readability, split lines, please

main_nscd will be removed due to new code change.
new_addr will be initialized.

[11]
1237      -                                        continue;
     1004 +                                        break;

- is it possible to remove just one server at time?

[12] lines 1893-1965

- why not as static function, to avoid /* FALL THROUGH */?

info_mutex is a static mutex_t declared in getldap_serverInfo_op() so lock/unlock has to be done in getldap_serverInfo_op().
The purpose is to maintain the original program flow of removing server. The difference is newly added contact_server() is done outside this flow to be non-blocking. After it's done, it calls getldap_serverInfo_op(INFO_OP_REMOVESERVER) to return back the remaining flow.
But other tasks of INFO_OP_GETSERVER can't be disrupted either. So there is fall_thru to indicate it's not removing server.

[13]
2585      -        int             rc, rc1;
     2388 +        int             rc, rc1, changed = 0;

- again, don't mix initialized and uninitialized variables

[14]
     2695 +test_server_change(server_info_t *head) {
     2766 +create_buf_and_notify(char *input, ns_server_status_t st) {
     2828 +remove_server_thread(void *arg) {
     2851 +remove_server(char *addr) {
     2860 +set_server_status(char *input, server_info_t *head) {
     2805 +static int contact_server(char *addr) {

- bracket on own line, the last one has even type on the same line

Will fix it.

[15]
     2697 +        int     len = 0, num = 0, ds_len, new_len, tlen = 0;
     2698 +        char    *tmp_buf = NULL, *ptr, *status;

- again, don't mix initialized and uninitialized variables

Will fix it.

[16]
- at least small comment to every new function would be welcomed

Will fix it.

[17]
test_server_change() and create_buf_and_notify()

- where do you free tmp_buf?

tmp_buf becomes chg.chg_data in chg_notify_statusChange().
chg.chg_data is freed in waiting_list_cleanup()
when the waiting list is empty.

[18]

     2853 +        if (thr_create(NULL, NULL, (void *(*)(void*))remove_server_thread,

- the second parameter is size_t, not pointer

will fix it also remove type cast.

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

{{{===========================================}}}
[19]
       89 +        uint32_t        data_size;       /* if server change: size of data */
       90 +        char            data[sizeof (int)]; /* real size is data_size */

       95 +        uint32_t        data_size;       /* length of the config string */
       96 +        char            config_str[sizeof (int)]; /* real size is data_size */

- so which one is truth? uint32_t or int ? Both "doublelines" claim different things

sizeof (int) is not real size. It's just serves as a place holder for a string.

ldap_get_change_out_t and ldap_config_out_t are header structures
of a data stream.
The data stream starts at data and config_str which goes beyond the size of (int) and the real size is calculated and saved in data_size. 

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

{{{===========================================}}}
[20]
- OK, just minor comments to function would be welcomed

Will fix it.

usr/src/cmd/ldapcachemgr/cachemgr_change.c

{{{==========================================}}}
[21]

- all functions have definition with bracket on the same line

[22] REMOVED UPON MILAN'S REQUEST

[23]
  91 static int
  92 chg_cookie_equal(ldap_get_chg_cookie_t *c1, ldap_get_chg_cookie_t *c2) {

- this could be nice to have it as boolean_t

Will fix it.

[24]
 101 static  int

- 2 spaces between static and int?

Will fix it.

[25]
 109         info->chg_num++;
 110         if (info->chg_num > max_threads_allowed) {
 111                 info->chg_num~--;

- only question: are you preventing race condition by this ++, >, ~-- ?

This code is removed and handled differently. The thread counting is moved to switcher when GETSTATUSCHANGE gets called. The ~-- is just decrement when the thread is door_return(), even in error condition.

[26]
- only question: is waiting list thread specific or must be locked by callers (I see no locking there)?

All waiting_list_* functions are called in chg_get_statusChange() and locked by
 chg.chg_lock.

[27]
 260         int     rc = CHG_SUCCESS, len, return_now;

- again, don't mix initialized and uninitialized variables

Will fix it.

[28]
 492  * The door server thead which has the door client pid will be marked

- typo - thead -> thread

Will redo comment due to changes for Nicole's comments.

[29]
 510         if (thr_create(NULL, NULL,
 511             (void *(*)(void*))chg_cleanup_waiting_threads,

- the second parameter is size_t, 0

- do you need to retype chg_cleanup_waiting_threads()?

Will fix it.

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

[30]
     1647 +        /* need to get a new MT connection, return the old
one */
1695 + /* need to get a new MT connection, return the old one */

- as the sentence, big letter and missing dot:

It needs ... . 

And maybe:

"It needs to get a new MT connection, return the old one if available."

mc> Will change the comment.

[31]
     1693 +        /* need to use the referred server now */

"It needs to ... ."

mc> Will change the comment.

[32]

- how will CLEAR RESULTS work for non-MT connections?

2059 2086 if (cookie->conn != NULL && cookie->conn->ld != NULL && 
2060      -            cookie->connectionId != -1 && cookie->msgId != 0) {     2087 +            (cookie->connectionId != -1 ||
     2088 +            (cookie->conn_user != NULL &&
     2089 +            cookie->conn_user->conn_mt != NULL)) &&
     2090 +            cookie->msgId != 0) {

conn_user == NULL and we will not drop connection in _DropConnection() (old code path)

Now is msgId not used as indicator of active connection for all cases.

mc> please note that we have a (cookie->connectionId != -1) or
mc> (cookie->conn_user != NULL && cookie->conn_user->conn_mt != NULL) here.
mc> So for non-MT connection, cookie->connectionId != -1, clear_results()
mc> will do its work. For MT connection, (cookie->conn_user != NULL &&
mc> cookie->conn_user->conn_mt != NULL) will be true, so the cleanup
mc> will be done. msgId is still used as an indication that there's
mc> may be results yet to be received.

[33]
   2192 +                        /* Check if we've reached MAX retries */
- missing dot at the end of the sentence.

mc> will fix.

[34]
     2192 +                        /* Check if we've reached MAX retries
*/
     2194 +                        if (cookie->retries > 2) {

MAX == 2?

mc> MAX is 3. To make it clear and have a common one to use, I added 
mc> #define NS_LIST_TRY_MAX 3 in ns_connmgmt.h
mc> and will change "> 2" to ">  NS_LIST_TRY_MAX - 1" 

[35]
- DO_SEARCH, NEXT_RESULT, MULTI_RESULT cases as independent static
functions to avoid indentation issues? Also NEXT_RESULT and MULTI_RESULT
duplicates a lot of code now, it should be shared.

mc> Thanks for the suggestion, but will leave the code the same.

[36]
2778 + * MT connection not usable, 
     2779 +                                         * close it

- missing dot (and "is not usable")

mc> Will fix.

[37]
Batch API interface should be reviewed by somebody who knows it well. I have not enough experience with it yet.

[38]
     3306 + * the input DN (with no retry)
     3409 + * the input DN (with retry)

- missing dot

mc> Will change.

[39]
     3310 +find_domainname(
     3413 +__s_api_find_domainname(
     3434 +firstEntry(
     3612 +__ns_ldap_firstEntry(
3316 3641  __ns_ldap_nextEntry(

- cstyle - not tab, but 4 chars for the next line for parameters? Or do you plan to add comment to every parameter?

mc> Changed to indent four chars.

[40]
__ns_ldap_getAcctMgmt()

- should we keep connection and retry for pam account management? Couldn't this leave access to system for active user even after disabling the account?

mc> This shouldn't be a problem. The connection used for pam_ldap
mc> is never reused (i.e., conn_user of type NS_CONN_USER_AUTH).
mc> OTOH, __ns_ldap_getAcctMgmt() is called to retrieve accounting
mc> information of users, and even connection is reused, the
mc> retrieving will happen every time, so a user with a disabled
mc> account will be detected.

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

{{{=======================================}}}
[41]
      925 +                        /* need to use the referred server now */

- It needs ... . (Big char and dot, sentence)

mc> Comments changed.

[42]
     1527 +        /* Project ID */
     1528 +        /*
     1529 +         * ibuf is 11 chars big, which should be enough for string
     1530 +         * representation of 32bit number + nul-car
     1531 +         */

- two comments for same part of code, couldn't it be merged?

mc> Merged.

- missing dot at the end of the sentence

mc> Changed.

[43]
     1532 +        (void) snprintf(ibuf, sizeof (ibuf), "%u", ptr->pj_projid);

- are you sure that snprintf() cannot fail here?

mc> Changed to check rc if less than zero then return error.