[caiman-discuss] code review comments for bug 810

[Jan Damborsky]

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.