[Duckwater-discuss] Code-review request for Duckwater Phase 0
Milan Jurik
Milan.Jurik at Sun.COM
Fri May 30 09:05:51 PDT 2008
Tomas,
OK, and [22] you can remove, it's correct. Friday in hot weather, not
good time for code review :-)
Best regards,
Milan
V pá, 30. 05. 2008 v 16:55, Tomas Heran píše:
> 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
> >
> _______________________________________________
> duckwater-discuss mailing list
> duckwater-discuss at opensolaris.org
> http://mail.opensolaris.org/mailman/listinfo/duckwater-discuss
More information about the duckwater-discuss
mailing list