[asterisk-dev] [Code Review] dialing through chan_local breaks t38 fax

[Mark Michelson]

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


I don't like the way that this is implemented because essentially, chan_local will claim that it can handle T.38 faxes even if the local channel is proxying a channel that cannot or should not handle T.38 faxes.

I think your comment in the diff is the right idea, but the execution is off. Essentially what chan_local needs to do is relay the indication to the channel that it is proxying and return the result of that indication. The tricky part of this is going to be getting the locking correct. When local_indicate is called, the "ast" channel passed in will already be locked. You will need to lock the other channel (you can determine which channel is the other channel using the IS_OUTBOUND macro) before calling ast_indicate_data() on it. However, I don't see a good reason to keep ast locked while calling ast_indicate_data() on the other channel.

Approximately how this would look:

if (condition == AST_CONTROL_T38) {
    ast_channel *other_channel = IS_OUTBOUND(ast, p) ? p->owner : p->chan;
    if (!other_channel) {
        return -1;
    }
    ao2_unlock(ast);
    ao2_lock(other_channel);
    res = ast_indicate_data(other_channel, condition, data, datalen);
    ao2_unlock(other_channel);
    ao2_lock(ast);
    return res;
}

There may be other things you need to do (like unreffing p before returning) but this is only meant to be approximate.

- Mark


On Aug. 14, 2012, 10:46 a.m., wdoekes wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/2070/
> -----------------------------------------------------------
> 
> (Updated Aug. 14, 2012, 10:46 a.m.)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Summary
> -------
> 
> See bug report. For this dialplan, faxing to 's' doesn't work, but faxing to 'fax' does. The patch fixes so it works. But I'm not familiar enough with T38 nor the indications to know if this is the right fix.
> 
> [default]
> exten => s,1,Dial(Local/fax at default)
> 
> exten => fax,1,NoOp()
>      same => n,ReceiveFAX(/tmp/abc.tiff)
>      same => n,Hangup()
> 
> 
> This addresses bug ASTERISK-20229.
>     https://issues.asterisk.org/jira/browse/ASTERISK-20229
> 
> 
> Diffs
> -----
> 
>   /branches/1.8/channels/chan_local.c 371243 
> 
> Diff: https://reviewboard.asterisk.org/r/2070/diff
> 
> 
> Testing
> -------
> 
> On 1.6.2.x faxing works without the patch.
> On 1.8.x faxing works on the 's' extension only after applying the patch.
> 
> I used the patched sipp from https://code.osso.nl/projects/sipp with the sendfax.xml and sendfax.pcap to replay a (lengthy) fax.
> 
> 
> Thanks,
> 
> wdoekes
> 
>

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