[asterisk-dev] [Code Review] [bug] Asterisk does not choose same SRV record when attempting to re-register

[Mark Michelson]

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


As a caveat to everyone, this is a section of code that I'm not intimately familiar with, so if I'm wrong in some comments, please let me know. As far as I can tell, though, I'm right.

I would really appreciate it if another developer could take a look to be certain that my analysis isn't dead wrong.


/trunk/channels/chan_sip.c
<http://reviewboard.digium.com/r/124/#comment680>

    The alignment of the comment here is off.



/trunk/channels/chan_sip.c
<http://reviewboard.digium.com/r/124/#comment681>

    A comment here would have been nice. It *looks* like the idea here is to force the registration timeout code to run immediately (given that we add a scheduler entry to run sip_reg_timeout 1 ms from now). It seems like there should be a more straightforward way of doing this, like just calling sip_reg_timeout directly.
    
    Of course, if I have misinterpreted this odd bit of code, then changing it to call sip_reg_timeout directly would be incorrect.



/trunk/channels/chan_sip.c
<http://reviewboard.digium.com/r/124/#comment682>

    There's no need to scope this code like this, and with a comment starting with the word "if," it makes things more confusing. I would suggest moving the transport variable to the top of the function and that way it doesn't have to be declared within two separate inner scopes.
    
    (Note: I am writing this next paragraph after looking at the srv handling code for the first time, so I may have missed something. Please correct me if I am wrong)
    
    More importantly, it seems like the way the code is written here, we will always end up picking the same srv record (unless of course the DNS indicates that SRV records have changed) as we did previously instead of falling to a lower weight one. While this works well in trying to solve this specific issue, it seems like it will be problematic if we really need to fail over to a lower weight record.


- Mark


On 2009-01-09 09:05:07, Mark Michelson wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.digium.com/r/124/
> -----------------------------------------------------------
> 
> (Updated 2009-01-09 09:05:07)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Summary
> -------
> 
> This is the patch that is attached to issue 12312 (http://bugs.digium.com/view.php?id=12312). For more details, see that issue.
> 
> 
> This addresses bug 12312.
>     http://bugs.digium.com/view.php?id=12312
> 
> 
> Diffs
> -----
> 
>   /trunk/channels/chan_sip.c 168013 
> 
> Diff: http://reviewboard.digium.com/r/124/diff
> 
> 
> Testing
> -------
> 
> There's no explicit mention of testing on the bugnotes of issue 12312, but that doesn't mean that someone hasn't silently tested this.
> 
> 
> Thanks,
> 
> Mark
> 
>