[asterisk-dev] [Code Review] 3025: ARI: Implement device state API

[Matt Jordan]

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



branches/12/include/asterisk/stasis_app_device_state.h
<https://reviewboard.asterisk.org/r/3025/#comment19564>

    is/has



branches/12/main/devicestate.c
<https://reviewboard.asterisk.org/r/3025/#comment19563>

    This is horribly expensive.
    
    We're going to have to dump out the entire cache (all devices!), then iterate over the entire container to determine if we want to clear out a single entry.
    
    That has a huge cost associated with it - I'm curious, why do we expect there to be more than one entry in the cache for a single device? I would expect that we could get the last known stasis message for a particular device, call stasis_cache_clear_create on it, then publish to that topic.



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

    You should always use ast_strlen_zero when determining if a character pointer has anything in it. That handles both NULL as well as *s == '\0'



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

    In general, you need some doxygen comments everywhere.
    
    In particular, when you have large lists of parameters containing pointers to pointers, it is especially important to document what those things are.



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

    I get nervous anytime I see manual parsing of URIs. Previously, this didn't attempt to pull out the scheme at all - it just called ast_begins_with on it to see if the URI began with the other.
    
    You should be able to do that here still. Just pass the URI directly to app_event_source_find, which will return back the event_source if the URI begins with the appropriate resource scheme.
    
    Since you now have the scheme - it's in the event_source - and the full URI, you can just do what it previously did and pass the id as uri + strlen(event_source->scheme).
    
    That way if the scheme happens to be something kooky that includes a ':' (which would be weird and strange and wrong, but okay), this will still all "just work".
    
    



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

    Use ast_strlen_zero



branches/12/res/res_stasis_device_state.c
<https://reviewboard.asterisk.org/r/3025/#comment19570>

    Use ast_strlen_zero



branches/12/res/res_stasis_device_state.c
<https://reviewboard.asterisk.org/r/3025/#comment19571>

    This looks like it was left over from debugging.


- Matt Jordan


On Nov. 20, 2013, 10:25 p.m., Kevin Harwell wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/3025/
> -----------------------------------------------------------
> 
> (Updated Nov. 20, 2013, 10:25 p.m.)
> 
> 
> Review request for Asterisk Developers, David Lee and Matt Jordan.
> 
> 
> Bugs: ASTERISK-22838
>     https://issues.asterisk.org/jira/browse/ASTERISK-22838
> 
> 
> Repository: Asterisk
> 
> 
> Description
> -------
> 
> Created a data model and implemented functionality for an ARI device state resource.  The following operations have been added that allow a user to manipulate an ARI controlled device:
> 
> PUT    /device-states/{deviceName}&{deviceState} - Create/Change the state of an ARI controlled device
> GET    /device-states/{deviceName} - Retrieve the current state of a device
> DELETE /device-states/{deviceName} - Destroy a device-state controlled by ARI
> 
> The ARI controlled device must begin with 'Stasis:'.  An example controlled device name would be Stasis:Example.
> 
> A 'DeviceStateChanged' event has also been added so that an application can subscribe and receive device change events.  Any device state, ARI controlled or not, can be subscribed to.
> 
> While adding the event, the underlying subscription control mechanism was refactored so that all current and future resource subscriptions would be the same.  Each event resource must now register itself in order to be able to properly handle [un]subscribes.
> 
> 
> Diffs
> -----
> 
>   branches/12/rest-api/resources.json 402915 
>   branches/12/rest-api/api-docs/events.json 402915 
>   branches/12/rest-api/api-docs/device_states.json PRE-CREATION 
>   branches/12/rest-api/api-docs/applications.json 402915 
>   branches/12/res/stasis/app.c 402915 
>   branches/12/res/res_stasis_device_state.exports.in PRE-CREATION 
>   branches/12/res/res_stasis_device_state.c PRE-CREATION 
>   branches/12/res/res_stasis.c 402915 
>   branches/12/res/res_ari_device_states.c PRE-CREATION 
>   branches/12/res/ari/resource_device_states.c PRE-CREATION 
>   branches/12/res/ari/resource_device_states.h PRE-CREATION 
>   branches/12/res/ari/resource_applications.h 402915 
>   branches/12/res/ari/ari_model_validators.c 402915 
>   branches/12/res/ari/ari_model_validators.h 402915 
>   branches/12/res/ari.make 402915 
>   branches/12/main/devicestate.c 402915 
>   branches/12/include/asterisk/stasis_app_device_state.h PRE-CREATION 
>   branches/12/include/asterisk/stasis_app.h 402915 
>   branches/12/include/asterisk/devicestate.h 402915 
> 
> Diff: https://reviewboard.asterisk.org/r/3025/diff/
> 
> 
> Testing
> -------
> 
> Some basic testing done using the swagger framework and a websocket connection.  Testing adding, changing, and removing device state with regards to an ARI controlled device were successful.  Also tested [un]subscribing to the events from device with success.  Planning on writing some testsuite test cases as well.
> 
> 
> Thanks,
> 
> Kevin Harwell
> 
>

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