<!-- 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, ...) - [ ] 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) - [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 -->
You can view, comment on, or merge this pull request online at:
https://github.com/kamailio/kamailio/pull/4166
-- Commit Summary --
* ims_registrar_scscf: fix reg_fetch_contacts call * ims_registrar_scscf: fix documentation
-- File Changes --
M src/modules/ims_registrar_scscf/doc/ims_registrar_scscf_admin.xml (6) M src/modules/ims_registrar_scscf/regpv.c (25)
-- Patch Links --
https://github.com/kamailio/kamailio/pull/4166.patch https://github.com/kamailio/kamailio/pull/4166.diff
@lyatanski pushed 1 commit.
526011898ba2d2d617b6b6ada1f040d24a9fdc15 ims_registrar_scscf: fix reg_fetch_contacts call
lyatanski left a comment (kamailio/kamailio#4166)
Hi, any updates or recommendations for improvement?
miconda left a comment (kamailio/kamailio#4166)
I am not using the module, if no other comments soon, it can be merged.
henningw left a comment (kamailio/kamailio#4166)
Thanks for the PR. Interesting that the previous name of the PV in the docs was "imsulc" - even if the code defines it otherwise. But as the module defines also other existing functions (save, lookup..) from main registration modules, it can be not used together with them. Could you maybe fix the commit message by adding the module name as prefix? Then it can be merged.
lyatanski left a comment (kamailio/kamailio#4166)
Hi and thank you for the feedback.
I believe the commit message alredy has the module name as prefix. The merge request name seems to have replaced undersvores with spaces and if this is what you mean - I'll try to update it
@henningw commented on this pull request.
Thanks for the feedback. Regarding the commit message, I misread it, sorry. I looked to the code, and add a few remarks. Please have a look, thank you.
@@ -440,7 +451,15 @@ int pv_fetch_contacts(
return -1; }
- ptr = 0; //r->contacts;TODO + rpp->impi.s = (char *)pkg_malloc(r->private_identity.len * sizeof(char)); + if(rpp->impi.s == NULL) { + LM_ERR("no mem for impi\n"); + return -1;
Do we need to free the earlier allocated memory here? Maybe also just jump to error?
@@ -455,8 +474,8 @@ int pv_fetch_contacts(
goto error; } memcpy(c0, ptr, ilen); - //c0->domain = {0,0};//NULL;TODO - //c0->aor = {0,0};//NULL; + c0->domain = (str){NULL, 0};
I don't think you need the casts here, it would be probably more consistent to use the {0, 0} as elsewhere in this function.
@@ -440,7 +451,15 @@ int pv_fetch_contacts(
return -1; }
- ptr = 0; //r->contacts;TODO + rpp->impi.s = (char *)pkg_malloc(r->private_identity.len * sizeof(char)); + if(rpp->impi.s == NULL) { + LM_ERR("no mem for impi\n"); + return -1;
I think a similar issue is probably in line 439?
@lyatanski commented on this pull request.
@@ -440,7 +451,15 @@ int pv_fetch_contacts(
return -1; }
- ptr = 0; //r->contacts;TODO + rpp->impi.s = (char *)pkg_malloc(r->private_identity.len * sizeof(char)); + if(rpp->impi.s == NULL) { + LM_ERR("no mem for impi\n"); + return -1;
yes, I have done it using AOR as example. I'm not sure if it is going to be a problem as in the config script we should have `reg_free_contacts("caller");` and even in failure case it should free the memory.
Anyway, if you recommend to use `goto error;` instead, I have no objections and I'll change it in both places. Please confirm.
@lyatanski commented on this pull request.
@@ -455,8 +474,8 @@ int pv_fetch_contacts(
goto error; } memcpy(c0, ptr, ilen); - //c0->domain = {0,0};//NULL;TODO - //c0->aor = {0,0};//NULL; + c0->domain = (str){NULL, 0};
there is compilation error without it. I assume this is the reason it was commented out and although it is not being used, it would probably be better to be initialized/zerroed.
I doubt it is related to particular compiler version, but with `gcc (Alpine 14.2.0) 14.2.0` I get:
``` Building C object src/modules/ims_registrar_scscf/CMakeFiles/ims_registrar_scscf.dir/regpv.c.o FAILED: src/modules/ims_registrar_scscf/CMakeFiles/ims_registrar_scscf.dir/regpv.c.o /usr/bin/cc -DADAPTIVE_WAIT -DADAPTIVE_WAIT_LOOPS=1024 -DARCH="x86_64" -DCC_GCC_LIKE_ASM -DCFG_DIR="/usr/local/etc/kamailio/" -DCOMPILER=""gcc 14.2.0"" -DDBG_SR_MEMORY -DDISABLE_NAGLE -DDNS_IP_HACK -DFAST_LOCK -DFMSTATS -DF_MALLOC -DHAVE_ALLOCA_H -DHAVE_EPOLL -DHAVE_GETHOSTBYNAME2 -DHAVE_IP_MREQN -DHAVE_MSGHDR_MSG_CONTROL -DHAVE_MSG_NOSIGNAL -DHAVE_RESOLV_RES -DHAVE_SCHED_SETSCHEDULER -DHAVE_SCHED_YIELD -DHAVE_SELECT -DHAVE_SIGIO_RT -DHAVE_TIMEGM -DHAVE_UNION_SEMUN -DKMSTATS -DKSR_PTHREAD_MUTEX_SHARED -DMALLOC_STATS -DMOD_NAME="ims_registrar_scscf" -DMOD_NAMEID=ims_registrar_scscf -DNAME="kamailio" -DOS=Linux -DOS_QUOTED="Linux" -DPKG_MALLOC -DQ_MALLOC -DRAW_SOCKS -DRUN_DIR="/run/kamailio" -DSHARE_DIR="/usr/local/share/kamailio/" -DSHM_MMAP -DSIGINFO64_WORKAROUND -DSTATISTICS -DTLSF_MALLOC -DTLS_HOOKS -DUSE_DNS_CACHE -DUSE_DNS_FAILOVER -DUSE_DST_BLOCKLIST -DUSE_MCAST -DUSE_NAPTR -DUSE_RAW_SOCKS -DUSE_SCTP -DUSE_TCP -DUSE_TLS -DVERSION="6.1.0-dev0" -DVERSIONVAL=6001000 -D__CPU_x86_64 -D__OS_linux -Dims_registrar_scscf_EXPORTS -isystem /usr/include/libxml2 -O3 -DNDEBUG -std=gnu11 -fPIC -ffile-prefix-map=/code/src/modules/ims_registrar_scscf/= -fPIC -Wall -funroll-loops -Wcast-align -Werror=implicit-function-declaration -Werror=implicit-int -m64 -minline-all-stringops -falign-loops -ftree-vectorize -fno-strict-overflow -mtune=generic -MD -MT src/modules/ims_registrar_scscf/CMakeFiles/ims_registrar_scscf.dir/regpv.c.o -MF src/modules/ims_registrar_scscf/CMakeFiles/ims_registrar_scscf.dir/regpv.c.o.d -o src/modules/ims_registrar_scscf/CMakeFiles/ims_registrar_scscf.dir/regpv.c.o -c /code/src/modules/ims_registrar_scscf/regpv.c /code/src/modules/ims_registrar_scscf/regpv.c: In function 'pv_fetch_contacts': /code/src/modules/ims_registrar_scscf/regpv.c:477:30: error: expected expression before '{' token 477 | c0->domain = {NULL, 0}; | ^ ninja: build stopped: subcommand failed. ```
lyatanski left a comment (kamailio/kamailio#4166)
Hi @henningw,
did you have the chance to check out my comments and confirm the direction we should go for error handling
@henningw commented on this pull request.
@@ -440,7 +451,15 @@ int pv_fetch_contacts(
return -1; }
- ptr = 0; //r->contacts;TODO + rpp->impi.s = (char *)pkg_malloc(r->private_identity.len * sizeof(char)); + if(rpp->impi.s == NULL) { + LM_ERR("no mem for impi\n"); + return -1;
My expection would be that if a script function fails, there is no need to cleanup necessary in the kamailio cfg. So we should do a proper cleanup here and also in line 439. If you compare to the standard registrar module function, it also just uses the "goto error" logic.
henningw left a comment (kamailio/kamailio#4166)
Hi @henningw,
did you have the chance to check out my comments and confirm the direction we should go for error handling
Thanks for the reminder, I have added a comment.
@lyatanski pushed 1 commit.
b83907b535c5dea503595f7758d5925870ca7a32 ims_registrar_scscf: fix reg_fetch_contacts call
@henningw commented on this pull request.
@@ -423,7 +434,7 @@ int pv_fetch_contacts(
rpp->aor.s = (char *)pkg_malloc(aor.len * sizeof(char)); if(rpp->aor.s == NULL) { LM_ERR("no more pkg\n"); - return -1; + goto error;
Sorry to be annoying, but I would prefer to keep the -1 here, as we are not having any memory allocated yet, and to also stay in sync with the main registrar module implementation
@lyatanski pushed 1 commit.
0abc33e81d6cb76d71cbae61a48a33121d3e25cb ims_registrar_scscf: fix reg_fetch_contacts call
@lyatanski commented on this pull request.
@@ -423,7 +434,7 @@ int pv_fetch_contacts(
rpp->aor.s = (char *)pkg_malloc(aor.len * sizeof(char)); if(rpp->aor.s == NULL) { LM_ERR("no more pkg\n"); - return -1; + goto error;
done, returned it back to just exiting, but changed 451 line to `goto error;` as there we have allocated memory for the AOR and have to clean it up before exiting
lyatanski left a comment (kamailio/kamailio#4166)
the changes have been pushed and the PR branch have been rebased on the master as there were failing checks, now they seem to be passing
Merged #4166 into master.
@lyatanski commented on this pull request.
@@ -440,7 +451,15 @@ int pv_fetch_contacts(
return -1; }
- ptr = 0; //r->contacts;TODO + rpp->impi.s = (char *)pkg_malloc(r->private_identity.len * sizeof(char)); + if(rpp->impi.s == NULL) { + LM_ERR("no mem for impi\n"); + return -1;
done
-
Daniel-Constantin Mierla -
Henning Westerholt
-
Lyubomir Yatanski