[asterisk-dev] [Code Review]: Initialize udptl only when dialog negotiates for image media

[Matt Jordan]

> On Jan. 16, 2012, 9:58 a.m., Mark Michelson wrote:
> > /branches/1.8/channels/chan_sip.c, lines 6758-6764
> > <https://reviewboard.asterisk.org/r/1668/diff/1/?file=23169#file23169line6758>
> >
> >     You should do a p->owner check before you allocate the udptl structure. The way it's written now, if p->owner is NULL, the UDPTL structure will be allocated but will not be used for the lifetime of p. If you detect that an owner is not present immediately, then you can exit early without having to allocate the udptl.
> >     
> >     If you do end up allocating a udptl, then you can just unconditionally set the channel fd on p->owner.

Will do


> On Jan. 16, 2012, 9:58 a.m., Mark Michelson wrote:
> > /branches/1.8/channels/chan_sip.c, lines 7088-7090
> > <https://reviewboard.asterisk.org/r/1668/diff/1/?file=23169#file23169line7088>
> >
> >     You can remove this if block. Since a udptl struct should not exist prior to channel creation, it should not be possible for i->udptl to be true.
> >     
> >     You may have been thinking of a case where some idiotic endpoint decides to indicate its support for T.38 in the initial INVITE it sends to Asterisk. The way you have written initialize_udptl(), if this occurs, then the sip_pvt will have a udptl struct allocated, but the sip_pvt's flag for UDPTL support will be cleared. The result is that any further attempts to change to T.38 will be rejected. And so it still doesn't make sense to set the channel fd in this block.

I thought about that and left it for two reasons:
1. Failing to set the channel file descriptor is very, very hard to debug.  It took me a long time to figure out that this was why my patch was failing - T38 negotiation will still work, but frames passed from the res_fax layer will die in channel.c and never make it up to chan_sip / udptl (as there's no file descriptor triggering the underlying read).  I considered this here to be defensive coding, in case the order of when udptl is created ever changes (again).
2. If we've already assigned a udptl file descriptor to the channel, this will simply replace the existing epoll data structure with a new one.  Since there's only one udptl structure per dialog per channel, we don't run the risk of orphaning anything here - that is, setting the fd twice shouldn't have any negative effects.

That being said, with the code as it is now, this should be okay to remove.


> On Jan. 16, 2012, 9:58 a.m., Mark Michelson wrote:
> > /branches/1.8/channels/chan_sip.c, line 6743
> > <https://reviewboard.asterisk.org/r/1668/diff/1/?file=23169#file23169line6743>
> >
> >     Add some doxygen to this function indicating that p->owner (if it exists) is expected to be locked before calling this function.

I'll update that with the previous doxygen finding.


- Matt


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


On Jan. 16, 2012, 9:09 a.m., Matt Jordan wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/1668/
> -----------------------------------------------------------
> 
> (Updated Jan. 16, 2012, 9:09 a.m.)
> 
> 
> Review request for Asterisk Developers, Joshua Colp, Mark Michelson, Terry Wilson, and schmidts.
> 
> 
> Summary
> -------
> 
> Currently, the udptl object is allocated in one of three places:
> 1) create_addr_from_peer
> 2) sip_alloc
> 3) handle_request_invite
> 
> This typically results in any dialog associated with a peer that supports T.38 having a udptl object (and its associated UDP ports, system resources, etc.) assigned to it.  This includes non-INVITE requests.
> 
> This patch moves the initialization of a dialog's udptl object to either (a) when the media is negotiated for that dialog, or (b) if a control frame is received in chan_sip indicating that the dialog needs to support T.38.  This should result in only those dialogs that are explicitly involved with sending / receiving image media having a udptl object assigned to them.
> 
> 
> This addresses bugs ASTERISK-16698 and ASTERISK-16794.
>     https://issues.asterisk.org/jira/browse/ASTERISK-16698
>     https://issues.asterisk.org/jira/browse/ASTERISK-16794
> 
> 
> Diffs
> -----
> 
>   /branches/1.8/channels/chan_sip.c 350786 
> 
> Diff: https://reviewboard.asterisk.org/r/1668/diff
> 
> 
> Testing
> -------
> 
> Patch passes the udptl tests in the Asterisk Test Suite.  
> 
> [update] - ported the patch to Asterisk 10 and tested with the Test Suite's gateway tests.  All tests passed.
> 
> However, given the complexity and number of ways in which media can be negotiated, additional testing would be appreciated from the open source community.
> 
> 
> Thanks,
> 
> Matt
> 
>

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