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

[Mark Michelson]

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



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

    While using ", " as a separator is good for human-readable output, I think that for AMI, which is supposed to be more text parser friendly, just using "," as the separator is a better plan.
    
    In response to the earlier concern that was brought up here, I can think of two viable suggestions:
    
    1) Manually perform the same check that generate_filenames_string() does to see if the list is empty. If it's empty and you get a NULL return, all is well. If it's not empty and you get a NULL return, uh oh!
    
    2) Change generate_filenames_string() to return an empty string on an empty list and NULL on an allocation failure.
    
    I think 2) is the cleaner way forward, and I don't think it creeps the scope too badly either.



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

    Both the if and else do the same initialization of p. This can be done outside of the if-else block.



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

    If you move the initialization of p out of the else, then you can combine the else and the if below it into an "else if" statement and gain back a level of indentation.



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

    Use ast_str_buffer() instead of directly accessing the contents of an ast_str


- Mark Michelson


On June 26, 2014, 7:54 p.m., Jonathan Rose wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/3666/
> -----------------------------------------------------------
> 
> (Updated June 26, 2014, 7:54 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.exports.in 416868 
>   /trunk/res/res_fax.c 416868 
>   /trunk/include/asterisk/res_fax.h 416868 
>   /trunk/CHANGES 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/20140626/ca63607a/attachment-0001.html>