[kamailio/kamailio] New record-route is not updating when the device network is broken/power-off & on (sca_subscribe.c). (PR #3569)
After initial subscription by the SCA device, if we power off and on again, we won't get an unsubscribe notification and we are getting a new record-route with sca-subscription. In this case kamailio not updating the new record-route for the SCA Device.
<!-- 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 --> - [ ] Commit message has the format required by CONTRIBUTING guide - [ *] Commits are split per component (core, individual modules, libs, utils, ...) - [ *] Each component has a single commit (if not, squash them into one commit) - [ *] 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 - [* ] 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 --> - [ ] PR should be backported to stable branches - [ *] Tested changes locally - [ ] Related to issue #XXXX (replace XXXX with an open issue number)
#### Description <!-- Describe your changes in detail --> When we initially subscribe to the SCA (Shared call appearance) device, we receive a new subscription notification. However, if we subsequently power off and then power on the SCA device, we will no longer receive an unsubscribe notification; instead, we are receiving a new subscription notification with a new record-route. In this scenario, kamailio is not updating the new record-route and sca notification is working after the device is powered on. In two different scenario this issue is occuring(1.Device power off & on 2. Device network broken).
You can view, comment on, or merge this pull request online at:
https://github.com/kamailio/kamailio/pull/3569
-- Commit Summary --
* Update sca_subscribe.c
-- File Changes --
M src/modules/sca/sca_subscribe.c (4)
-- Patch Links --
https://github.com/kamailio/kamailio/pull/3569.patch https://github.com/kamailio/kamailio/pull/3569.diff
Thanks for the PR!
A few remarks though:
- the commit has to be for development version (git master branch), not 5.5 branch. It will be then backported if it is a bug. - 5.5 series is actually end of maintenance (its last release was out a while ago: https://www.kamailio.org/w/2023/07/kamailio-v5-5-7-released/) - you have to format the commit message as per contributing guidelines in order to facilitate merging from the web interface, see: - https://github.com/kamailio/kamailio/blob/master/.github/CONTRIBUTING.md#com...
In short, you have to do a new PR for the git master branch.
@linuxmaniac: any comment related to the change proposed here? Patch looks pretty minimal.
@miconda I think the fix is not really good since is not taking care of memory and if the previous value is equal. Created a new PR with a possible solution. @schiramchetty please take a look and test it
@miconda I think the fix is not really good since is not taking care of memory and if the previous value is equal. Created a new PR with a possible solution. @schiramchetty please take a look and test it
Hi @linuxmaniac, Thanks for your feedback. We are not allocating any new memory for the requested PR and we are just updating the existing variable value using
SCA_STR_COPY(&saved_sub->rr, &update_sub->rr);
Also we have tested with this PR and we are not finding any memory-related issues and all the scenarios are working as expected. Please let us know do we need to create a new PR as proposed by @miconda. or do you need any clarifications on this PR. Please suggest.
Hi, what is going to happen if the length of updated RR is bigger than the reserved memory at saved RR? And in the case of RR being the same value?
Hi, what is going to happen if the length of updated RR is bigger than the reserved memory at saved RR? And in the case of RR being the same value?
Hi @linuxmaniac, The updated RR and saved RR variables are updated by kamailio itself when receiving any sca subscription message. This is already handled by kamailio existing source code, also both variables are using the same "_sca_subscription" structure. We are only copying values between these variables. Hope it will clarify.
@schiramchetty
when you say you're only copying ... are you assuming that the length is always the same?
``` #define SCA_STR_COPY(str1, str2) \ memcpy((str1)->s, (str2)->s, (str2)->len); \ (str1)->len = (str2)->len; ```
SCA_STR_COPY doesn't do any magic. You need to check that you have enough memory reserved for that. And on top of that, why should I copy a value that is exactly the same? So before copying we need to check the value is different.
@schiramchetty
When you say you're only copying ... are you assuming that the length is always the same?
#define SCA_STR_COPY(str1, str2) \ memcpy((str1)->s, (str2)->s, (str2)->len); \ (str1)->len = (str2)->len;SCA_STR_COPY doesn't do any magic. You need to check that you have enough memory reserved for that. And on top of that, why should I copy a value that is exactly the same? So before copying we need to check the value is different.
Hi @linuxmaniac, Please refer to line number 907 in sca_subscribe.c file, The existing code used to copy the RR in the below line ...
SCA_STR_COPY(&update_sub->rr, &saved_sub->rr);
The same code we are used to update the new RR. Please note that in our scenario the updated RR and saved RR are different. Still, if you feel the code needs validation can you please add the validation code here. So that I will be able to raise the PR with the modified code.
I already created the code at https://github.com/kamailio/kamailio/pull/3571 Please test it
I already created the code at #3571 Please test it
Thanks for the update, We will test this code and will get back to you.
Closing this one, further discussions should be done to:
- https://github.com/kamailio/kamailio/pull/3571
Closed #3569.
-
Daniel-Constantin Mierla -
schiramchetty
-
Victor Seva