[asterisk-dev] [Code Review] 3666: res_fax: fax show session, fax show sessions, fax show stats - providing similar AMI commands

[Matt Jordan]

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



/trunk/res/res_fax.c
<https://reviewboard.asterisk.org/r/3666/#comment22433>

    Use an enum field for this and explicitly call out what values are allowed



/trunk/res/res_fax.c
<https://reviewboard.asterisk.org/r/3666/#comment22434>

    Again, use an enum field here and explicitly call out what values are allowed.



/trunk/res/res_fax.c
<https://reviewboard.asterisk.org/r/3666/#comment22435>

    Enum field



/trunk/res/res_fax.c
<https://reviewboard.asterisk.org/r/3666/#comment22436>

    For duplicate fields, use an xi:include to pull in the documentation. Don't reproduce documentation that you don't have to reproduce.



/trunk/res/res_fax.c
<https://reviewboard.asterisk.org/r/3666/#comment22437>

    Here, you have "TransmitAttempts" - previously, you have "TXPages". Same for Receive/RX.
    
    I'd either spell out Transmit/Receive all the time, or else use the abbreviation.



/trunk/res/res_fax.c
<https://reviewboard.asterisk.org/r/3666/#comment22430>

    Declare these on separate lines. Particularly when one is a pointer (and ref counted) and the other is statically declared, it's nice to be able to tell the two apart.
    
    tmp is also never a good variable name.



/trunk/res/res_fax.c
<https://reviewboard.asterisk.org/r/3666/#comment22429>

    Length limit the width of the allowed session number.



/trunk/res/res_fax.c
<https://reviewboard.asterisk.org/r/3666/#comment22431>

    This leaks session



/trunk/res/res_fax.c
<https://reviewboard.asterisk.org/r/3666/#comment22432>

    You can just use ao2_ref(session, -1) here. ao2_cleanup is only necessary when the item can be NULL.
    
    Using the ao2_ref macros directly has the benefit of being slightly nicer for ref debug logs.



/trunk/res/res_fax.c
<https://reviewboard.asterisk.org/r/3666/#comment22440>

    I wouldn't name this function "append", as it doesn't really append the event - it sends it out as a completely separate event.
    
    



/trunk/res/res_fax.c
<https://reviewboard.asterisk.org/r/3666/#comment22438>

    This call can fail. If it does, emit an error and bail appropriately.



/trunk/res/res_fax.c
<https://reviewboard.asterisk.org/r/3666/#comment22441>

    Rename the cli_session_type function and cli_session_operation function to denote that they are not used just by the CLI any more.



/trunk/res/res_fax.c
<https://reviewboard.asterisk.org/r/3666/#comment22439>

    Remove the extra line between variable declarations



/trunk/res/res_fax_spandsp.c
<https://reviewboard.asterisk.org/r/3666/#comment22445>

    I wouldn't use either of these functions. I'd remove both, and instead use an ast_str to build the output.



/trunk/res/res_fax_spandsp.c
<https://reviewboard.asterisk.org/r/3666/#comment22446>

    Rather than having all of these various malloc'd strings floating around, I'd use a single ast_str to build up the custom body of the event. That way you don't have to do a lot of memory management (just one), and you can directly place the integer values into the string using ast_str_append.



/trunk/res/res_fax_spandsp.c
<https://reviewboard.asterisk.org/r/3666/#comment22428>

    You can free NULL values. It's okay. They won't mind.


- Matt Jordan


On June 23, 2014, 2:42 p.m., Jonathan Rose wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/3666/
> -----------------------------------------------------------
> 
> (Updated June 23, 2014, 2:42 p.m.)
> 
> 
> Review request for Asterisk Developers, Matt Jordan and Mark Michelson.
> 
> 
> Repository: Asterisk
> 
> 
> Description
> -------
> 
> More CLI to AMI command conversions, this time focusing on everyone's favorite method for sending documents, FAX!
> 
> FAXSessions replicates the functionality of fax show sessions, and is more or less a 1:1 duplication.
> FAXSession replicates the functionality of fax show session. Output is slightly stripped down from the CLI variant in order to keep things consistent across multiple FAX modules and not just spandsp.
> FAXStats replicates the functionality of fax show stats, but only provides the fields that res_fax would normally provide in fax show stats and not any of the technology specific fields.
> 
> 
> Diffs
> -----
> 
>   /trunk/res/res_fax_spandsp.c 416868 
>   /trunk/res/res_fax.c 416868 
>   /trunk/include/asterisk/res_fax.h 416868 
> 
> Diff: https://reviewboard.asterisk.org/r/3666/diff/
> 
> 
> Testing
> -------
> 
> Created some simple transmit/receive fax sessions by originating calls to/from faxsend/faxreceive extensions and then ran each of these commands.
> Did the above with and without action_id included to make sure it would be reproduced across events and responses.
> Checked the output of documentation for events and actions for sanity.
> 
> 
> Thanks,
> 
> Jonathan Rose
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-dev/attachments/20140623/13e32210/attachment-0001.html>