Jan Damborsky wrote: > Hi Sanjay, > > thank you very much for your comments. > > Please see my response in line. > > Jan > > > > Sanjay Nadkarni wrote: > >> Please make sure that code passes hg nits. >> > > Done. > > >> A couple of observations. This is not your code change but more along >> the line of clean up. >> >> Is there a real need to remap the levels for various modules ? i.e. >> Does the define IDM_DBGLVL_ERROR which maps to LS_DBGLVL_ERR provide >> any extra value ? Since what we can now is a common logging service, >> it would make sense that all modules use the same define instead of >> remapping. Similarly imm_debug_print which maps to ls_debug_print or >> td_debug_print, IMO is redundant. >> > > No - this remapping is not necessary and might be removed. > Should it be handled as part of this code review or is it ok > to file separate bug for this ? >
According to our chat, it will be handled as separate bug. > >> Here are few more comments. >> >> ls_main.c >> LS_MAXCMDLEN - can't this be MAXPATHLEN ? >> > > I have changed this. > > >> 173: misleading message. strlcat failed, as opposed to the >> redirection. Why not use strerror to provide the real error. It >> appears that even if this redirection fails we continue. >> > > You are right - I changed the message. I think we can't take advantage of > using strerror in this case since, strlcat doesn't set errno ? > Based of our discussion, strerror will be used in this case, since strlcat does set errno. > >> 182: The information returned from error is discarded. It should be >> included. >> > > Agreed - I have modified the code, so that if CLI returns with error, it > is logged. > > >> 294: This is a redundant test since e can never be NULL. Worst case, >> e = s and you have already verified that s is != NULL. >> >> > > You are right - I removed the redundant check. > > > _______________________________________________ > caiman-discuss mailing list > caiman-discuss at opensolaris.org > http://mail.opensolaris.org/mailman/listinfo/caiman-discuss >
