#### 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 - [ ] Small bug fix (non-breaking change which fixes an issue) - [x] 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 - [x] Tested changes locally - [x] Related to PR #2162
#### Description As discussed on #2162 with this change it's possible to decide witch xavp_rcd values will be stored using the new module parameter xavp_rcd_mask
Using the same helper function on build_contact() so the behavior is consistant. Before only ruid and expires where stored when build_contact() was called.
You can view, comment on, or merge this pull request online at:
https://github.com/kamailio/kamailio/pull/2168
-- Commit Summary --
* registrar: control what values to add to xavp_rcd via xavp_rcd_mask * registrar: add documentation for xavp_rcd_mask parameter
-- File Changes --
M src/modules/registrar/doc/registrar_admin.xml (31) M src/modules/registrar/lookup.c (35) M src/modules/registrar/lookup.h (4) M src/modules/registrar/registrar.c (3) M src/modules/registrar/registrar.h (8) M src/modules/registrar/reply.c (47)
-- Patch Links --
https://github.com/kamailio/kamailio/pull/2168.patch https://github.com/kamailio/kamailio/pull/2168.diff
Overall I am fine with the PR, but because of limited time to test, I am not sure if there is same result now that the lookup of `list` and `xavp` is no longer done only once in `build_contact()`. In previous version the attributes for all contacts were done inside `build_contact()` using this single lookup. Now the lookup is done for each contact inside the helper function.
I expect is the same, based on quick thinking. If you did the tests and the result is the same in terms of what is stored in xavp and the order of values, then I am fine to merge.
@linuxmaniac i think the default should be set to the value that makes it work like in 5.3
@lazdo - I think it would be more user friendly this way. If one is concerned about the performance implication it is easy to deactivate it. Or do you have other concerns here?
@henningw just saw `Defines what values to skip when xavp_rcd is stored.` which i think its the wrong approach. imo, it should define what values to retrieve and default that to the existing ones in 5.3 branch. with this implementation, if new values are added they are retrieved by default which may increase the memory footprint and take more time to process it (shouldn't be relevant).
@lazedo We don't have a policy to keep the memory requirements completely equal during major releases. Otherwise it would be not possible to do non-trivial extensions to our code-base. All kind of extensions done in the core or modules might slighly increase memory and/or run-time for this particular operation. (That said - I don't think that is most of them are noticable or even significanly measurable.) You can easily disable this feature it in your installation if you are concerned about it.
Daniel already agreed to merge it, I also think we should merge it this way.
@henningw `You can easily disable this feature it in your installation if you are concerned about it` this is exactly what you should not ask the user to do, no ?
i'm not blocking this in any way, just concerned.
@lazedo - actually the PR saving new field in xavp is merged. I suggested at that time to add an option to control what is added, because more and more fields were wanted in the xavp and they were stored by implicitly -- that was the what every developer did before.
So all these were following the previous pattern - save implicitly. With this new PR there is an option for fine control. I think it is better to have a straight rule -- with default option: save all fields or don't save any, then this parameter comes to do selective decision on saving specific fields. The preference was to store all and have option to skip -- from my point of view could have been also the opposite: don't store any by default and specify what to store.
I think it would be a bit more confusing/unnatural to have a "default random list of stored attributes".
@miconda i'm not proposing `"default random list of stored attributes"`, i `was` proposing a known value that is compatible with 5.3 `if` it was implemented by `adding`instead of `skipping`. but as i said before, i'm not blocking this in any way, just concerned about memory footprint from one version to another as a principle, tho in this is case, it adds almost none
I am not sure if there is same result now that the lookup of `list` and `xavp` is no longer done only once in `build_contact()`. In previous version the attributes for all contacts were done inside `build_contact()` using this single lookup. Now the lookup is done for each contact inside the helper function.
I expect is the same, based on quick thinking. If you did the tests and the result is the same in terms of what is stored in xavp and the order of values, then I am fine to merge.
I'm going to do a unit for that in http://www.github.com/kamailio/kamailio-tests so We can be sure. Holding this until then.
@lazedo - I used "random" for the personal opinions/needs of the developers which added storage of the fields in the past, not being a decision done based on a discussion devs and evaluation of the impact. If resource usage was a concern, that should have been with the first attribute added, then it was also an impact on memory usage between kamailio major versions.
The approach was to add new attributes as some dev had new needs. Because the list seemed to grow over the time, it made sense to add an option to control in long term. But from consistency point of view it should be one way or the other: store all by default and allow option to skip some; or just don't add any and allow option to specify which to store.
This new parameter it doesn't change the existing behaviour of the master branch before merging the PR -- again, at this moment in core all the attributes wanted are stored implicitly. Once merging this PR, all the attributes are still stored, so it doesn't change the default behavior. Just add option to be able to skip some via modparam.
Tested that xavp_rcd has the same order with this change, for sure now it will have more values depending of the value of xavp_rcd_mask
OK, thanks, it can be merged.
Merged #2168 into master.
-
Daniel-Constantin Mierla -
Henning Westerholt
-
lazedo
-
Victor Seva