<!-- 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 - [ ] 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 - [ ] Related to issue #XXXX (replace XXXX with an open issue number)
#### Description <!-- Describe your changes in detail -->
Hello. I want to ask if increasing this #define(s) are feasible to commit upstream? We've come to the case where need at least +16 more for acc module and at least +8 more for core branches. Not sure if other people would need this. 1. If yes, let me know if i should split it in 2 commits, one for core and one for acc module 2. If no, please close this PR
Thank you, Stefan You can view, comment on, or merge this pull request online at:
https://github.com/kamailio/kamailio/pull/2891
-- Commit Summary --
* <a href="https://github.com/kamailio/kamailio/pull/2891/commits/378cfd7d02243544807aca05f61cfe69bc0d812e">core, acc: increase define values</a>
-- File Changes --
M src/core/config.h (2) M src/modules/acc/acc_api.h (2) M src/modules/acc/acc_cdr.h (2)
-- Patch Links --
https://github.com/kamailio/kamailio/pull/2891.patch https://github.com/kamailio/kamailio/pull/2891.diff
For the number of branches, there is a global parameter to set it without changing default defines, see:
* https://www.kamailio.org/wiki/cookbooks/5.5.x/core#max_branches
At least that part of the patch is not needed.
Then, for acc, maybe it would be better to make them modparam (e.g., acc_extra_size, cdr_extra_size) and allocate the arrays in mod init.
Thanks for the feedback! I will update this PR soon, only for acc module, after some testing.
@smititelu pushed 1 commit.
dbb3d548c852a5e075672e3d8af042bfb19739d4 acc: increase acc and cdr extra values
Updated PR. Tests look ok.
Thank you, Stefan
@smititelu pushed 1 commit.
b90f181a8963744d39abc96bc5bf9ab81dc0d6f1 acc: increase acc and cdr extra values
Added extern declaration in .h files and definition in respective .c files (instead of static arrays); sid gcc build passes now.
@miconda commented on this pull request.
@@ -61,9 +60,9 @@ extern struct acc_extra *db_extra;
/* arrays used to collect the values before being * pushed to the storage backend (whatever used) * (3 = datetime + max 2 from time_mode) */ -static str val_arr[ACC_CORE_LEN+MAX_ACC_EXTRA+MAX_ACC_LEG+3]; -static int int_arr[ACC_CORE_LEN+MAX_ACC_EXTRA+MAX_ACC_LEG+3]; -static char type_arr[ACC_CORE_LEN+MAX_ACC_EXTRA+MAX_ACC_LEG+3]; +str *val_arr; +int *int_arr; +char *type_arr;
These pointers must be initialized to `NULL` so mod_destroy() does not end up crashing if mod_init() is not executed.
@miconda commented on this pull request.
+ if ((type_arr = pkg_malloc((ACC_CORE_LEN + acc_extra_size + MAX_ACC_LEG + 3) * sizeof(char))) == NULL) { + LM_ERR("failed to alloc type_arr\n"); + } + + if ((log_attrs = pkg_malloc((ACC_CORE_LEN + acc_extra_size + MAX_ACC_LEG + 3) * sizeof(str))) == NULL) { + LM_ERR("failed to alloc log_attrs\n"); + } + + if ((db_keys = pkg_malloc((ACC_CORE_LEN + acc_extra_size + MAX_ACC_LEG + 3) * sizeof(db_key_t))) == NULL) { + LM_ERR("failed to alloc db_keys\n"); + } + + if ((db_vals = pkg_malloc((ACC_CORE_LEN + acc_extra_size + MAX_ACC_LEG + 3) * sizeof(db_val_t))) == NULL) { + LM_ERR("failed to alloc db_vals\n"); + }
I think it is better to fail completely and don't start if malloc fails, the module will crash or not work, so makes no sense to continue. Return -1 in such cases.
Actually, I think it is better that the new pointer variables are still declared `static` in the `.c` files needing them, with a function that initializes them (for use in mod_init()) and another one that destroys them (in mod_destroy(). Practically copy the memory allocation/free code to their own functions in the files declaring the static pointers and expose them in `.h` files use from main file of the module.
Otherwise, some variables have pretty generic name (e.g., db_keys) and can confuse some linkers and can mess up references at runtime.
@smititelu pushed 1 commit.
e3507be9769215976defda2c39625138158bdc8e acc: increase acc and cdr extra values
@smititelu pushed 1 commit.
d30d7e19e57f3b9645da7235389ea43f823d5562 acc: increase acc and cdr extra values
Thanks for the feedback! Updated the PR accordingly. Basic tested it and is ok.
Thanks, merging!
Merged #2891 into master.
-
Daniel-Constantin Mierla -
Stefan Mititelu