[asterisk-dev] [Code Review] New manager actions for xmldoc

[paul.belanger at polybeacon.com]

> On 2010-04-28 16:32:34, Russell Bryant wrote:
> > /trunk/include/asterisk/manager.h, lines 139-182
> > <https://reviewboard.asterisk.org/r/627/diff/2/?file=9567#file9567line139>
> >
> >     I'm not a huge fan of this part of the patch.
> >     
> >     It breaks API and ABI.  While it is trunk, we should always avoid both types of change when possible.
> >     
> >     I don't understand the purpose of registering a manager event.  It seems to be overloading the register API call for actions, which is for registering callbacks.  If a register API call events really is needed, then I'd rather see it as a separate API call, instead of overloading this one and causing the API disruption in the tree.
> >     
> >     Instead of requiring registration of each event, why not just open the XML doc and traverse it for ami_events?

My logic for this was since events and commands _seemed_ very similar, I might as well mash them into the same structure. I don't have a problem changing this.


> On 2010-04-28 16:32:34, Russell Bryant wrote:
> > /trunk/include/asterisk/manager.h, lines 86-88
> > <https://reviewboard.asterisk.org/r/627/diff/2/?file=9567#file9567line86>
> >
> >     New additions to the public API should have an AST_ prefix

I knew that, not sure why I didn't do it.


- pabelanger


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


On 2010-04-27 09:08:53, pabelanger wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/627/
> -----------------------------------------------------------
> 
> (Updated 2010-04-27 09:08:53)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Summary
> -------
> 
> This patch adds 2 new elements for xmldoc: <ami_command> (replacing <manager>) and <ami_event>.
> 
> I have also modified the structure for manager_action to include a type field, creating MANAGER_COMMAND and MANAGER_EVENT.  The major difference with MANAGER_COMMAND and MANAGER_EVENT is events will pass NULL for the function parameter.
> 
> The majority of this patch is converting <manager> to <ami_command>, this was only done for a cosmetic reason.  If the people consider this a big change, I can revert to using <manager>.
> 
> *NOTE* This patch is currently missing updates for in-line documentation.
> 
> 
> This addresses bug 17173.
>     https://issues.asterisk.org/view.php?id=17173
> 
> 
> Diffs
> -----
> 
>   /trunk/apps/app_meetme.c 258973 
>   /trunk/apps/app_mixmonitor.c 258973 
>   /trunk/apps/app_queue.c 258973 
>   /trunk/apps/app_senddtmf.c 258973 
>   /trunk/apps/app_voicemail.c 258973 
>   /trunk/channels/chan_agent.c 258973 
>   /trunk/channels/chan_dahdi.c 258973 
>   /trunk/channels/chan_iax2.c 258973 
>   /trunk/channels/chan_sip.c 258973 
>   /trunk/channels/chan_skinny.c 258973 
>   /trunk/doc/appdocsxml.dtd 258973 
>   /trunk/include/asterisk/manager.h 258973 
>   /trunk/main/data.c 258973 
>   /trunk/main/db.c 258973 
>   /trunk/main/features.c 258973 
>   /trunk/main/manager.c 258973 
>   /trunk/main/pbx.c 258973 
>   /trunk/main/xmldoc.c 258973 
>   /trunk/res/res_agi.c 258973 
>   /trunk/res/res_jabber.c 258973 
>   /trunk/res/res_monitor.c 258973 
>   /trunk/res/res_mutestream.c 258973 
> 
> Diff: https://reviewboard.asterisk.org/r/627/diff
> 
> 
> Testing
> -------
> 
> Local developers and test box.
> 
> 
> Thanks,
> 
> pabelanger
> 
>