[asterisk-dev] [Code Review] CLI filtering [GSoC 2009]

Fri Aug 7 11:58:02 CDT 2009

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/336/#review1008
-----------------------------------------------------------

I may have a few more comments later, but this is what I have for my first passthrough. My comments may overlap with eliel's since I was in the middle of writing this when his review was published.

/trunk/include/asterisk/logger.h
<https://reviewboard.asterisk.org/r/336/#comment2377>

    Please add doxygen-style documentation to these function declarations.

/trunk/include/asterisk/pbx.h
<https://reviewboard.asterisk.org/r/336/#comment2378>

    This should be removed.

/trunk/include/asterisk/threadstorage.h
<https://reviewboard.asterisk.org/r/336/#comment2379>

    This change seems unrelated to your work and should be reverted.

/trunk/main/logger.c
<https://reviewboard.asterisk.org/r/336/#comment2389>

    I suggest that instead of making type an int, you should use an enum. This way, the types may be expressed using constants that suggest what the type is supposed to be. This way, you don't have to use arbitrary 1, 2, and 3 types like you are currently using.

/trunk/main/logger.c
<https://reviewboard.asterisk.org/r/336/#comment2380>

    This comment doesn't make sense and is not correct. strcasecmp and strncasecmp are both case INsensitive. Also, you say that you do not use strncasecmp, but it is quite clear that you are using it in the comparison below.

    This is a case where the code is 100% correct but the comment is not.

/trunk/main/logger.c
<https://reviewboard.asterisk.org/r/336/#comment2381>

    Remove the extra space at the end.

/trunk/main/logger.c
<https://reviewboard.asterisk.org/r/336/#comment2382>

    s/has/have/

/trunk/main/logger.c
<https://reviewboard.asterisk.org/r/336/#comment2383>

    whitespace

/trunk/main/logger.c
<https://reviewboard.asterisk.org/r/336/#comment2384>

    lol. Please revert this.

/trunk/main/logger.c
<https://reviewboard.asterisk.org/r/336/#comment2385>

    s/provides/provide/

/trunk/main/logger.c
<https://reviewboard.asterisk.org/r/336/#comment2386>

    s/provides/provide/

/trunk/main/logger.c
<https://reviewboard.asterisk.org/r/336/#comment2394>

    space after comma.

/trunk/main/logger.c
<https://reviewboard.asterisk.org/r/336/#comment2395>

    whitespace

/trunk/main/logger.c
<https://reviewboard.asterisk.org/r/336/#comment2387>

    Be sure to add another \r\n after the idText.

/trunk/main/logger.c
<https://reviewboard.asterisk.org/r/336/#comment2399>

    You should make sure a name was provided before calling ast_filter_remove.

/trunk/main/logger.c
<https://reviewboard.asterisk.org/r/336/#comment2400>

    remove the space after ( and the space before )

/trunk/main/logger.c
<https://reviewboard.asterisk.org/r/336/#comment2398>

    \r\n after the idText.

/trunk/main/logger.c
<https://reviewboard.asterisk.org/r/336/#comment2397>

    s/doesnt exists/doesn't exist/

/trunk/main/logger.c
<https://reviewboard.asterisk.org/r/336/#comment2402>

    whitespace

/trunk/main/logger.c
<https://reviewboard.asterisk.org/r/336/#comment2403>

    \r\n after the idText

/trunk/main/logger.c
<https://reviewboard.asterisk.org/r/336/#comment2388>

    Since ast_strdup allocates space on the heap, you should check for failures here.

/trunk/main/logger.c
<https://reviewboard.asterisk.org/r/336/#comment2405>

    See my other comment about using an enum for the type.

/trunk/main/logger.c
<https://reviewboard.asterisk.org/r/336/#comment2407>

    I think you can safely remove this TODO comment. At the beginning of the function, you call ast_filter_get_by_name and return if you get a result.

/trunk/main/logger.c
<https://reviewboard.asterisk.org/r/336/#comment2409>

    whitespace

/trunk/main/logger.c
<https://reviewboard.asterisk.org/r/336/#comment2408>

    Move the AST_LIST_TRAVERSE_SAFE_END to outside the list traversal loop.

/trunk/main/logger.c
<https://reviewboard.asterisk.org/r/336/#comment2412>

    These statements are indented to the right too much.

