Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

nathelper : added new function set_alias_to_pv #2124

Merged
merged 7 commits into from Nov 25, 2019

Conversation

ycaner06
Copy link
Contributor

@ycaner06 ycaner06 commented Nov 8, 2019

Pre-Submission Checklist

  • 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:

  • PR should be backported to stable branches
  • Tested changes locally
  • Related to issue #XXXX (replace XXXX with an open issue number)

Description

new function that read alias from Contact header then write to given avp as sip uri

root and others added 3 commits November 8, 2019 05:14
added new function proto type int to str
new function that read alias then write to given avp as sip uri
added description of set_alias_to_avp function
@ycaner06
Copy link
Contributor Author

ycaner06 commented Nov 8, 2019

This function , later , will be used for keepalive modules. I have new updates for keepalive module.

Copy link
Contributor

@henningw henningw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks you Yasin for the pull request. I've did a quick review, and have added some comments and questions to the code.

src/core/parser/parse_uri.c Outdated Show resolved Hide resolved
src/modules/nathelper/nathelper.c Outdated Show resolved Hide resolved
src/modules/nathelper/nathelper.c Outdated Show resolved Hide resolved
src/modules/nathelper/nathelper.c Outdated Show resolved Hide resolved
src/modules/nathelper/nathelper.c Outdated Show resolved Hide resolved
src/modules/nathelper/nathelper.c Outdated Show resolved Hide resolved
src/modules/nathelper/nathelper.c Outdated Show resolved Hide resolved
src/modules/nathelper/nathelper.c Show resolved Hide resolved
@ycaner06
Copy link
Contributor Author

ycaner06 commented Nov 8, 2019

I will solve problems then add a docs. Thanks for support Henning

fixed memory-leak for new function set_alias_to_avp
fixed some spelling
added return for functions write_to_avp and alias_to_uri
@ycaner06
Copy link
Contributor Author

Hello,
if it is ok for function name , i will add doc.
thanks

@miconda
Copy link
Member

miconda commented Nov 11, 2019

Looking at the resulting diff of the PR (https://patch-diff.githubusercontent.com/raw/kamailio/kamailio/pull/2124.diff), I see that the src/core/parser/parse_uri.h has a single empty line removed. I guess that is not really needed.

The the name of the C function exported to kamailio.cfg file is named ki_set_alias_to_avp, but the ki_ prefix is used for functions exported to KEMI interface. Those exported to kamailio.cfg have usually the w_ prefix. Then the prototype for kamailio.cfg functions is with three parameters (one sip_msg* and two char*), it is safer to set it like this even if the 2nd parameter is not used, because the core interpreter gives NULL as 2nd char* param when executing the function.

Then, you should export the new function to kemi interface as well, which is more or less the set_alias_to_avp_f(), eventually you name this one ki_set_alias_to_avp.

function set_alias_to_avp_f is renamed to w_set_alias_to_avp
added kemi interface
added documents for set_alias_to_avp function
@miconda
Copy link
Member

miconda commented Nov 18, 2019

Looking once again at the code, the function can set the value in any (pseduo-)variable, not only in AVP, right? Because it is using pv->setf() function, not the AVP specific functions.

Do you want to restrict to writing to AVP or it is fine writing to any variable?

@ycaner06
Copy link
Contributor Author

Looking once again at the code, the function can set the value in any (pseduo-)variable, not only in AVP, right? Because it is using pv->setf() function, not the AVP specific functions.

Do you want to restrict to writing to AVP or it is fine writing to any variable?

I dont want to restrict because it will be use to add keepalive module as a destination or it can be use to setting/checking/comparing other variables like $du , $ru etc.

@henningw
Copy link
Contributor

If you don't want to restrict it to avps, probably it would be good to rename the function (code and configuration file export) to "set_alias_to_pv" or something like this. I guess this was intention of Daniels question.

set_alias_to_avp renamed to set_alias_to_pv
@ycaner06 ycaner06 changed the title nathelper : added new function set_alias_to_avp nathelper : added new function set_alias_to_pv Nov 21, 2019
@henningw henningw merged commit 28cfa39 into kamailio:master Nov 25, 2019
@henningw
Copy link
Contributor

Thank you, merged. I will add 1-2 small formatting changes directly in git master. If there are more changes, they can be done directly in git master as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants