----------------------------------------------------------- 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 > >
-- _____________________________________________________________________ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
