[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?
> 
> pabelanger wrote:
>     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.
> 
> Russell Bryant wrote:
>     I suppose one benefit to going the registration route is that the events available in the documentation reflect what events are actually supported given what modules have been loaded and the options they were compiled with.  The same goes for the other XML documentation types, since they are loaded at entity registration time.
>     
>     However, if we were to proceed with having the event docs registered, then I think it should be its own API call.
>     
>     Another complexity with registering events is figuring out who is responsible for registering the more common event types that are used in more than one place.  How would that be handled?
>     
>     I'm a little torn, and tempted to just recommend going with loading the docs straight from the XML doc in this case.

If you have spare time, perhaps we call talk this week?


- 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
> 
>