<p></p>
<p><b>@henningw</b> requested changes on this pull request.</p>
<p dir="auto">Thank you for the PR. I've did longer review today and added several comments.</p>
<p dir="auto">Many comments are rather small, like removing some commented out code or review and adapt the log levels (which probably are set higher from development). The documentation was already mentioned from Carsten. I've added also some suggestions for naming of parameter.</p>
<p dir="auto">There is probably one condition regarding memory allocation which needs to be fixed.</p>
<p dir="auto">The major issue is that you use the DB API incorrect. You should not use raw queries to execute statements on the database. Instead the appropriate select and delete functions should be used. I've added some notes also with pointers to examples.</p>
<p dir="auto">Just let me know if there are any questions.</p><hr>
<p>In <a href="https://github.com/kamailio/kamailio/pull/3279#discussion_r1018358394">src/modules/ims_usrloc_pcscf/README</a>:</p>
<pre style='color:#555'>> @@ -171,6 +171,17 @@ modparam("ims_usrloc_pcscf", "db_url",
reflected in the database. This is slow but very reliable. This
mode will ensure that no registration data is lost as a result of a
restart or crash.
+ * 3 - DB_ONLY Scheme. All changes to usrloc are immediately
</pre>
<p dir="auto">As mentioned, this should be done in the XML files in the doc sub-directory.</p>
<hr>
<p>In <a href="https://github.com/kamailio/kamailio/pull/3279#discussion_r1018359018">src/modules/ims_usrloc_pcscf/ims_usrloc_pcscf_mod.c</a>:</p>
<pre style='color:#555'>> @@ -110,8 +116,10 @@ static param_export_t params[] = {
{"timer_interval", INT_PARAM, &timer_interval },
{"db_mode", INT_PARAM, &db_mode },
- {"match_contact_host_port", INT_PARAM, &match_contact_host_port },
- {"expires_grace", INT_PARAM, &expires_grace },
+ {"match_contact_host_port", INT_PARAM, &match_contact_host_port },
</pre>
<p dir="auto">As mentioned, this two parameter should be also documented in the XML.<br>
If the parameter are for the DB cleanup job, its probably better to name them in this direction, e.g. db_cleanup_interval or similar.</p>
<p dir="auto">What is the purpose of the expired_pcontacts_timeout, if there is already the expires_grace value?</p>
<hr>
<p>In <a href="https://github.com/kamailio/kamailio/pull/3279#discussion_r1018360764">src/modules/ims_usrloc_pcscf/pcontact.c</a>:</p>
<pre style='color:#555'>> + return;
+ }
+ }
+ else{
+ if ((_c->expires - act_time) + expires_grace >= 0) {
+ return;
+ }
+ }
+ }
+ else{
+ // not found in DB: better delete in cache also
+ update_stat(_c->slot->d->expired, 1);
+ mem_delete_pcontact(_c->slot->d, _c);
+ return;
+ }
+ // PRM1945 : PUBLISH already sent: delete pcontact and pua after time window 30 secs
</pre>
<p dir="auto">Not sure what PRM1945 is, if its some internal bug tracker please remove it.</p>
<hr>
<p>In <a href="https://github.com/kamailio/kamailio/pull/3279#discussion_r1018367559">src/modules/ims_usrloc_pcscf/pcontact.c</a>:</p>
<pre style='color:#555'>> @@ -277,6 +277,17 @@ int new_pcontact(struct udomain* _d, str* _contact, struct pcontact_info* _ci, s
(*_c)->num_service_routes = _ci->num_service_routes;
}
}
+ // add the rx session id
+ if ((_ci->rx_regsession_id) && (_ci->rx_regsession_id->len > 0) && (_ci->rx_regsession_id->s)) {
+ (*_c)->rx_session_id.s = shm_malloc(_ci->rx_regsession_id->len);
+ if (!((*_c)->rx_session_id.s)) {
+ LM_ERR("no more shm mem\n");
+ goto out_of_memory;
+ }
+ memcpy((*_c)->rx_session_id.s, _ci->rx_regsession_id->s, _ci->rx_regsession_id->len);
+ (*_c)->rx_session_id.len = _ci->rx_regsession_id->len;
+ }
+ //(*_c)->str_callback_registered = 0;
</pre>
<p dir="auto">if not needed, remove</p>
<hr>
<p>In <a href="https://github.com/kamailio/kamailio/pull/3279#discussion_r1018373315">src/modules/ims_usrloc_pcscf/udomain.c</a>:</p>
<pre style='color:#555'>> @@ -406,9 +408,11 @@ int update_pcontact(struct udomain* _d, struct pcontact_info* _ci, struct pconta
//TODO: update path, etc
- if (db_mode == WRITE_THROUGH && db_update_pcontact(_c) != 0) {
- LM_ERR("Error updating record in DB");
- return -1;
+ if ((db_mode == WRITE_THROUGH) || (db_mode == DB_ONLY)){
+ if (db_update_pcontact(_c) != 0) {
</pre>
<p dir="auto">Minor comment, would be nice to combine the two if statements, similar as done below in several instances.</p>
<hr>
<p>In <a href="https://github.com/kamailio/kamailio/pull/3279#discussion_r1018387964">src/modules/ims_usrloc_pcscf/udomain.c</a>:</p>
<pre style='color:#555'>> +
+struct tm *t;
+char str_time[100];
+
+str query_location, aor, expiry_date;
+query_location.s = p_location;
+query_location.len = strlen(query_location.s);
+
+if (use_location_pcscf_table("location") < 0) {
+ LM_ERR("failed to use table location");
+ return 0;
+}
+
+expired_50secs = time(0) - expires_grace - audit_expired_pcontacts_timeout;
+
+t = localtime(&expired_50secs);
</pre>
<p dir="auto">use the existing function time2str(..) for the conversion, it has already the format and error checks</p>
<hr>
<p>In <a href="https://github.com/kamailio/kamailio/pull/3279#discussion_r1018392123">src/modules/ims_usrloc_pcscf/usrloc_db.c</a>:</p>
<pre style='color:#555'>>
int db_update_pcontact(pcontact_t* _c)
{
str impus, service_routes;
db_val_t match_values[2];
- db_key_t match_keys[2] = { &aor_col, &received_port_col };
- db_op_t op[2];
- db_key_t update_keys[8] = { &expires_col, ®_state_col,
- &service_routes_col, &received_col,
- &received_port_col, &received_proto_col,
- &rx_session_id_col, &public_ids_col };
+ //db_key_t match_keys[2] = { &aor_col, &received_port_col };
</pre>
<p dir="auto">remove</p>
<hr>
<p>In <a href="https://github.com/kamailio/kamailio/pull/3279#discussion_r1018392193">src/modules/ims_usrloc_pcscf/usrloc_db.c</a>:</p>
<pre style='color:#555'>> @@ -146,8 +151,8 @@ int db_update_pcontact(pcontact_t* _c)
VAL_NULL(match_values + 1) = 0;
VAL_INT(match_values + 1) = _c->received_port;
- op[0]=OP_EQ;
- op[1]=OP_EQ;
+ // op[0]=OP_EQ;
</pre>
<p dir="auto">remove</p>
<hr>
<p>In <a href="https://github.com/kamailio/kamailio/pull/3279#discussion_r1018392289">src/modules/ims_usrloc_pcscf/usrloc_db.c</a>:</p>
<pre style='color:#555'>> @@ -195,17 +200,22 @@ int db_update_pcontact(pcontact_t* _c)
SET_PROPER_NULL_FLAG(impus, values, 7);
SET_STR_VALUE(values + 7, impus);
- if((ul_dbf.update(ul_dbh, match_keys, op, match_values, update_keys,values, 2, 8)) !=0){
- LM_ERR("could not update database info\n");
- return -1;
- }
+ //if((ul_dbf.update(ul_dbh, match_keys, op, match_values, update_keys,values, 2, 8)) !=0){
</pre>
<p dir="auto">remove</p>
<hr>
<p>In <a href="https://github.com/kamailio/kamailio/pull/3279#discussion_r1018392435">src/modules/ims_usrloc_pcscf/usrloc_db.c</a>:</p>
<pre style='color:#555'>>
- if (ul_dbf.affected_rows && ul_dbf.affected_rows(ul_dbh) == 0) {
- LM_DBG("no existing rows for an update... doing insert\n");
- if (db_insert_pcontact(_c) != 0) {
- LM_ERR("Failed to insert a pcontact on update\n");
- }
- }
+ if (db_insert_pcontact(_c) != 0) {
+ LM_ERR("Failed to insert a pcontact on update\n");
+ return -1;
+ }
+
+// if (ul_dbf.affected_rows && ul_dbf.affected_rows(ul_dbh) == 0) {
</pre>
<p dir="auto">remove</p>
<hr>
<p>In <a href="https://github.com/kamailio/kamailio/pull/3279#discussion_r1018394042">src/modules/ims_usrloc_pcscf/usrloc_db.c</a>:</p>
<pre style='color:#555'>> @@ -341,7 +351,7 @@ int db_insert_pcontact(struct pcontact* _c)
return -1;
}
- if (ul_dbf.insert(ul_dbh, keys, values, 16) < 0) {
+ if (ul_dbf.insert_update(ul_dbh, keys, values, 16) < 0) {
</pre>
<p dir="auto">Why you are changing the module behaviour from INSERT to INSERT ON DUPLICATE KEY UPDATE?</p>
<hr>
<p>In <a href="https://github.com/kamailio/kamailio/pull/3279#discussion_r1018394591">src/modules/ims_usrloc_pcscf/ul_rpc.c</a>:</p>
<pre style='color:#555'>> + }
+ } else {
+ if (rpc->struct_add(ah, "d", "Expires", (int) (c->expires - t)) < 0) {
+ unlock_ulslot(dom, i);
+ rpc->fault(ctx, 500, "Internal error adding expire");
+ return;
+ }
+ }
+
+ if (rpc->struct_add(ah, "S", "Path", &c->path) < 0) {
+ unlock_ulslot(dom, i);
+ rpc->fault(ctx, 500, "Internal error creating path struct");
+ return;
+ }
+
+ // if (rpc->struct_add(ah, "{", "Service Routes", &sr) < 0) {
</pre>
<p dir="auto">This can be kept, it was commented out before.</p>
<hr>
<p>In <a href="https://github.com/kamailio/kamailio/pull/3279#discussion_r1018395081">src/modules/ims_usrloc_pcscf/ul_callback.h</a>:</p>
<pre style='color:#555'>>
#ifndef _UL_CALLBACKS_H
#define _UL_CALLBACKS_H
#include "../../core/dprint.h"
+#include "../../core/parser/msg_parser.h"
+#include "../../core/parser/contact/parse_contact.h"
+#include "../../core/ut.h"
+#include "../ims_usrloc_pcscf/usrloc.h"
+#include "../../lib/ims/ims_getters.h"
+// %KTACT%-END bugfix__PRM19/0000138_no_str_sent
</pre>
<p dir="auto">Should be probably removed</p>
<hr>
<p>In <a href="https://github.com/kamailio/kamailio/pull/3279#discussion_r1018395720">src/modules/ims_usrloc_pcscf/ul_callback.c</a>:</p>
<pre style='color:#555'>> + LM_CRIT("null callback function\n");
+ return E_BUG;
+ }
+
+ /* build a new callback structure */
+ if ( types & PCSCF_CONTACT_UPDATE){
+ if (!(cbp_registrar=(struct ul_callback*)shm_malloc(sizeof( struct ul_callback)))) {
+ LM_ERR("no more share mem\n");
+ return E_OUT_OF_MEM;
+ }
+ cbp_registrar->callback = f;
+ }
+ else{
+ if (!(cbp_qos=(struct ul_callback*)shm_malloc(sizeof( struct ul_callback)))) {
+ LM_ERR("no more share mem\n");
+ return E_OUT_OF_MEM;
</pre>
<p dir="auto">You probably need to free the previous allocated memory at cbp_registrar.</p>
<hr>
<p>In <a href="https://github.com/kamailio/kamailio/pull/3279#discussion_r1018396575">src/modules/ims_usrloc_pcscf/udomain.c</a>:</p>
<pre style='color:#555'>> }
+expiry_date.s = str_time;
+expiry_date.len = expiry_str_len;
+
+len = query_location.len + expiry_str_len + 1/*nul*/;
+query_buffer.s = (char*) pkg_malloc(len);
+if (!query_buffer.s) {
+ LM_ERR("mo more pkg mem\n");
+ return -1;
+}
+query_buffer_len = len;
+snprintf(query_buffer.s, query_buffer_len, p_location, expiry_date.len, expiry_date.s);
+query_buffer.len = strlen(query_buffer.s);//len;
+
+LM_INFO("location QUERY IS [%.*s] and len is %d\n", query_buffer.len, query_buffer.s, query_buffer.len);
</pre>
<p dir="auto">As this is probably executed every timer run, it should probably be DEBUG log level.</p>
<hr>
<p>In <a href="https://github.com/kamailio/kamailio/pull/3279#discussion_r1018396803">src/modules/ims_usrloc_pcscf/udomain.c</a>:</p>
<pre style='color:#555'>> +query_buffer.s = (char*) pkg_malloc(len);
+if (!query_buffer.s) {
+ LM_ERR("mo more pkg mem\n");
+ return -1;
+}
+query_buffer_len = len;
+snprintf(query_buffer.s, query_buffer_len, p_location, expiry_date.len, expiry_date.s);
+query_buffer.len = strlen(query_buffer.s);//len;
+
+LM_INFO("location QUERY IS [%.*s] and len is %d\n", query_buffer.len, query_buffer.s, query_buffer.len);
+if (ul_dbf.raw_query(ul_dbh, &query_buffer, &location_rs) != 0) {
+ LM_ERR("Unable to query DB for expired pcontacts\n");
+ ul_dbf.free_result(ul_dbh, location_rs);
+} else {
+ if (RES_ROW_N(location_rs) == 0) {
+ LM_INFO("no expired pcontacts found in DB\n");
</pre>
<p dir="auto">See above</p>
<hr>
<p>In <a href="https://github.com/kamailio/kamailio/pull/3279#discussion_r1018397222">src/modules/ims_usrloc_pcscf/udomain.c</a>:</p>
<pre style='color:#555'>> + LM_ERR("Unable to query DB for expired pcontacts\n");
+ ul_dbf.free_result(ul_dbh, location_rs);
+} else {
+ if (RES_ROW_N(location_rs) == 0) {
+ LM_INFO("no expired pcontacts found in DB\n");
+ ul_dbf.free_result(ul_dbh, location_rs);
+ goto done;
+ }
+
+ for(i = 0; i < RES_ROW_N(location_rs); i++) {
+ row = RES_ROWS(location_rs) + i;
+
+ aor.s = (char*) VAL_STRING(ROW_VALUES(row) + 1);
+
+ if (VAL_NULL(ROW_VALUES(row) + 1) || aor.s == 0 || aor.s[0] == 0) {
+ LM_CRIT("empty aor record in table %s...skipping\n", _d->name->s);
</pre>
<p dir="auto">You should use CRITICAL log level only for really critical topics. This looks more that ERROR is fine.</p>
<hr>
<p>In <a href="https://github.com/kamailio/kamailio/pull/3279#discussion_r1018397522">src/modules/ims_usrloc_pcscf/udomain.c</a>:</p>
<pre style='color:#555'>> + if (VAL_NULL(ROW_VALUES(row) + 1) || aor.s == 0 || aor.s[0] == 0) {
+ LM_CRIT("empty aor record in table %s...skipping\n", _d->name->s);
+ continue;
+ }
+ aor.len = strlen(aor.s);
+ ci = dbrow2info(ROW_VALUES(row) + 1, &aor);
+ if (!ci) {
+ LM_WARN("Failed to get contact info from DB.... continuing...\n");
+ continue;
+ }
+ ci->aor = aor;
+ ci->searchflag = SEARCH_NORMAL;
+ ci->reg_state = PCONTACT_ANY;
+ if (get_pcontact_from_cache(_d, ci, &c, 0) == 0){
+ /*
+ if (&c->head->public_identity)
</pre>
<p dir="auto">remove</p>
<hr>
<p>In <a href="https://github.com/kamailio/kamailio/pull/3279#discussion_r1018398172">src/modules/ims_usrloc_pcscf/udomain.c</a>:</p>
<pre style='color:#555'>> +str query_buffer = { 0, 0 };
+int query_buffer_len = 0;
+
+struct tm *t;
+char str_time[100];
+
+str query_location, aor, expiry_date;
+query_location.s = p_location;
+query_location.len = strlen(query_location.s);
+
+if (use_location_pcscf_table("location") < 0) {
+ LM_ERR("failed to use table location");
+ return 0;
+}
+
+expired_50secs = time(0) - expires_grace - audit_expired_pcontacts_timeout;
</pre>
<p dir="auto">Consider using a better variable name</p>
<hr>
<p>In <a href="https://github.com/kamailio/kamailio/pull/3279#discussion_r1018399663">src/modules/ims_usrloc_pcscf/udomain.c</a>:</p>
<pre style='color:#555'>>
- ul_dbf.free_result(_c, res);
- return 0;
+
+int get_pcontact(udomain_t* _d, pcontact_info_t* contact_info, struct pcontact** _c, int reverse_search) {
+
+ int ret = get_pcontact_from_cache(_d, contact_info, _c, reverse_search);
+
+ if (ret && (db_mode == DB_ONLY)){
+ // LM_INFO("contact not found in cache for contact_info->via_port [%d]\n", contact_info->via_port);
</pre>
<p dir="auto">remove</p>
<hr>
<p>In <a href="https://github.com/kamailio/kamailio/pull/3279#discussion_r1018400798">src/modules/ims_usrloc_pcscf/udomain.c</a>:</p>
<pre style='color:#555'>> + return 0;
+ }
+ LM_INFO("Handling Result for query received\n");
+
+ for(i = 0; i < RES_ROW_N(res); i++) {
+ row = RES_ROWS(res) + i;
+
+ aor.s = (char*) VAL_STRING(ROW_VALUES(row) + 1);
+ if (VAL_NULL(ROW_VALUES(row) + 1) || aor.s == 0 || aor.s[0] == 0) {
+ LM_CRIT("empty aor record in table %s...skipping\n", _d->name->s);
+ continue;
+ }
+ aor.len = strlen(aor.s);
+
+ if ((_aor->len==0 && !_aor->s) && (VAL_NULL(ROW_VALUES(row) + 5) || VAL_NULL(ROW_VALUES(row) + 6))){
+ LM_CRIT("empty received_host or received_port record in table %s...skipping\n", _d->name->s);
</pre>
<p dir="auto">You should use CRITICAL log level only for really critical topics. This looks more that ERROR is fine.</p>
<hr>
<p>In <a href="https://github.com/kamailio/kamailio/pull/3279#discussion_r1018400931">src/modules/ims_usrloc_pcscf/udomain.c</a>:</p>
<pre style='color:#555'>> + LM_DBG("host [%.*s] and port [%.*s] not found in table %.*s\n", contact_info->received_host.len, contact_info->received_host.s, port.len, port.s, _d->name->len, _d->name->s);
+ }
+
+ ul_dbf.free_result(db_handle, res);
+ if (query_buffer.s)
+ pkg_free(query_buffer.s);
+ return 0;
+ }
+ LM_INFO("Handling Result for query received\n");
+
+ for(i = 0; i < RES_ROW_N(res); i++) {
+ row = RES_ROWS(res) + i;
+
+ aor.s = (char*) VAL_STRING(ROW_VALUES(row) + 1);
+ if (VAL_NULL(ROW_VALUES(row) + 1) || aor.s == 0 || aor.s[0] == 0) {
+ LM_CRIT("empty aor record in table %s...skipping\n", _d->name->s);
</pre>
<p dir="auto">See below</p>
<hr>
<p>In <a href="https://github.com/kamailio/kamailio/pull/3279#discussion_r1018401228">src/modules/ims_usrloc_pcscf/udomain.c</a>:</p>
<pre style='color:#555'>> +
+ len = query_location.len + querylen + 2;
+ query_buffer.s = (char*) pkg_malloc(len);
+ if (!query_buffer.s) {
+ LM_ERR("no more pkg mem\n");
+ return 0;
+ }
+
+ if (_aor->len>0 && _aor->s){
+ snprintf(query_buffer.s, len, p_location, _aor->len, _aor->s);
+ } else {
+ snprintf(query_buffer.s, len, p_location, contact_info->received_host.len, contact_info->received_host.s, port.len, port.s);
+ }
+ query_buffer.len = strlen(query_buffer.s);//len;
+
+ LM_INFO("QUERY IS [%.*s] and len is %d\n", query_buffer.len, query_buffer.s, query_buffer.len);
</pre>
<p dir="auto">This should be probably DEBUG log level</p>
<hr>
<p>In <a href="https://github.com/kamailio/kamailio/pull/3279#discussion_r1018401563">src/modules/ims_usrloc_pcscf/udomain.c</a>:</p>
<pre style='color:#555'>> +
+ if (_aor->len>0 && _aor->s){
+ LM_INFO("Querying database for P-CSCF contact [%.*s]\n", _aor->len, _aor->s);
+
+ p_location = "SELECT domain,aor,host,port,protocol,received,received_port,received_proto,rx_session_id,reg_state,expires,socket,service_routes,public_ids,path from location WHERE aor=\"%.*s\""; // AND reg_state=%d";
+ querylen = _aor->len;
+ } else {
+ LM_INFO("Querying database for P-CSCF received_host [%.*s] and received_port [%d]\n", contact_info->received_host.len, contact_info->received_host.s, contact_info->received_port);
+ p_location = "SELECT domain,aor,host,port,protocol,received,received_port,received_proto,rx_session_id,reg_state,expires,socket,service_routes,public_ids,path from location WHERE received=\"%.*s\" AND received_port=\"%.*s\"";
+ port.s = int2str(contact_info->received_port, &port.len);
+ querylen = contact_info->received_host.len + port.len;
+ }
+ query_location.s = p_location;
+ query_location.len = strlen(query_location.s);
+
+ LM_INFO("TEST: Query location [%.*s]\n", query_location.len, query_location.s);
</pre>
<p dir="auto">This should be probably DEBUG log level</p>
<hr>
<p>In <a href="https://github.com/kamailio/kamailio/pull/3279#discussion_r1018401685">src/modules/ims_usrloc_pcscf/udomain.c</a>:</p>
<pre style='color:#555'>> - columns[11] = &socket_col;
- columns[12] = &service_routes_col;
- columns[13] = &public_ids_col;
- columns[14] = &path_col;
+ int i, len, querylen;
+ str aor, query_location, query_buffer, port={0,0};
+ db1_con_t* db_handle = 0;
+ char *p_location;
+
+ if (_aor->len>0 && _aor->s){
+ LM_INFO("Querying database for P-CSCF contact [%.*s]\n", _aor->len, _aor->s);
+
+ p_location = "SELECT domain,aor,host,port,protocol,received,received_port,received_proto,rx_session_id,reg_state,expires,socket,service_routes,public_ids,path from location WHERE aor=\"%.*s\""; // AND reg_state=%d";
+ querylen = _aor->len;
+ } else {
+ LM_INFO("Querying database for P-CSCF received_host [%.*s] and received_port [%d]\n", contact_info->received_host.len, contact_info->received_host.s, contact_info->received_port);
</pre>
<p dir="auto">See below</p>
<hr>
<p>In <a href="https://github.com/kamailio/kamailio/pull/3279#discussion_r1018401960">src/modules/ims_usrloc_pcscf/udomain.c</a>:</p>
<pre style='color:#555'>> - columns[6] = &received_port_col;
- columns[7] = &received_proto_col;
- columns[8] = &rx_session_id_col;
- columns[9] = ®_state_col;
- columns[10] = &expires_col;
- columns[11] = &socket_col;
- columns[12] = &service_routes_col;
- columns[13] = &public_ids_col;
- columns[14] = &path_col;
+ int i, len, querylen;
+ str aor, query_location, query_buffer, port={0,0};
+ db1_con_t* db_handle = 0;
+ char *p_location;
+
+ if (_aor->len>0 && _aor->s){
+ LM_INFO("Querying database for P-CSCF contact [%.*s]\n", _aor->len, _aor->s);
</pre>
<p dir="auto">See below</p>
<hr>
<p>In <a href="https://github.com/kamailio/kamailio/pull/3279#discussion_r1018402212">src/modules/ims_usrloc_pcscf/udomain.c</a>:</p>
<pre style='color:#555'>> +
+ for(i = 0; i < RES_ROW_N(res); i++) {
+ row = RES_ROWS(res) + i;
+
+ aor.s = (char*) VAL_STRING(ROW_VALUES(row) + 1);
+ if (VAL_NULL(ROW_VALUES(row) + 1) || aor.s == 0 || aor.s[0] == 0) {
+ LM_CRIT("empty aor record in table %s...skipping\n", _d->name->s);
+ continue;
+ }
+ aor.len = strlen(aor.s);
+
+ if ((_aor->len==0 && !_aor->s) && (VAL_NULL(ROW_VALUES(row) + 5) || VAL_NULL(ROW_VALUES(row) + 6))){
+ LM_CRIT("empty received_host or received_port record in table %s...skipping\n", _d->name->s);
+ continue;
+ }
+ LM_INFO("Convert database values extracted with AOR.");
</pre>
<p dir="auto">DEBUG log level</p>
<hr>
<p>In <a href="https://github.com/kamailio/kamailio/pull/3279#discussion_r1018402555">src/modules/ims_usrloc_pcscf/udomain.c</a>:</p>
<pre style='color:#555'>>
- ul_dbf.free_result(_c, res);
- return 0;
+
+int get_pcontact(udomain_t* _d, pcontact_info_t* contact_info, struct pcontact** _c, int reverse_search) {
+
+ int ret = get_pcontact_from_cache(_d, contact_info, _c, reverse_search);
+
+ if (ret && (db_mode == DB_ONLY)){
+ // LM_INFO("contact not found in cache for contact_info->via_port [%d]\n", contact_info->via_port);
+ LM_INFO("contact not found in cache for contact_info->received_port [%d]\n", contact_info->received_port);
</pre>
<p dir="auto">DEBUG log level?</p>
<hr>
<p>In <a href="https://github.com/kamailio/kamailio/pull/3279#discussion_r1018402639">src/modules/ims_usrloc_pcscf/udomain.c</a>:</p>
<pre style='color:#555'>> +int get_pcontact(udomain_t* _d, pcontact_info_t* contact_info, struct pcontact** _c, int reverse_search) {
+
+ int ret = get_pcontact_from_cache(_d, contact_info, _c, reverse_search);
+
+ if (ret && (db_mode == DB_ONLY)){
+ // LM_INFO("contact not found in cache for contact_info->via_port [%d]\n", contact_info->via_port);
+ LM_INFO("contact not found in cache for contact_info->received_port [%d]\n", contact_info->received_port);
+ // following if: change for CATT test tool - PCSCF redundancy test cases
+ if (contact_info->searchflag == SEARCH_RECEIVED){
+ LM_INFO("Trying contact_info.extra_search_criteria = 0\n");
+ contact_info->extra_search_criteria = 0;
+ ret = get_pcontact_from_cache(_d, contact_info, _c, reverse_search);
+ if (ret == 0){
+ return ret;
+ }
+ LM_INFO("contact not found in cache for contact_info->via_port [%d]\n", contact_info->via_port);
</pre>
<p dir="auto">see above</p>
<hr>
<p>In <a href="https://github.com/kamailio/kamailio/pull/3279#discussion_r1018402746">src/modules/ims_usrloc_pcscf/udomain.c</a>:</p>
<pre style='color:#555'>> + if (ret && (db_mode == DB_ONLY)){
+ // LM_INFO("contact not found in cache for contact_info->via_port [%d]\n", contact_info->via_port);
+ LM_INFO("contact not found in cache for contact_info->received_port [%d]\n", contact_info->received_port);
+ // following if: change for CATT test tool - PCSCF redundancy test cases
+ if (contact_info->searchflag == SEARCH_RECEIVED){
+ LM_INFO("Trying contact_info.extra_search_criteria = 0\n");
+ contact_info->extra_search_criteria = 0;
+ ret = get_pcontact_from_cache(_d, contact_info, _c, reverse_search);
+ if (ret == 0){
+ return ret;
+ }
+ LM_INFO("contact not found in cache for contact_info->via_port [%d]\n", contact_info->via_port);
+ contact_info->extra_search_criteria = SEARCH_SERVICE_ROUTES;
+
+ contact_info->searchflag = SEARCH_NORMAL;
+ LM_INFO("Trying contact_info.searchflag = SEARCH_NORMAL\n");
</pre>
<p dir="auto">see above</p>
<hr>
<p>In <a href="https://github.com/kamailio/kamailio/pull/3279#discussion_r1018402869">src/modules/ims_usrloc_pcscf/udomain.c</a>:</p>
<pre style='color:#555'>> + contact_info->extra_search_criteria = 0;
+ ret = get_pcontact_from_cache(_d, contact_info, _c, reverse_search);
+ if (ret == 0){
+ return ret;
+ }
+ LM_INFO("contact not found in cache for contact_info->via_port [%d]\n", contact_info->via_port);
+ contact_info->extra_search_criteria = SEARCH_SERVICE_ROUTES;
+
+ contact_info->searchflag = SEARCH_NORMAL;
+ LM_INFO("Trying contact_info.searchflag = SEARCH_NORMAL\n");
+ ret = get_pcontact_from_cache(_d, contact_info, _c, reverse_search);
+ if (ret == 0){
+ return ret;
+ }
+ else {
+ LM_INFO("Trying contact_info.extra_search_criteria = 0\n");
</pre>
<p dir="auto">see above</p>
<hr>
<p>In <a href="https://github.com/kamailio/kamailio/pull/3279#discussion_r1018403028">src/modules/ims_usrloc_pcscf/udomain.c</a>:</p>
<pre style='color:#555'>> + return ret;
+ }
+ else {
+ LM_INFO("Trying contact_info.extra_search_criteria = 0\n");
+ contact_info->extra_search_criteria = 0;
+ ret = get_pcontact_from_cache(_d, contact_info, _c, reverse_search);
+ if (ret == 0){
+ return ret;
+ }
+ LM_INFO("contact not found in cache for contact_info->via_port [%d]\n", contact_info->via_port);
+ contact_info->extra_search_criteria = SEARCH_SERVICE_ROUTES;
+ contact_info->searchflag = SEARCH_RECEIVED;
+ }
+ }
+ else {
+ LM_INFO("Trying contact_info.extra_search_criteria = 0\n");
</pre>
<p dir="auto">See above</p>
<hr>
<p>In <a href="https://github.com/kamailio/kamailio/pull/3279#discussion_r1018403280">src/modules/ims_usrloc_pcscf/udomain.c</a>:</p>
<pre style='color:#555'>> + }
+ else {
+ LM_INFO("Trying contact_info.extra_search_criteria = 0\n");
+ contact_info->extra_search_criteria = 0;
+ ret = get_pcontact_from_cache(_d, contact_info, _c, reverse_search);
+ if (ret == 0){
+ return ret;
+ }
+ LM_INFO("contact not found in cache for contact_info->via_port [%d]\n", contact_info->via_port);
+ contact_info->extra_search_criteria = SEARCH_SERVICE_ROUTES;
+ }
+ if (db_load_pcontact(_d, &contact_info->aor, 1/*insert_cache*/, _c, contact_info)){
+ LM_INFO("loaded location from db for AOR [%.*s]\n", contact_info->aor.len, contact_info->aor.s);
+ return 0;
+ } else {
+ LM_INFO("download location DB failed for AOR [%.*s]\n", contact_info->aor.len, contact_info->aor.s);
</pre>
<p dir="auto">Is this an ERROR?</p>
<hr>
<p>In <a href="https://github.com/kamailio/kamailio/pull/3279#discussion_r1018403446">src/modules/ims_usrloc_pcscf/udomain.c</a>:</p>
<pre style='color:#555'>> - ul_dbf.free_result(_c, res);
- return 0;
- }
+ if (!port.s) {
+ LM_DBG("aor [%.*s] not found in table %.*s\n",_aor->len, _aor->s, _d->name->len, _d->name->s);
+ }
+ else {
+ LM_DBG("host [%.*s] and port [%.*s] not found in table %.*s\n", contact_info->received_host.len, contact_info->received_host.s, port.len, port.s, _d->name->len, _d->name->s);
+ }
+
+ ul_dbf.free_result(db_handle, res);
+ if (query_buffer.s)
+ pkg_free(query_buffer.s);
+ return 0;
+ }
+ LM_INFO("Handling Result for query received\n");
</pre>
<p dir="auto">DEBUG</p>
<hr>
<p>In <a href="https://github.com/kamailio/kamailio/pull/3279#discussion_r1018403880">src/modules/ims_usrloc_pcscf/udomain.c</a>:</p>
<pre style='color:#555'>> + if (ul_dbf.use_table(db_handle, &_pua) < 0) {
+ LM_ERR("failed to use table pua");
+ return 0;
+ }
+
+ int len = query_pua.len + presentity_uri->len + 2;
+ query_buffer.s = (char*) pkg_malloc(len);
+ if (!query_buffer.s) {
+ LM_ERR("mo more pkg mem\n");
+ return 0;
+ }
+ snprintf(query_buffer.s, len, p_pua, presentity_uri->len, presentity_uri->s);
+ query_buffer.len = strlen(query_buffer.s);//len;
+
+
+ LM_INFO("QUERY IS [%.*s] and len is %d\n", query_buffer.len, query_buffer.s, query_buffer.len);
</pre>
<p dir="auto">DEBUG log level</p>
<hr>
<p>In <a href="https://github.com/kamailio/kamailio/pull/3279#discussion_r1018404945">src/modules/ims_usrloc_pcscf/pcontact.c</a>:</p>
<pre style='color:#555'>> + return;
+ }
+ }
+ }
+ else{
+ // not found in DB: better delete in cache also
+ update_stat(_c->slot->d->expired, 1);
+ mem_delete_pcontact(_c->slot->d, _c);
+ return;
+ }
+ // PRM1945 : PUBLISH already sent: delete pcontact and pua after time window 30 secs
+ // currently not clear what happens if more contacts existing for one public_identity (pua)
+ // which are not expired
+ if ((_c->reg_state == PCONTACT_DEREG_PENDING_PUBLISH) &&
+ ((_c->expires - act_time) + expires_grace + 30 <= 0)){
+ LM_INFO("Deleting expired pcontact: <%.*s>\n", _c->aor.len, _c->aor.s);
</pre>
<p dir="auto">DEBUG log level?</p>
<hr>
<p>In <a href="https://github.com/kamailio/kamailio/pull/3279#discussion_r1018408076">src/modules/ims_usrloc_pcscf/udomain.c</a>:</p>
<pre style='color:#555'>> @@ -1077,91 +1113,408 @@ int preload_udomain(db1_con_t* _c, udomain_t* _d)
return -1;
}
-pcontact_t* db_load_pcontact(db1_con_t* _c, udomain_t* _d, str *_aor)
+int db_delete_presentityuri_from_pua(str *presentity_uri)
+{
+ db1_res_t* res = NULL;
+ str _pua;
+
+ db1_con_t* db_handle = 0;
+
+ char *p_pua = "DELETE from pua where pua.pres_uri =\"%.*s\"";
</pre>
<p dir="auto">Do I understand it correctly that you delete data from the pua module table? I am afraid this is not the way it should be done. If you need this functionality, the proper way would be to use an API of the PUA module.</p>
<hr>
<p>In <a href="https://github.com/kamailio/kamailio/pull/3279#discussion_r1018410092">src/modules/ims_usrloc_pcscf/udomain.c</a>:</p>
<pre style='color:#555'>> - columns[8] = &rx_session_id_col;
- columns[9] = ®_state_col;
- columns[10] = &expires_col;
- columns[11] = &socket_col;
- columns[12] = &service_routes_col;
- columns[13] = &public_ids_col;
- columns[14] = &path_col;
+ int i, len, querylen;
+ str aor, query_location, query_buffer, port={0,0};
+ db1_con_t* db_handle = 0;
+ char *p_location;
+
+ if (_aor->len>0 && _aor->s){
+ LM_INFO("Querying database for P-CSCF contact [%.*s]\n", _aor->len, _aor->s);
+
+ p_location = "SELECT domain,aor,host,port,protocol,received,received_port,received_proto,rx_session_id,reg_state,expires,socket,service_routes,public_ids,path from location WHERE aor=\"%.*s\""; // AND reg_state=%d";
</pre>
<p dir="auto">This is not the way you should interact with the DB API. There are actually methods to build a select query. You can find some examples e.g. in the usrloc/dlist.c or in usrloc_db.c files.<br>
This way the module will work with all database drivers that implement the proper functions.</p>
<hr>
<p>In <a href="https://github.com/kamailio/kamailio/pull/3279#discussion_r1018410236">src/modules/ims_usrloc_pcscf/udomain.c</a>:</p>
<pre style='color:#555'>> - columns[12] = &service_routes_col;
- columns[13] = &public_ids_col;
- columns[14] = &path_col;
+ int i, len, querylen;
+ str aor, query_location, query_buffer, port={0,0};
+ db1_con_t* db_handle = 0;
+ char *p_location;
+
+ if (_aor->len>0 && _aor->s){
+ LM_INFO("Querying database for P-CSCF contact [%.*s]\n", _aor->len, _aor->s);
+
+ p_location = "SELECT domain,aor,host,port,protocol,received,received_port,received_proto,rx_session_id,reg_state,expires,socket,service_routes,public_ids,path from location WHERE aor=\"%.*s\""; // AND reg_state=%d";
+ querylen = _aor->len;
+ } else {
+ LM_INFO("Querying database for P-CSCF received_host [%.*s] and received_port [%d]\n", contact_info->received_host.len, contact_info->received_host.s, contact_info->received_port);
+ p_location = "SELECT domain,aor,host,port,protocol,received,received_port,received_proto,rx_session_id,reg_state,expires,socket,service_routes,public_ids,path from location WHERE received=\"%.*s\" AND received_port=\"%.*s\"";
</pre>
<p dir="auto">See above</p>
<hr>
<p>In <a href="https://github.com/kamailio/kamailio/pull/3279#discussion_r1018410457">src/modules/ims_usrloc_pcscf/udomain.c</a>:</p>
<pre style='color:#555'>> +
+ return ret;
+
+}
+
+
+int audit_usrloc_expired_pcontacts(udomain_t* _d) {
+db1_res_t* location_rs = NULL;
+
+pcontact_info_t *ci;
+db_row_t *row;
+int i;
+pcontact_t* c;
+time_t expired_50secs;
+
+char *p_location = "SELECT domain,aor,host,port,protocol,received,received_port,received_proto,rx_session_id,reg_state,expires,socket,service_routes,public_ids,path from location WHERE expires < \"%.*s\"";
</pre>
<p dir="auto">This is not the way you should interact with the DB API. There are actually methods to build a select query. You can find some examples e.g. in the usrloc/dlist.c or usrloc_db files.<br>
This way the module will work with all database drivers that implement the proper functions.</p>
<p style="font-size:small;-webkit-text-size-adjust:none;color:#666;">—<br />Reply to this email directly, <a href="https://github.com/kamailio/kamailio/pull/3279#pullrequestreview-1174667681">view it on GitHub</a>, or <a href="https://github.com/notifications/unsubscribe-auth/ABO7UZO5SXWQ4HOONNLNCM3WHQHCXANCNFSM6AAAAAAR3RTGSI">unsubscribe</a>.<br />You are receiving this because you are subscribed to this thread.<img src="https://github.com/notifications/beacon/ABO7UZLMD63SFC45WAJSAO3WHQHCXA5CNFSM6AAAAAAR3RTGSKWGG33NNVSW45C7OR4XAZNRKB2WY3CSMVYXKZLTORJGK5TJMV32UY3PNVWWK3TUL5UWJTSGAQA2C.gif" height="1" width="1" alt="" /><span style="color: transparent; font-size: 0; display: none; visibility: hidden; overflow: hidden; opacity: 0; width: 0; height: 0; max-width: 0; max-height: 0; mso-hide: all">Message ID: <span><kamailio/kamailio/pull/3279/review/1174667681</span><span>@</span><span>github</span><span>.</span><span>com></span></span></p>
<script type="application/ld+json">[
{
"@context": "http://schema.org",
"@type": "EmailMessage",
"potentialAction": {
"@type": "ViewAction",
"target": "https://github.com/kamailio/kamailio/pull/3279#pullrequestreview-1174667681",
"url": "https://github.com/kamailio/kamailio/pull/3279#pullrequestreview-1174667681",
"name": "View Pull Request"
},
"description": "View this Pull Request on GitHub",
"publisher": {
"@type": "Organization",
"name": "GitHub",
"url": "https://github.com"
}
}
]</script>