[asterisk-dev] [Code Review] 2922: ARI Recording: If the file already exists and ifExist is set to fail when we issue the command and we can verify it, issue a 409 instead of a success.

[jrose]

> On Oct. 17, 2013, 2:33 p.m., Mark Michelson wrote:
> > /branches/12/res/res_stasis_recording.c, line 375
> > <https://reviewboard.asterisk.org/r/2922/diff/1/?file=47149#file47149line375>
> >
> >     Hm, I'm not too keen on the use of EAGAIN for this. EAGAIN is typically set if a non-blocking operation would block if attempted. This doesn't really fit here. I think there doesn't need to be a disambiguation between whether the recording exists on the filesystem or if the recording is in progress. In the eyes of most users, the problem is the same, which is that there is a resource conflict.  I think this change can be reverted to use EEXIST.

Well, here's the problem I see with this... file names change with the format specified and you can overwrite them if you have the right options enabled. When it comes to this particular error though, it's not because there was a file name problem. It's because there was another recording going on with the same base name. Changing the format in that case does nothing and neither does enabling overwrite. These errors need to be differentiated somehow in order to be useful and I'd need to make bigger changes to propagate the exact failure cause back to the functions that are actually responsible for posting the response if I can't just change the errno.


- jrose


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


On Oct. 16, 2013, 8:48 p.m., jrose wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/2922/
> -----------------------------------------------------------
> 
> (Updated Oct. 16, 2013, 8:48 p.m.)
> 
> 
> Review request for Asterisk Developers, David Lee and kmoore.
> 
> 
> Bugs: ASTERISK-22623
>     https://issues.asterisk.org/jira/browse/ASTERISK-22623
> 
> 
> Repository: Asterisk
> 
> 
> Description
> -------
> 
> This affects both channel and bridge recording. Basically it just adds a simple check to make sure that the requested file doesn't already exist on the file system when we want to record. If it does, we set EEXIST on errno and return error. I went ahead and switched the errno value for when another live recording exists that uses the same base name to EAGAIN... not sure how appropriate that is.
> 
> I also added event models for Recording events, but nothing issues these events yet.  They'll be useful later.
> 
> I've written a test for this, which is in review here: https://reviewboard.asterisk.org/r/2921/
> 
> 
> Diffs
> -----
> 
>   /branches/12/res/ari/ari_model_validators.h 400905 
>   /branches/12/res/ari/ari_model_validators.c 400905 
>   /branches/12/res/ari/resource_bridges.c 400905 
>   /branches/12/res/ari/resource_channels.c 400905 
>   /branches/12/res/res_ari_bridges.c 400905 
>   /branches/12/res/res_ari_channels.c 400905 
>   /branches/12/res/res_stasis_recording.c 400905 
>   /branches/12/rest-api/api-docs/bridges.json 400905 
>   /branches/12/rest-api/api-docs/channels.json 400905 
>   /branches/12/rest-api/api-docs/events.json 400905 
>   /branches/12/rest-api/api-docs/recordings.json 400905 
> 
> Diff: https://reviewboard.asterisk.org/r/2922/diff/
> 
> 
> Testing
> -------
> 
> I tested it manually with Swagger UI and attempted to record when a file existed where I wanted one and received the expected 409.  I also checked to make sure I would still succeed if ifExists was set to overwrite. I also checked for success when the file just didn't exist in the first place. I think that covers all my bases. All of these scenarios are also covered by the automated test in review 2921
> 
> 
> Thanks,
> 
> jrose
> 
>

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