[Asterisk-code-review] sip to pjsip: Migrate IPv4/IPv6 (Dual Stack) configurations. (asterisk[13])

[Mark Michelson]

Mark Michelson has posted comments on this change.

Change subject: sip_to_pjsip: Migrate IPv4/IPv6 (Dual Stack) configurations.
......................................................................


Patch Set 1: Code-Review-1

(4 comments)

https://gerrit.asterisk.org/#/c/3657/1/contrib/scripts/sip_to_pjsip/sip_to_pjsip.py
File contrib/scripts/sip_to_pjsip/sip_to_pjsip.py:

PS1, Line 625:                     bind = sip.multi_get('general',
             :                                          [protocol + 'bindaddr',
             :                                           'bindaddr'])[0]
This lookup can be simplified. Since you already have tried looking up 'udpbindaddr' and that threw a LookupError, you can just look up:

    bind = sip.get('general', 'bindaddr')[0]


Line 636:         if host == '::':
Nitpick:

What happens if someone specified the IPv6 wildcard address in a different way? What if they had:

    bindaddr = ::0

As unlikely as it is for someone to do this, you're better off parsing IP addresses into some sort of structure and then checking that structure to see if they have bound to the wildcard address than by performing a string comparison of the address.


Line 639:     host = build_host(sip, bind, 'general', 'bindport')
bindport in sip.conf is only used for UDP. Imagine the following sip.conf

    [general]
    bindaddr = 1.2.3.4
    bindport = 6789
    tcpbindaddr = 1.2.3.4

The way chan_sip works now is that it will create a UDP transport bound to address 1.2.3.4 and port 6789. It will create a TCP transport bound to address 1.2.3.4 and port 5060.

With this line, when creating the TCP transport in pjsip.conf, you will end up binding the TCP transport to address 1.2.3.4 and port 6789.

You need to remove the 'bindport' parameter from build_host when the protocol is not UDP.


PS1, Line 1192:    print 'Please, report any issue at:'
              :     print '    https://issues.asterisk.org/jira/browse/ASTERISK-22374'
I like the idea of giving a link to report issues, but please just link to issues.asterisk.org instead of a specific issue. This way, different issues that people have can be handled separately, and there is no danger of a single issue growing too large.


-- 
To view, visit https://gerrit.asterisk.org/3657
To unsubscribe, visit https://gerrit.asterisk.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia4799a0f80fc30c0550fc373efc207c3330aeb48
Gerrit-PatchSet: 1
Gerrit-Project: asterisk
Gerrit-Branch: 13
Gerrit-Owner: Alexander Traud <pabstraud at compuserve.com>
Gerrit-Reviewer: Anonymous Coward #1000019
Gerrit-Reviewer: Mark Michelson <mmichelson at digium.com>
Gerrit-HasComments: Yes