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

[Jonathan Rose]

> On June 23, 2014, 4:36 p.m., Matt Jordan wrote:
> > /trunk/res/res_fax.c, lines 318-325
> > <https://reviewboard.asterisk.org/r/3666/diff/1/?file=60383#file60383line318>
> >
> >     For duplicate fields, use an xi:include to pull in the documentation. Don't reproduce documentation that you don't have to reproduce.

Part of the problem here is that in the case of Operation at least, both FAXSessions and FAXSession are both using different sets of values to mean the same thing. I think I'll go ahead and unify the language for these


> On June 23, 2014, 4:36 p.m., Matt Jordan wrote:
> > /trunk/res/res_fax.c, line 4078
> > <https://reviewboard.asterisk.org/r/3666/diff/1/?file=60383#file60383line4078>
> >
> >     I wouldn't name this function "append", as it doesn't really append the event - it sends it out as a completely separate event.
> >     
> >

changed to manager_fax_sessions_entry on account of the event name used.


> On June 23, 2014, 4:36 p.m., Matt Jordan wrote:
> > /trunk/res/res_fax.c, line 4084
> > <https://reviewboard.asterisk.org/r/3666/diff/1/?file=60383#file60383line4084>
> >
> >     This call can fail. If it does, emit an error and bail appropriately.

Problem is, there is currently no way to detect whether generate_filenames_string returned NULL due to an empty list or if it was actually due to an allocation failure. Currently the CLI output doesn't try to distinguish between the two, but I suppose at the very least I should add some NULL checking.


- Jonathan


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


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/2c80b7d7/attachment-0001.html>