[asterisk-dev] [Code Review] 2693: ARI: Implement /recordings/stored API's

[Matt Jordan]

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



/trunk/res/stasis_recording/stored.c
<https://reviewboard.asterisk.org/r/2693/#comment18383>

    Typically when I see a char * in a struct, I assume that it points to dynamically allocated memory.
    
    In this case, format points to the place in either file or file_with_ext that contains the format extension. Since both of those are const char *, this can probably be a const char * as well.
    
    (You also wouldn't want someone to attempt to modify a string field through this char * either)
    
    That would make the semantics of format a bit clearer.



/trunk/res/stasis_recording/stored.c
<https://reviewboard.asterisk.org/r/2693/#comment18384>

    I'm assuming that the intent of providing an initializer list here is to set the members of entry to 0.
    
    It's a little vague to me whether or not having an empty initializer list counts as having at least one initializer, and thus whether or not each struct member past the first initializer will be set to 0 or not becomes very murky.
    
    If this were " entry = { 0, };" it'd be much clearer what the intent was.



/trunk/res/stasis_recording/stored.c
<https://reviewboard.asterisk.org/r/2693/#comment18385>

    Same finding here.


- Matt Jordan


On July 29, 2013, 10:21 p.m., David Lee wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/2693/
> -----------------------------------------------------------
> 
> (Updated July 29, 2013, 10:21 p.m.)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Bugs: ASTERISK-21582
>     https://issues.asterisk.org/jira/browse/ASTERISK-21582
> 
> 
> Repository: Asterisk
> 
> 
> Description
> -------
> 
> This patch implements the ARI API's for stored recordings. While the
> original task only specified deleting a recording, it was simple
> enough to implement the GET for all recordings, and for an individual
> recording.
> 
> The recording playback operation was modified to use the same code for
> accessing the recording as the REST API, so that they will behave
> consistently.
> 
> There were several problems with the api-docs that were also fixed,
> bringing the ARI spec in line with the implementation. There were some
> 'wishful thinking' fields on the stored recording model (duration and
> timestamp) that were removed, because I ended up not implementing a
> metadata file to go along with the recording to store such information.
> 
> The GET /recordings/live operation was removed, since it's not really
> that useful to get a list of all recordings that are currently going
> on in the system. (At least, if we did that, we'd probably want to
> also list all of the current playbacks. Which seems weird.)
> 
> 
> Diffs
> -----
> 
>   /trunk/include/asterisk/stasis_app_recording.h 395718 
>   /trunk/res/Makefile 395718 
>   /trunk/res/ari/ari_model_validators.h 395718 
>   /trunk/res/ari/ari_model_validators.c 395718 
>   /trunk/res/ari/resource_recordings.h 395718 
>   /trunk/res/ari/resource_recordings.c 395718 
>   /trunk/res/res_ari_recordings.c 395718 
>   /trunk/res/res_stasis_playback.c 395718 
>   /trunk/res/res_stasis_recording.c 395718 
>   /trunk/res/stasis_recording/stored.c PRE-CREATION 
>   /trunk/rest-api/api-docs/recordings.json 395718 
> 
> Diff: https://reviewboard.asterisk.org/r/2693/diff/
> 
> 
> Testing
> -------
> 
> Used Swagger-UI to poke around the API. Verified that you couldn't
> delete anything outside the recording's directory.
> 
> 
> Thanks,
> 
> David Lee
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-dev/attachments/20130805/84b7cdfa/attachment-0001.htm>