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

usrloc: call contact expired call back for a domain with db_mode: DB_ONLY #1683

Merged
merged 3 commits into from Oct 30, 2018

Conversation

adil-mafzool
Copy link
Contributor

@adil-mafzool adil-mafzool commented Oct 22, 2018

usrloc: call contact expired call back for a domain with db_mode: DB_ONLY; database rows are removed by the timer function: db_timer_udomain

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)
  • [X ] New feature (non-breaking change which adds new functionality)
  • Breaking change (fix or feature that would change existing functionality)

Checklist:

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

Description

Currently call contact expired call back for a domain with db_mode: DB_ONLY doesn't callback function for "usrloc:contact-expired" to carry out call back application actions since database rows are removed by the timer function: db_timer_udomain. There is addition of new function udomain_contact_expired_cb which will only call the call back function when contact are being removed from the database.

@miconda
Copy link
Member

miconda commented Oct 24, 2018

Thanks for the contribution!

This will bring coherence with the other values for db_mode, so if the other devels have nothing against, it can be merged, even if we are in the freezing phase for v5.2.0.

The only adjustment I would make is to add a mod param to control the execution of the new function, because it can be a lengthly execution when there are many records in the database and this functionality is not needed always.

@henningw
Copy link
Contributor

@miconda: It is not strictly a bugfix - but as you said it brings coherence to the module with the other values for db_mode. So I have nothing against the exception to merge it now in the current phase of development. To addea module parameter is a good idea as well.

@adil-mafzool: I have one question about your patch: you create both a new urecord and a ucontact in the timer callback function, and I think both are allocated in new memory. But you only cleanup the ucontact, not the urecord after the callback processing. Is this not necessary or it is cleanup at another place?

@adil-mafzool
Copy link
Contributor Author

@miconda: Adding db_mode would be required since db_timer_udomain is now being called for different db_modes.
@henningw: Had seen that but since the concept was duplicated from ``preload_udomain``` where ucontact was being freed where as urecord was not. It looks like urecord is using static memory function get_static_urecord and doesn't use shm_malloc.

@henningw
Copy link
Contributor

About duplicating the concept from preload_udomain() - this is generally fine, but the purpose of this function is a different. The preload_udomain() loads entries during startup to keep them in the cache and expire them later during runtime. This is not used in mode DB_ONLY. If I understand your code correctly, it just load all the entries to call the expiry callback and then it is not needed anymore.

You call mem_insert_urecord(), this calls new_urecord(), and this allocates new memory with shm_malloc(). As you are running in DB_ONLY mode, you should not keep any urecord entries after the function completes according to my understanding of your patch.

I would suggest to execute your patch in Kamailio debugging mode, maybe add a bit of logging to the places I mentioned, and verify this allocation and de-allocation of the memory.

@adil-mafzool
Copy link
Contributor Author

@henningw thanks for the review. removed redundant function call mem_insert_urecord from udomain_contact_expired_cb.

@adil-mafzool
Copy link
Contributor Author

@miconda will this be backported to stable 4.4 branch?

@miconda
Copy link
Member

miconda commented Oct 29, 2018

This is not a pure fix to a bug -- I said I am fine to merge it in this phase of testing the upcoming 5.2.0 release because it brings coherence with the other db modes. With git is easy to clone the repo and maintain back ports as you need.

@adil-mafzool
Copy link
Contributor Author

@miconda thats ok!!

@henningw henningw self-assigned this Oct 30, 2018
@henningw henningw merged commit 9757bb8 into kamailio:master Oct 30, 2018
@henningw
Copy link
Contributor

Thank you, merged into master branch.

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