[asterisk-dev] [Code Review]: Pinequeue: Play queue prompts in the background - making call available to agent faster

Thu May 24 07:09:26 CDT 2012

> On April 26, 2012, 5:48 p.m., Mark Michelson wrote:
> > /trunk/apps/app_queue.c, lines 4985-4993
> > <https://reviewboard.asterisk.org/r/1887/diff/2/?file=27573#file27573line4985>
> >
> >     I dont' understand this comment, and I don't understand why you would allow the memberdelay to occur without checking if autoservice succeeded in starting.

Neither do I as I did not write it myself, so it's now removed.

> On April 26, 2012, 5:48 p.m., Mark Michelson wrote:
> > /trunk/apps/app_queue.c, lines 4998-5000
> > <https://reviewboard.asterisk.org/r/1887/diff/2/?file=27573#file27573line4998>
> >
> >     This change seems unnecessary since res3 is not referenced anywhere else.

ok

> On April 26, 2012, 5:48 p.m., Mark Michelson wrote:
> > /trunk/apps/app_queue.c, line 6220
> > <https://reviewboard.asterisk.org/r/1887/diff/2/?file=27573#file27573line6220>
> >
> >     Use PATH_MAX for file name sizes.

ok

> On April 26, 2012, 5:48 p.m., Mark Michelson wrote:
> > /trunk/apps/app_queue.c, line 6357
> > <https://reviewboard.asterisk.org/r/1887/diff/2/?file=27573#file27573line6357>
> >
> >     Use PATH_MAX for the size of file names.

ok

> On April 26, 2012, 5:48 p.m., Mark Michelson wrote:
> > /trunk/apps/app_queue.c, line 6380
> > <https://reviewboard.asterisk.org/r/1887/diff/2/?file=27573#file27573line6380>
> >
> >     Always have the channel locked when you call ast_channel_datastore_find()

ok

> On April 26, 2012, 5:48 p.m., Mark Michelson wrote:
> > /trunk/apps/app_queue.c, lines 6384-6386
> > <https://reviewboard.asterisk.org/r/1887/diff/2/?file=27573#file27573line6384>
> >
> >     Use ast_copy_string instead of strcpy.

ok

> On April 26, 2012, 5:48 p.m., Mark Michelson wrote:
> > /trunk/apps/app_queue.c, lines 6401-6402
> > <https://reviewboard.asterisk.org/r/1887/diff/2/?file=27573#file27573line6401>
> >
> >     What is the point of these two lines?

A very good question. Removed.

> On April 26, 2012, 5:48 p.m., Mark Michelson wrote:
> > /trunk/apps/app_queue.c, line 6403
> > <https://reviewboard.asterisk.org/r/1887/diff/2/?file=27573#file27573line6403>
> >
> >     Use ast_free()

Done

> On April 26, 2012, 5:48 p.m., Mark Michelson wrote:
> > /trunk/apps/app_queue.c, lines 6427-6428
> > <https://reviewboard.asterisk.org/r/1887/diff/2/?file=27573#file27573line6427>
> >
> >     Check the return of both functions to make sure that data was successfully allocated.

Fixed.

> On April 26, 2012, 5:48 p.m., Mark Michelson wrote:
> > /trunk/apps/app_queue.c, line 6449
> > <https://reviewboard.asterisk.org/r/1887/diff/2/?file=27573#file27573line6449>
> >
> >     You don't need to output an error for a failed allocation since ast_calloc will do that for you.

Ok

> On April 26, 2012, 5:48 p.m., Mark Michelson wrote:
> > /trunk/apps/app_queue.c, line 6453
> > <https://reviewboard.asterisk.org/r/1887/diff/2/?file=27573#file27573line6453>
> >
> >     I don't understand why you save chan into the generator. The generator callbacks all take the channel as a parameter.

Further investigation needed.

> On April 26, 2012, 5:48 p.m., Mark Michelson wrote:
> > /trunk/apps/app_queue.c, line 6652
> > <https://reviewboard.asterisk.org/r/1887/diff/2/?file=27573#file27573line6652>
> >
> >     Use ast_calloc and put a space after the comma. Check the result of ast_calloc to be sure that aqsi was properly allocated.

Ok

> On April 26, 2012, 5:48 p.m., Mark Michelson wrote:
> > /trunk/apps/app_queue.c, line 6653
> > <https://reviewboard.asterisk.org/r/1887/diff/2/?file=27573#file27573line6653>
> >
> >     Instead of doing this, just initialize datastore to NULL when it is declared.

ok

> On April 26, 2012, 5:48 p.m., Mark Michelson wrote:
> > /trunk/apps/app_queue.c, line 6659
> > <https://reviewboard.asterisk.org/r/1887/diff/2/?file=27573#file27573line6659>
> >
> >     Check to be sure that ast_datastore_alloc() succeeds. Also, remove the parentheses after ast_prompt_list
> 
> Olle E Johansson wrote:
>     Why remove parenthesis when calling a function?
> 
> Mark Michelson wrote:
>     Sorry I thought I had remove my comment about the parentheses once I realized that it was a function.

