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 ? > > 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 ? > > 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.
