[Asterisk-code-review] res pjsip: Re-use IP version of signaling (SIP) for media (R... (asterisk[13])

[George Joseph]

George Joseph has posted comments on this change.

Change subject: res_pjsip: Re-use IP version of signaling (SIP) for media (RTP).
......................................................................


Patch Set 4:

> > some test failures
 > 
 > Instead of "some", please, be as specific as possible. 4 cases from
 > the automated Asterisk Test Suite failed, which prevented the
 > integration of this change here. All cases belong to the SIP
 > channel driver PJSIP and are part of the Basic-Call tests:
 > tests/channels/pjsip/basic_calls/outgoing/nominal/echo
 > tests/channels/pjsip/basic_calls/outgoing/nominal/playback
 > tests/channels/pjsip/basic_calls/two_parties/nominal/alice_initiated/alice_hang
 > tests/channels/pjsip/basic_calls/two_parties/nominal/alice_initiated/bob_hangs_up
 > 
 > In the next step, explain the contributor what is expected from
 > him. Please, go through this Wiki page to reproduce these failures
 > on your computer: <https://wiki.asterisk.org/wiki/display/AST/Installing+the+Asterisk+Test+Suite>.
 > Start with one case. When you have problems to create the
 > dependencies or are not able to reproduce the case, simply report
 > at [I have no idea].
 > 
 > > audio isn't detected on the channel
 > 
 > In those cases, ast_sip_session->contact->via_addr is not set and
 > therefore, the test for the IP version is wrong. I changed that, so
 > that the code runs into the error case, then. With that, the
 > change-set here passes all test cases. However, that would be
 > cheating, because the user would receive an LOG_ERROR. Therefore, I
 > added ast_assert(0) to reliably fail all test cases, which run into
 > the error case.
 > 
 > ast_sip_session_create_invite
 > -create_local_sdp
 > --add_sdp_streams
 > ---create_outgoing_sdp_stream
 > via_addr is only set, when a SIP-REGISTER is involved. In those
 > test cases, a SIP contact is called directly by its SIP-URI,
 > without registering. Furthermore, via_addr, ast_sip_session->pjsip_inv_session->pjsip_dialog->via_addr,
 > nor the linked list ast_sip_session->pjsip_inv_session->pjsip_dialog->inv_hdr(PJSIP_H_VIA)
 > contain an IP address already. The ast_sip_session->ast_sip_contact->uri
 > is set, but might contain a domain which is not resolved by DNS,
 > yet.
 > 
 > Consequently, my change does not catch all cases. Because I am not
 > able to find/access an IP address and I do not have a bird's eye
 > view, this is an architectural issue in res_pjsip. Anyway, please,
 > regate this change so we see whether even more test cases fail (the
 > Asterisk Test Suite does not work on my computer/setup). It is
 > going to fail, but at least that tells us the scenarios which are
 > affected. Digium can then:
 > A) remove ast_assert(0) and accept this as partial fix
 > B) abandon this
 > 
 > For me, this issue here is a blocker. From my understanding, this
 > is a blocker for all VoIP/SIP providers because they abstract the
 > IP version and do not know the IP version of their users in
 > advance. I cannot envision a scenario which can live with this
 > issue except an IPv4-only server. I do not have the resources to
 > continue with this change-set because the remaining cases are not
 > mine.

I've created an internal issue to see if we can get this patch pushed through.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I01a85a8c6723fcc12e86139f80e090e2078d04bb
Gerrit-PatchSet: 4
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: George Joseph <gjoseph at digium.com>
Gerrit-Reviewer: Joshua Colp <jcolp at digium.com>
Gerrit-Reviewer: Mark Michelson <mmichelson at digium.com>
Gerrit-HasComments: No