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

Matt Jordan reviewboard at asterisk.org
Thu May 1 14:26:01 CDT 2014 

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



/branches/12/include/asterisk/stasis_app.h
<https://reviewboard.asterisk.org/r/3494/#comment21607>

    While a callback is needed for the event source to turn itself into something useful for a multi object blob message, I think this could be made more generally useful.
    
    Rather than having the callback explicitly add itself to the multi object blob message, have a callback that converts the object into a snapshot:
    
    void *(*to_snapshot)(void);
    
    Where the void * is an ao2 object representing a snapshot of the object.
    
    This is actually what the various implementations of this callback do - call the snapshot method and add it to the ast_multi_object_blob.
    
    There's two benefits here:
    (1) This is more generally useful than just adding this object to a multi object blob
    (2) It keeps the callbacks in line with the 'to_json' callback - that is, the callbacks aren't responsible for performing actions on some other object, but rather provide information about the object they are implemented on.
    
    



/branches/12/include/asterisk/stasis_app.h
<https://reviewboard.asterisk.org/r/3494/#comment21613>

    Why AGER? App_generate?
    
    I'd use APP_GEN, personally. It's a bit easier to read.



/branches/12/include/asterisk/stasis_user.h
<https://reviewboard.asterisk.org/r/3494/#comment21617>

    Since the multi object blob should get moved to stasis.h, it may be best to move these into the same header file.
    
    That also prevent us from needing yet another thing to try and keep track of in the Asterisk start up sequence (which, really, is already out of control...)
    
    User events are a somewhat generic concept; I think having them defined in stasis.h isn't the worst place in the world :-)



/branches/12/include/asterisk/stasis_user.h
<https://reviewboard.asterisk.org/r/3494/#comment21614>

    Since these are public functions, they need full doxygen documentation.



/branches/12/include/asterisk/stasis_user.h
<https://reviewboard.asterisk.org/r/3494/#comment21615>

    The concept of a multi object blob is really a generic concept, and probably doesn't belong specifically associated with a 'user event'. It just happens to get used by that.
    
    Since this is actually truly a generic payload - it can accept 'stuff' from anything (so long as it is ao2 ref counted), this could probably get moved directly into stasis.h.
    
    On that note, all three of the payload type functions can also be coalesced down to a single function:
    
    ast_multi_object_blob_add(struct ast_multi_object_blob *blob, const char *name, void *obj);
    
    The contract for the function should specify that the object being added must be an ao2 object.
    
    The minimal amount of type safety we get from this is probably not worth the duplication.



/branches/12/main/stasis_user.c
<https://reviewboard.asterisk.org/r/3494/#comment21618>

    There's some finicky parsers that actually scrape this out of the source mid-stream, and will look for the call to ast_manager_... on the next line. I'd remove the empty space.
    
    Proactively, you may want to double check that a 'make full' generates the correct documentation for this event still when you're done.



/branches/12/res/res_stasis.c
<https://reviewboard.asterisk.org/r/3494/#comment21611>

    When the callback is refactored, this should instead get the snapshot from the event source, make sure that the snapshot is valid, then add it to the multi object blob itself.



/branches/12/res/res_stasis.c
<https://reviewboard.asterisk.org/r/3494/#comment21612>

    It's probably worth justifying here in a comment why we publish this twice - once to the Stasis application topic, and once to the AMI topic.



/branches/12/res/stasis/app.c
<https://reviewboard.asterisk.org/r/3494/#comment21608>

    You shouldn't actually need to generate a new snapshot here.
    
    A new snapshot is only needed when the channel itself has changed. Since the channel is not changing as a result of the user event, you can use the cached snapshot (in the Stasis cache) and return that.
    
    Obtaining the cached snapshot is generally much, much faster than generating a new snapshot.



/branches/12/res/stasis/app.c
<https://reviewboard.asterisk.org/r/3494/#comment21609>

    Same finding here about getting the snapshot out of the cache.



/branches/12/res/stasis/app.c
<https://reviewboard.asterisk.org/r/3494/#comment21610>

    Same finding here about getting the snapshot out of the cache.



/branches/12/rest-api/api-docs/applications.json
<https://reviewboard.asterisk.org/r/3494/#comment21606>

    I don't think there should be a userEvent field in the query parameter.
    
    If a user wants to pass arbitrary key/value pairs to Asterisk in their event, that should be done in the body of request, in the same fashion that channel variables are passed.


- Matt Jordan


On April 29, 2014, 11:15 a.m., Scott Griepentrog wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/3494/
> -----------------------------------------------------------
> 
> (Updated April 29, 2014, 11:15 a.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/rest-api/api-docs/applications.json 413115 
>   /branches/12/res/stasis/app.c 413115 
>   /branches/12/res/res_stasis.c 413115 
>   /branches/12/res/res_ari_applications.c 413115 
>   /branches/12/res/ari/resource_applications.c 413115 
>   /branches/12/res/ari/resource_applications.h 413115 
>   /branches/12/res/ari/ari_model_validators.c 413115 
>   /branches/12/res/ari/ari_model_validators.h 413115 
>   /branches/12/main/stasis_user.c PRE-CREATION 
>   /branches/12/main/stasis_channels.c 413115 
>   /branches/12/main/manager_channels.c 413115 
>   /branches/12/main/asterisk.c 413115 
>   /branches/12/include/asterisk/stasis_user.h PRE-CREATION 
>   /branches/12/include/asterisk/stasis_channels.h 413115 
>   /branches/12/include/asterisk/stasis_app.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/20140501/24333a87/attachment-0001.html>

More information about the asterisk-dev mailing list