[kamailio/kamailio] topos: BYE sent by caller before call was connected was badly managed (PR #3644)
<!-- Kamailio Pull Request Template -->
<!-- IMPORTANT: - for detailed contributing guidelines, read: https://github.com/kamailio/kamailio/blob/master/.github/CONTRIBUTING.md - pull requests must be done to master branch, unless they are backports of fixes from master branch to a stable branch - backports to stable branches must be done with 'git cherry-pick -x ...' - code is contributed under BSD for core and main components (tm, sl, auth, tls) - code is contributed GPLv2 or a compatible license for the other components - GPL code is contributed with OpenSSL licensing exception -->
#### Pre-Submission Checklist <!-- Go over all points below, and after creating the PR, tick all the checkboxes that apply --> <!-- All points should be verified, otherwise, read the CONTRIBUTING guidelines from above--> <!-- If you're unsure about any of these, don't hesitate to ask on sr-dev mailing list --> - [x] Commit message has the format required by CONTRIBUTING guide - [x] Commits are split per component (core, individual modules, libs, utils, ...) - [x] Each component has a single commit (if not, squash them into one commit) - [x] No commits to README files for modules (changes must be done to docbook files in `doc/` subfolder, the README file is autogenerated)
#### Type Of Change - [x] Small bug fix (non-breaking change which fixes an issue) - [ ] New feature (non-breaking change which adds new functionality) - [ ] Breaking change (fix or feature that would change existing functionality)
#### Checklist: <!-- Go over all points below, and after creating the PR, tick the checkboxes that apply --> - [x] PR should be backported to stable branches - [x] Tested changes locally - [ ] Related to issue #XXXX (replace XXXX with an open issue number)
#### Description <!-- Describe your changes in detail --> When caller sent a BYE instead a CANCEL into a non connected call, route are badly reappend by topos. regression introduced by patch 091dc9a76bcec5c8a4bc73e863ed10b1b9d76c92 topos: fix early-dialog b-side UPDATE requests routing (GH #3437) You can view, comment on, or merge this pull request online at:
https://github.com/kamailio/kamailio/pull/3644
-- Commit Summary --
* topos: BYE sent by caller before call was connected was badly managed
-- File Changes --
M src/modules/topos/tps_msg.c (2)
-- Patch Links --
https://github.com/kamailio/kamailio/pull/3644.patch https://github.com/kamailio/kamailio/pull/3644.diff
Thanks for the PR. It is indeed valid to send a BYE instead of an CANCEL in an early-dialog. Its a small patch, lets wait for some more feedback before merging it.
can it be added in kamailio 5.7.3 ?
@fgaisnon: release process for v5.7.3 started earlier today and I didn't merge before because @henningw's comment is rather confusing for me, requesting also more time for others to review the PR.
As I understood from the description of the PR, this should be a fix for a regression introduced by commit 091dc9a76bcec5c8a4bc73e863ed10b1b9d76c92, which was done by @henningw. So handling early dialog BYEs was fine before that commit, but his comment is about validity of sending BYE instead of CANCEL in an early dialog.
BYE during early dialog has a different purpose than CANCEL, but that is not the real matter here, handling early dialog BYEs was (supposed to be) supported for long time by topos. Considering that I thought that @henningw's comment is unrelated to the commit of the PR and since it was opened yesterday I haven't had the time to review again the previous related commit and what this PR brings to it, combined with the fact that I had no easy option to test early dialog BYEs, I skipped it for 5.7.3.
Probably it will be merged soon if all ok with it and then backported, afterwards you can use nightly builds of 5.7 series if you rely on Debian packages, till 5.7.4 is out.
@miconda As I noticed that you are working on some other topos fixes right now, I don't wanted to simply merge it and cause problems with your work in progress fixes. Sorry if this caused confusion, I'll merge it of course as I was the one caused the regression.
Merged #3644 into master.
Thanks, merged
Thanks
@henningw: I didn't blame about introducing regressions, probably I added many more than others, my remarks were that your comments were not on point whether the PR changes seem good or not, but rather confusing: just that commit is small and that BYE can be sent instead of CANCEL. None of them giving clear indication of proper review was done and combined with suggesting to wait for feedback from others, holding the merge seemed the appropriate decision before the 5.7.3 release.
-
Daniel-Constantin Mierla -
Frédéric Gaisnon
-
Henning Westerholt