done.

> On April 26, 2012, 5:48 p.m., Mark Michelson wrote:
> > /trunk/apps/app_queue.c, line 6664
> > <https://reviewboard.asterisk.org/r/1887/diff/2/?file=27573#file27573line6664>
> >
> >     Use ast_copy_string since strcpy is prone to buffer overflows. It may be that qe.moh and aqsi.moh are currently sized the same, but that guarantee may not always last.

done

> On April 26, 2012, 5:48 p.m., Mark Michelson wrote:
> > /trunk/apps/app_queue.c, line 6667
> > <https://reviewboard.asterisk.org/r/1887/diff/2/?file=27573#file27573line6667>
> >
> >     Always lock the channel when adding a datastore.

locked. key thrown away.

> On April 26, 2012, 5:48 p.m., Mark Michelson wrote:
> > /trunk/apps/app_queue.c, line 6704
> > <https://reviewboard.asterisk.org/r/1887/diff/2/?file=27573#file27573line6704>
> >
> >     Please don't add initials to code comments. See the third bullet point here: https://wiki.asterisk.org/wiki/display/AST/Coding+Guidelines#CodingGuidelines-GeneralRules
> 
> Olle E Johansson wrote:
>     I thought I remove all of those. Just a mistake.

Fixed.

> On April 26, 2012, 5:48 p.m., Mark Michelson wrote:
> > /trunk/include/asterisk/channel.h, lines 3516-3535
> > <https://reviewboard.asterisk.org/r/1887/diff/2/?file=27575#file27575line3516>
> >
> >     One thing that may simplify your life here is to check out global_datastores.h and global_datastores.c. We store datastore information in those files if the datastore may be accessed by multiple source files. An example of one is the datastore that keeps track of all the interfaces that have been dialed by a channel to ensure that the channel does not get caught in a forwarding loop.
> >     
> >     Using those files, you would not have to spread out the datastore information between channel.h, file.h, and file.c
> 
> Olle E Johansson wrote:
>     Great. Thanks.

Not done. Moved to "general overhaul".

> On April 26, 2012, 5:48 p.m., Mark Michelson wrote:
> > /trunk/include/asterisk/channel.h, lines 3527-3529
> > <https://reviewboard.asterisk.org/r/1887/diff/2/?file=27575#file27575line3527>
> >
> >     Use doxygen-style comments here.

Done.

> On April 26, 2012, 5:48 p.m., Mark Michelson wrote:
> > /trunk/main/say.c, line 443
> > <https://reviewboard.asterisk.org/r/1887/diff/2/?file=27579#file27579line443>
> >
> >     Invert this logic so that you can return early if !aqsi and then you can reduce the indentation of the majority of this function. You can actually give the same treatment to the if block above where you call ast_channel_datastore_find()

Done.

- Olle E

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

On April 26, 2012, 9:42 a.m., Olle E Johansson wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/1887/
> -----------------------------------------------------------
> 
> (Updated April 26, 2012, 9:42 a.m.)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Summary
> -------
> 
> Today, when a prompt is being played to a call waiting in the queue and an agent becomes available, the agent will not get the call until the prompt is finished. Both customer and agent is kept waiting.
> 
> With this rather big piece of code, we attach a generator to play prompts. The generator can, like all generators, be stopped at any time so that the agent (queue member) can get the call immediately.
> 
> The generator is now placed in app_queue, but could propably be moved somewhere else. It also changes functionality in main/say.c in order to be able to place those prompts in the same prompt queue for background processing. The same generator could be used to sevice conference bridges and maybe be added as a dialplan function at some point for background playlists.
> 
> 
> This addresses bug 19795.
>     https://issues.asterisk.org/jira/browse/19795
> 
> 
> Diffs
> -----
> 
>   /trunk/apps/app_queue.c 364000 
>   /trunk/configs/queues.conf.sample 364000 
>   /trunk/include/asterisk/channel.h 364000 
>   /trunk/include/asterisk/file.h 364000 
>   /trunk/main/asterisk.dynamics 364000 
>   /trunk/main/file.c 364000 
>   /trunk/main/say.c 364000 
> 
> Diff: https://reviewboard.asterisk.org/r/1887/diff
> 
> 
> Testing
> -------
> 
> Quite a lot of testing in our environment during the rather long time we've been testing this. Customer is happy.
> 
> 
> There was some complaints from a bowl of petunias, which made me a bit surprised.
> 
> 
> Thanks,
> 
> Olle E
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-dev/attachments/20120524/b5a51230/attachment-0001.htm>