[asterisk-dev] [Code Review]: MixMonitor Leaves Empty Audio Files Behind

[elguero]

> On Aug. 12, 2012, 8:14 p.m., Matt Jordan wrote:
> > While I agree this behavior is annoying and probably not wanted, changing the code to delete 0 byte files will change how Asterisk works.  For example, people may have a MixMonitor post-processing script that they run that always assumes a file exists - regardless of its size - after a call has terminated.  You may want to update the UPGRADE notes reflecting the change in behavior.

So with this being a change in behavior, is this only a candidate for trunk?


> On Aug. 12, 2012, 8:14 p.m., Matt Jordan wrote:
> > /branches/10/apps/app_mixmonitor.c, lines 480-489
> > <https://reviewboard.asterisk.org/r/2068/diff/1/?file=30910#file30910line480>
> >
> >     There are two comments I have here.
> >     
> >     1) While this approach will certainly work, it will result in setting the wrotesomething flag each time the audiohook has received frames from the channel.  Instead, you could use the stat function to get the size of the file.  If the stat function returns a filesize of 0 bytes, you could then delete the file.  This would prevent having to use a flag to record when a frame is written.
> >     
> >     2) The code as currently written will have a race condition between deleting the file and the spawning of the external post-processing process.  This should probably be something like:
> >     
> >     if (file_is_empty) {
> >       ast_filedelete(mixmonitor->filename, NULL);
> >     } else if (mixmonitor->post_process) {
> >       ast_safe_system(mixmonitor->post_process);
> >     }

Yes, I was not happy with the flag being set like that.  The problem is that the filesize will not be 0 bytes.  Before capturing frames and saving it to the file, the file contains a header which will probably vary the size of the file depending on the format used.  Otherwise, yes, using stat would have been the quick solution.

Good call on the race condition.  I missed that line.  We obviously do not want to post process the file if it is going to be deleted or was deleted.

I have put some more thought into this and have a different approach to determining if a file should be removed or not.


- elguero


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


On Aug. 12, 2012, 2:12 p.m., elguero wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/2068/
> -----------------------------------------------------------
> 
> (Updated Aug. 12, 2012, 2:12 p.m.)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Summary
> -------
> 
> When the MixMonitor application is started, it creates an empty audio file (just the header is present) for writing an audio stream.
> 
> In the event that MixMonitor is started before dialing (in the case on the issue, record only a bridged channel) but the bridge never is setup due to the other side not answering or for whatever reason the bridge did not succeed in being created, MixMonitor does not clean up the empty audio file.  This has the effect of leaving empty audio files behind.
> 
> This proposed patch is a simple one.  I took a look at app_record to see how it handles files that do not contain any audio and proceeded to create this patch to match that behavior.
> 
> 
> This addresses bug ASTERISK-20156.
>     https://issues.asterisk.org/jira/browse/ASTERISK-20156
> 
> 
> Diffs
> -----
> 
>   /branches/10/apps/app_mixmonitor.c 371176 
> 
> Diff: https://reviewboard.asterisk.org/r/2068/diff
> 
> 
> Testing
> -------
> 
> Reproducing this issue is pretty straight forward.  I tested this patch on one of my local dev machines.  I have yet to get a response on the issue as to whether it is working for the original reporter.
> 
> 
> Thanks,
> 
> elguero
> 
>

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