/trunk/main/logger.c
<https://reviewboard.asterisk.org/r/336/#comment2410>

    Move AST_LIST_TRAVERSE_SAFE_END to outside the list traversal loop.

/trunk/main/logger.c
<https://reviewboard.asterisk.org/r/336/#comment2413>

    Since your list is not an RWLIST, you should just use AST_LIST_INSERT_HEAD here.

/trunk/main/logger.c
<https://reviewboard.asterisk.org/r/336/#comment2416>

    You can decrease the indentation level here by reversing the logic of the if statement and returning early if channels is NULL.

/trunk/main/logger.c
<https://reviewboard.asterisk.org/r/336/#comment2415>

    Russell has already pointed out that you need to increase the channel refcount in ast_tls_add_channel, but it should also be pointed out that you will need to decrease the refcount in this function.

/trunk/main/logger.c
<https://reviewboard.asterisk.org/r/336/#comment2414>

    Move the AST_LIST_TRAVERSE SAFE_END...

/trunk/main/logger.c
<https://reviewboard.asterisk.org/r/336/#comment2417>

    I strongly suggest reversing the logic in the if statement so that you can reduce the indentation level of this function.

/trunk/main/logger.c
<https://reviewboard.asterisk.org/r/336/#comment2418>

    Another chance to reduce the indentation level.

/trunk/main/logger.c
<https://reviewboard.asterisk.org/r/336/#comment2419>

    Add spaces before and after =

/trunk/main/logger.c
<https://reviewboard.asterisk.org/r/336/#comment2420>

    Add spaces before and after =

/trunk/main/logger.c
<https://reviewboard.asterisk.org/r/336/#comment2421>

    whitespace

/trunk/main/logger.c
<https://reviewboard.asterisk.org/r/336/#comment2411>

    These debugging functions should be removed before you merge this work.

/trunk/main/manager.c
<https://reviewboard.asterisk.org/r/336/#comment2390>

    Please make sure the documentation is accurate. It looks like you created a "skeleton" for documentation but did not fill in the proper details.

    Also, I think this documentation should be moved to logger.c since that is where the actual code for these manager actions is.

/trunk/main/pbx.c
<https://reviewboard.asterisk.org/r/336/#comment2422>

    Be sure to remove the channel from this thread's list when ast_pbx_start is called.

/trunk/main/threadstorage.c
<https://reviewboard.asterisk.org/r/336/#comment2423>

    This change is unrelated and should be removed.

- Mark

On 2009-08-06 22:01:42, junky wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/336/
> -----------------------------------------------------------
> 
> (Updated 2009-08-06 22:01:42)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Summary
> -------
> 
> CLI filtering for the gsoc 2009
> 
> This is the first patch to add functionality to filter CLI.
> 
> 
> See usages and examples in doc/cli_filters.txt
> 
> 
> Diffs
> -----
> 
>   /trunk/channels/chan_agent.c 210860 
>   /trunk/channels/chan_alsa.c 210860 
>   /trunk/channels/chan_bridge.c 210860 
>   /trunk/channels/chan_dahdi.c 210860 
>   /trunk/channels/chan_gtalk.c 210860 
>   /trunk/channels/chan_h323.c 210860 
>   /trunk/channels/chan_iax2.c 210860 
>   /trunk/channels/chan_jingle.c 210860 
>   /trunk/channels/chan_local.c 210860 
>   /trunk/channels/chan_mgcp.c 210860 
>   /trunk/channels/chan_misdn.c 210860 
>   /trunk/channels/chan_multicast_rtp.c 210860 
>   /trunk/channels/chan_nbs.c 210860 
>   /trunk/channels/chan_oss.c 210860 
>   /trunk/channels/chan_phone.c 210860 
>   /trunk/channels/chan_skinny.c 210860 
>   /trunk/channels/chan_unistim.c 210860 
>   /trunk/channels/chan_usbradio.c 210860 
>   /trunk/channels/chan_vpb.cc 210860 
>   /trunk/include/asterisk/logger.h 210860 
>   /trunk/include/asterisk/pbx.h 210860 
>   /trunk/include/asterisk/threadstorage.h 210860 
>   /trunk/main/logger.c 210860 
>   /trunk/main/manager.c 210860 
>   /trunk/main/pbx.c 210860 
>   /trunk/main/threadstorage.c 210860 
> 
> Diff: https://reviewboard.asterisk.org/r/336/diff
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> junky
> 
>