[Duckwater-discuss] Code-review request for Duckwater Phase 0
Tomas Heran
Tomas.Heran at Sun.COM
Fri May 30 07:55:32 PDT 2008
Thanks!
Your comments and our replies and progress addressing your comments will
be tracked here:
http://opensolaris.org/os/project/duckwater/duckwater_phase0/codereview/milan_ldapcachemgr/
Tomas
Milan Jurik wrote:
> Hi,
>
> 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?
>
>
> 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?
>
> [02] lines 69-73 and 88-98
>
> - it would be better to use tabs before comments
>
> [03]
> 122 +extern int is_nscd(pid_t pid); /* in cachemgr.c */
>
> - is_call_from_nscd(), to not confuse libsldap and ldap_cachemgr
> implementations?
>
> usr/src/cmd/ldapcachemgr/cachemgr.c
> ===================================
> [04]
> 579 + * kick off the thread which cleans ups waiting
> threads for
>
> - typo - cleans ups - too much "s"?
>
> [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 *))
>
> [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
>
> 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 ?
>
> [08]
> 615 + exitrc = NS_LDAP_SUCCESS;
>
> - already initialized exitrc for NS_LDAP_SUCCESS, duplicite line
>
> [09]
> 880 - int reset_bindtime)
> 771 + int reset_bindtime, info_op_t op)
>
> - double tab?
>
> [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
>
> [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 */?
>
> [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
>
> [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
>
> [16]
> - at least small comment to every new function would be welcomed
>
> [17]
> test_server_change() and create_buf_and_notify()
>
> - where do you free tmp_buf?
>
> [18]
>
> 2853 + if (thr_create(NULL, NULL, (void *(*)(void*))remove_server_thread,
>
> - the second parameter is size_t, not pointer
>
> 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
>
> usr/src/lib/libsldap/common/ns_cache_door.c
> ===========================================
> [20]
> - OK, just minor comments to function would be welcomed
>
> usr/src/cmd/ldapcachemgr/cachemgr_change.c
> ==========================================
> [21]
>
> - all functions have definition with bracket on the same line
>
> [22]
> 77 static void
> 78 chg_config_cookie_increment_seq_num(void) {
> 79 (void) mutex_lock(&config_cookie_lock);
> 80 config_cookie.seq_num++;
> 81 (void) mutex_unlock(&config_cookie_lock);
> 82 }
>
> - do we need to lock for atomic incrementation?
>
> [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
>
> [24]
> 101 static int
>
> - 2 spaces between static and int?
>
> [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 ++, >, -- ?
>
> [26]
> - only question: is waiting list thread specific or must be locked by callers (I see no locking there)?
>
> [27]
> 260 int rc = CHG_SUCCESS, len, return_now;
>
> - again, don't mix initialized and uninitialized variables
>
> [28]
> 492 * The door server thead which has the door client pid will be marked
>
> - typo - thead -> thread
>
> [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()?
>
> Best regards,
>
> Milan
>
More information about the duckwater-discuss
mailing list