[asterisk-dev] [Code Review] 3494: ARI: Add the ability to raise an arbitrary User Event from the Applications resource

[Scott Griepentrog]

> On May 20, 2014, 10:38 a.m., Matt Jordan wrote:
> > /branches/12/main/stasis.c, lines 1024-1030
> > <https://reviewboard.asterisk.org/r/3494/diff/3/?file=58411#file58411line1024>
> >
> >     Sticking heterogeneous types into a single ao2_container still feels like the wrong approach here, for a few reasons:
> >     
> >     (1) The types are known before hand. We know that we have channels, bridges, and endpoint. We can use an array here of some other container, which will be much more efficient then constructing a specific hash based on a prefix type. It also would eliminate code that determines the type of snapshot later.
> >     
> >     (2) ao2 containers are a bad choice here. They are useful when the pattern of usage is to look up an item by key; however, here the pattern of usage is to iterate over all the items. This should use a list structure or a vector instead.
> >     
> >     Ideally, what we'd have here would be something like this:
> >     
> >     struct ast_multi_object_blob {
> >         struct ast_json *blob;
> >         AST_VECTOR(, void *) snapshots[STASIS_UMOS_MAX];
> >     };
> >     
> >     That gives you an array of vectors, each of which is typed to the enum you've already defined. Since this is really an array of pointers to arrays of pointers, it's about as fast as you're going to get. It also doesn't require any custom keying, comparisons, or a lot of ao2 lifetime management that is unnecessary.
> >     
> >     (As an aside: when we wrote the multi-channel blob stuff originally, we didn't have vectors. It'd probably be good to open an issue to go back and vectorize those).

Although it has resulted in much cleaner code, this change eliminates my original plan of having multi object blob be a compatible replacement for multi channel due to no longer having named object support.  Similarly, while changing multi channel to use vectors is desirable, an object wrapper is still necessary in that instance to hold the name prefix to differentiate multiple instances of the same object type (used by AMI, although not by ARI).


> On May 20, 2014, 10:38 a.m., Matt Jordan wrote:
> > /branches/12/main/stasis.c, lines 1288-1303
> > <https://reviewboard.asterisk.org/r/3494/diff/3/?file=58411#file58411line1288>
> >
> >     This documentation is incomplete. Right now, the only parameter it will show as being 'possible' will be a single channel and the UserEvent parameter, which isn't really true.
> >     
> >     There's also no real benefit defining this in-line in the source file. The only benefit for doing that is when you have multiple events of the same type, or when the parameters of the event can be inferred from the manager_event* call, neither of which is true here.
> >     
> >     I'd define the event at the top, and specify the parameters that can be present specifically. Parameters can be labelled as being optional. If the parameters are too difficult to list (since there can be a substantial number of them now), at a minimum a description of the event should be added and specify how and what parameters are possible.

Trying to mark bridge and endpoint snapshots as required="false" resulted in an error.  I resorted to describing the additional functionality instead.


- Scott


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


On May 12, 2014, 3:45 p.m., Scott Griepentrog wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/3494/
> -----------------------------------------------------------
> 
> (Updated May 12, 2014, 3:45 p.m.)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Bugs: ASTERISK-22697
>     https://issues.asterisk.org/jira/browse/ASTERISK-22697
> 
> 
> Repository: Asterisk
> 
> 
> Description
> -------
> 
> This changes the implementation of UserEvent stasis messaging and adds additional features:
> 
> 1) Existing channel_user_event stasis messaging replaced with new multi_object_blob in stasis_user.c
> 2) Multi object blob provides ability to signal blob + any number of channel, bridge, or endpoint snapshots
> 3) Changed existing Userevent() app to use new multi object blob - but still publishes to channel
> 4) Added ARI implementation of userevent as /ari/applications/{applicationName}/user/{eventName}
> 5) ARI generated userevent messages are published to application, and also to AMI if channel present
> 
> 
> Diffs
> -----
> 
>   /branches/12/rest-api/api-docs/events.json 413115 
>   /branches/12/res/stasis/app.c 413115 
>   /branches/12/res/res_stasis.c 413115 
>   /branches/12/res/res_ari_events.c 413115 
>   /branches/12/res/ari/resource_events.c 413115 
>   /branches/12/res/ari/resource_events.h 413115 
>   /branches/12/res/ari/ari_model_validators.c 413115 
>   /branches/12/res/ari/ari_model_validators.h 413115 
>   /branches/12/main/stasis_endpoints.c 413115 
>   /branches/12/main/stasis_channels.c 413115 
>   /branches/12/main/stasis.c 413115 
>   /branches/12/main/manager_channels.c 413115 
>   /branches/12/include/asterisk/stasis_channels.h 413115 
>   /branches/12/include/asterisk/stasis_app.h 413115 
>   /branches/12/include/asterisk/stasis.h 413115 
>   /branches/12/apps/app_userevent.c 413115 
> 
> Diff: https://reviewboard.asterisk.org/r/3494/diff/
> 
> 
> Testing
> -------
> 
> Tested with valgrind to insure no invalid references.  Some leaks found which have been attributed to other code to be fixed separately.
> 
> 
> Thanks,
> 
> Scott Griepentrog
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-dev/attachments/20140521/d0531c53/attachment-0001.html>