-
Notifications
You must be signed in to change notification settings - Fork 84
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Moved 180 Ringing response from core to app (menu module) #1167
Conversation
if (!sessp || !sock || !msg || scode < 101 || scode > 299 || | ||
!cuser || !ctype) | ||
if (!sessp || !sock || !msg || (scode != 0 && scode < 100) || | ||
scode > 299 || !cuser || !ctype) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure with this. Why not directly use sipsess_alloc()
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll investigate.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sipsess_alloc()
is not in re API.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@cspiel1 What is wrong with sipsess_accept()
? In addition to sipsess_alloc()
it does lots other stuff that would also need to be handled separately:
err = sip_dialog_accept(&sess->dlg, msg);
if (err)
goto out;
hash_append(sock->ht_sess,
hash_joaat_str(sip_dialog_callid(sess->dlg)),
&sess->he, sess);
sess->msg = mem_ref((void *)msg);
err = sip_strans_alloc(&sess->st, sess->sip, msg, cancel_handler,
sess);
So I would prefer to keep the PR as it currently is. I have made tests and they all have worked fine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Christian Spielberger writes:
In my opinion `sipsess_accept()` should send a reply. @alfredh and @sreimers please review and decide!
If so, we can sent 100 Trying like it was.
Should we revert this, when we have another solution?
It is OK to me to revert this IF the other solution fixes the bug, i.e.,
does not by default send 180 Ringing response.
|
The 100 Trying response was not accepted before. See response of Alfred:
|
Maximilian Fridrich writes:
The 100 Trying response was not accepted before. See response of Alfred:
> please read the spec carefully regarding 100 usage and 101-199 replies:
> https://datatracker.ietf.org/doc/html/rfc3261#section-21.1.1
> 100 Trying should only be sent by a SIP Proxy...
#808 (comment)
RFC text uses term "next hop server" that very well can be an UAS = User
Agent Server. See for example this Cisco example call flow:
https://community.cisco.com/t5/collaboration-knowledge-base/basic-sip-call-flows-amp-troubleshooting-commands/ta-p/3110162
But this PR does not currently send a 100 Trying response.
Please commit this PR or provide another one that does not by default
send 180 Ringing response to INVITE request.
|
@juha-h Can we close this? |
Yes, forgot about the re PR when I closed the baresip PR. |
Required by baresip/baresip#3088