[asterisk-dev] [Code Review] An alternate way of accomplishing the goal of review 295

[Mark Michelson]

> On 2009-06-29 15:22:30, Kevin Fleming wrote:
> >

Kevin, do you think that this approach is superior to the method used in review 295? Just curious.


> On 2009-06-29 15:22:30, Kevin Fleming wrote:
> > /branches/1.4/channels/chan_sip.c, lines 16140-16142
> > <http://reviewboard.digium.com/r/298/diff/1/?file=5763#file5763line16140>
> >
> >     Where did these flag sets move to? Are they no longer necessary?

They were removed because they accomplished nothing. In the original version, if this case were reached, these flags would be set and then handle_request would return 0. The caller of handle_request does no further processing of the sip_request struct, so those flags did nothing. What's interesting is that there are some checks inside of handle_response which will check the SIP_PKT_IGNORE flag on the sip_request; however, it was impossible for this flag to be set and for handle_response to be called. The SIP_PKT_IGNORE_RESP flag is never checked anywhere. It may have accomplished something at some point in the lifetime of chan_sip, but now it is useless.


> On 2009-06-29 15:22:30, Kevin Fleming wrote:
> > /branches/1.4/channels/chan_sip.c, lines 13147-13157
> > <http://reviewboard.digium.com/r/298/diff/1/?file=5763#file5763line13147>
> >
> >     To reduce code duplication, I'd suggest storing the result of __sip_semi_ack/__sip_ack in a variable, and then testing and making the ignore decision.

Good idea.


- Mark


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviewboard.digium.com/r/298/#review903
-----------------------------------------------------------


On 2009-06-26 14:45:53, Mark Michelson wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.digium.com/r/298/
> -----------------------------------------------------------
> 
> (Updated 2009-06-26 14:45:53)
> 
> 
> Review request for Asterisk Developers and Kevin Fleming.
> 
> 
> Summary
> -------
> 
> In review 295 (https://reviewboard.asterisk.org/r/295), I proposed a method of solving issue 11231. Even though I already have received a "Ship it!" from a couple of people, Kevin told me about an alternate way of approaching the problem, so I coded up a new way of handling this.
> 
> Since we have to traverse the list of packets on the sip_pvt, remove the retransmission of the packet from the scheduler, and free the packet anyway, we can use that function to determine if the received response is one we are expecting. This required modifying the __sip_ack function to return a boolean instead of returning void.
> 
> This approach requires no code additions, but requires rearrangement of existing code. While making this change, I noticed that the "ignore" parameter in handle_response could never be TRUE and that it wasn't even used anyway, so I removed it. This change also made the lastnoninvite field on a sip_pvt useless, so I removed it as well.
> 
> Let me know if you think this is a better approach than what was proposed in review 295. The only thing that worries me is that this may be a bit harder to merge to other branches than the 295 method. Thanks!
> 
> 
> This addresses bug 11231.
>     https://issues.asterisk.org/view.php?id=11231
> 
> 
> Diffs
> -----
> 
>   /branches/1.4/channels/chan_sip.c 202966 
> 
> Diff: http://reviewboard.digium.com/r/298/diff
> 
> 
> Testing
> -------
> 
> I performed the same test as per review 295.
> 
> 
> Thanks,
> 
> Mark
> 
>