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

p_usrloc: new parameter "mdb_availability_control" has been created. #1529

Merged
merged 1 commit into from May 23, 2018

Conversation

hdikme
Copy link
Contributor

@hdikme hdikme commented May 14, 2018

  • The new parameter "mdb_availability_control" overwrites the "write_on_master_db" parameter based on the availability of master database.

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

@miconda miconda requested a review from lbalaceanu May 15, 2018 05:59
@henningw
Copy link
Contributor

@lbalaceanu - any comments on this patch?

Copy link
Contributor

@lbalaceanu lbalaceanu left a comment

Choose a reason for hiding this comment

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

I will continue the review tomorrow.

@@ -307,6 +309,13 @@ static int mod_init(void)
}
#endif

if((write_on_master_db_shared = shm_malloc(sizeof(dbm_write_t))) == NULL) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Make sure to shm_free this allocated memory.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The parameter will be created once at the beginning of mod_init and used so long as the process runs, therefore i haven't freed it.

@@ -123,5 +123,11 @@ extern int connection_expires;
extern int alg_location;

extern int max_loc_nr;
typedef struct dbm_write {
Copy link
Contributor

Choose a reason for hiding this comment

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

Structure name must be a noun. Start with sth like mdb_

Copy link
Contributor Author

Choose a reason for hiding this comment

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

has been changed with "db_shared_param".

@@ -96,7 +96,10 @@ void check_dbs(unsigned int ticks, void *param){
ul_db_handle_list_t * tmp2, * new_element;
int found;
int i;


if(mdb_availability_control) {
Copy link
Contributor

Choose a reason for hiding this comment

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

mdb_availability_control is also checked inside check_master_db() function.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

correct, i m removing the one inside the check_master_db() function.

@@ -149,6 +152,25 @@ void check_dbs(unsigned int ticks, void *param){
lock_release(list_lock);
}

void check_master_db(int dbm_write_default) {
if(mdb_availability_control) {
if(mdb.write.dbh){
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it necessary to close the connection each time when verifying it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is unfortunately no other function other than "db_init_f init;" that could be used to check if the master db is available. Therefore each time a new mdb handler is created from the scratch and this requires first closing the existing mdb handler.

@@ -677,6 +677,26 @@ modparam("usrloc", "db_update_as_insert", 1)
...
modparam("p_usrloc", "default_db_url", "mysql://ser:ser@localhost/ser")
...
</programlisting>
Copy link
Contributor

Choose a reason for hiding this comment

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

Since this is related to write_on_master_db, please move this documentation under the write_on_master_db parameter.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done!

@@ -102,13 +102,16 @@ int ul_db_child_init(void) {
if(ul_db_child_locnr_init() == -1) return -1;

LM_INFO("location number is %d\n", max_loc_nr);
if(db_master_write){
lock_get(&write_on_master_db_shared->lock);
Copy link
Contributor

Choose a reason for hiding this comment

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

Check whether in ul_db_init() you need to acquire locks at all. It seems like this function is called one time only, before any fork is done, so most probably no synchronisation is needed. Same for lock release.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

p_usrloc_mod.c:
The fork() system call is triggered in the init_db_check() function, which takes place just before calling the ul_db_child_init() function where the above mentioned lock checking mechanism is used.

- The new parameter "mdb_availability_control" overwrites the "write_on_db" parameter based on the availability of master database.
@lbalaceanu lbalaceanu merged commit bec6a75 into kamailio:master May 23, 2018
@lbalaceanu
Copy link
Contributor

Thank you @hdikme. Closing the pull request.

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