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

[Alexander Traud]

Alexander Traud has posted comments on this change.

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


Patch Set 1: Code-Review-1

(8 comments)

https://gerrit.asterisk.org/#/c/3657/1//COMMIT_MSG
Commit Message:

PS1, Line 11: no bindaddr at all
In absence of any bindaddr, the default of chan_sip is not :: but 0.0.0.0. Therefore, please, remove this part of the sentence.


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 239: if not url.port:
Currently, bindport is used only when there was no port in bindaddr. However. chan_sip overwrites port in bindaddr with bindport.


PS1, Line 625:                     bind = sip.multi_get('general',
             :                                          [protocol + 'bindaddr',
             :                                           'bindaddr'])[0]
> This lookup can be simplified. Since you already have tried looking up 'udp
Done


PS1, Line 629: ::
In absence of any bindaddr, the default of chan_sip is not :: but 0.0.0.0.


PS1, Line 631: try:
             :                     bind = pjsip.get('transport-udp6', 'bind')[0]
             :                 except LookupError:
             :                     bind = pjsip.get('transport-udp', 'bind')[0]
TLS does not reuse host:port of UDP.
TLS reuses just the host of UDP.


Line 636:         if host == '::':
> Nitpick:
> you are better off

This is the job of split_hostport. There, this is documented as a known issue (TODO), because compressing of IPv6 addresses does not happen. One approach would be report this upstream as bug because urlparse is not following RFC 5952. This does not fix the other return statement in split_hostport (which faces the same issue). Anyway, if you think this is important, please, create a new issue report (or re-open the then closed report of this one here). Then, somebody can look at this because right now I have no idea how to tackle this.


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


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 
Done

I was aware of all your rationales, however another one outweighed it for me: Although there were a lot of issues in this script, the amount of reports in Jira were rather low. Actually, I found only one report created by an external member and just one comment by another external member. In more than two years. To lower the barrier as much as possible, I pointed to an existing issue report. That way, the user does not have to go/learn/understand all the fields of the issue tracker. He can simply comment and go again. Some users might see this script as a best effort script, others might understand it as contribution of a third-party unmaintained, others might wonder about the bugs and the res_pjsip module in whole and turn around (when the Asterisk team is not able to create a working migration script, is that channel driver already out of beta), others might simply fix their pjsip.conf and proceed.

So the idea was to lower the barrier as much as possible. I went your way, to get this passed. If you like the previous state, please, say so.


-- 
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: Alexander Traud <pabstraud at compuserve.com>
Gerrit-Reviewer: Anonymous Coward #1000019
Gerrit-Reviewer: Mark Michelson <mmichelson at digium.com>
Gerrit-HasComments: Yes