[asterisk-dev] [Code Review]: Convert app_page to use app_confbridge internally

[Joshua Colp]

> On Feb. 20, 2012, 3:58 p.m., Russell Bryant wrote:
> > /trunk/apps/app_confbridge.c, line 1002
> > <https://reviewboard.asterisk.org/r/1754/diff/1/?file=24445#file24445line1002>
> >
> >     Is this going to block while the announcement is played?  Is it ok to block with the conf bridge lock held?
> 
> Russell Bryant wrote:
>     I talked to file about this last night.  It's safe.
>     
>     Ship it!

Nope! play_prompt_to_channel unlocks and locks again. It's made to handle that.


> On Feb. 20, 2012, 3:58 p.m., Russell Bryant wrote:
> > /trunk/apps/app_page.c, line 199
> > <https://reviewboard.asterisk.org/r/1754/diff/1/?file=24446#file24446line199>
> >
> >     One of these days, we should make this return a ref counted object ... (not necessary to do in this patch, of course)

Agreed.


> On Feb. 20, 2012, 3:58 p.m., Russell Bryant wrote:
> > /trunk/apps/confbridge/conf_config_parser.c, line 190
> > <https://reviewboard.asterisk.org/r/1754/diff/1/?file=24447#file24447line190>
> >
> >     I guess this should be added to CHANGES

Yesssss.


> On Feb. 20, 2012, 3:58 p.m., Russell Bryant wrote:
> > /trunk/apps/confbridge/conf_config_parser.c, line 520
> > <https://reviewboard.asterisk.org/r/1754/diff/1/?file=24447#file24447line520>
> >
> >     Seems like overkill when just setting the first byte to 0 is sufficient

I followed the convention in the file, that is what is used for other settings.


- Joshua


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


On Feb. 20, 2012, 10:33 a.m., Joshua Colp wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/1754/
> -----------------------------------------------------------
> 
> (Updated Feb. 20, 2012, 10:33 a.m.)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Summary
> -------
> 
> The Page application was originally written to use MeetMe internally but as development has progressed ConfBridge has become the new favored conference bridge application. This task was originally to change Page to use the bridging API internally but that proved to be too low level and numerous features from ConfBridge would have been duplicated. Page has therefore been rewritten to use ConfBridge internally.
> 
> 
> Diffs
> -----
> 
>   /trunk/apps/app_confbridge.c 355010 
>   /trunk/apps/app_page.c 355010 
>   /trunk/apps/confbridge/conf_config_parser.c 355010 
>   /trunk/apps/confbridge/include/confbridge.h 355010 
>   /trunk/include/asterisk/dial.h 355010 
>   /trunk/main/dial.c 355010 
> 
> Diff: https://reviewboard.asterisk.org/r/1754/diff
> 
> 
> Testing
> -------
> 
> Tested Paging multiple phones with different combinations of features.
> 
> 
> Thanks,
> 
> Joshua
> 
>

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