while doing pua module tests in db_mode=2, i used pua_mi to send two publish requests. in both of them id and etag params had value '.'.
first generated publish request without sip-if-match header (as it should) and after 200 ok was received, record was properly inserted to pua table with etag value a.1413029856.5521.112.0 and empty pres_id.
the second for a different pres_uri however resulted in error in presence server:
Oct 12 10:06:42 siika /usr/bin/pres-serv[5522]: ERROR: presence [presentity.c:807]: update_presentity(): No E_Tag match a.1413029856.5521.112.0
after digging into it, i found with wireshark that the second publish indeed included sip-if-match header with that etag value even when it was initial publish (etag and id params '.') for another pres_uri.
then i took a look at pua source and found that if etag param is null it is not included in db query at all leaving only pres_id param to match against:
ua_pres_t *get_record_puadb(str pres_id, str *etag, ua_pres_t *result, db1_res_t **dbres) { int n_query_cols = 0, nr_rows; ... q_cols[n_query_cols] = &str_pres_id_col; q_vals[n_query_cols].type = DB1_STR; q_vals[n_query_cols].nul = 0; q_vals[n_query_cols].val.str_val = pres_id; n_query_cols++;
if (etag != NULL) { q_cols[n_query_cols] = &str_etag_col; q_vals[n_query_cols].type = DB1_STR; q_vals[n_query_cols].nul = 0; q_vals[n_query_cols].val.str_val.s = etag->s; q_vals[n_query_cols].val.str_val.len = etag->len; n_query_cols++; } ... if(query_fn(pua_db, q_cols, 0, q_vals, NULL,n_query_cols,0,0,&res) < 0) { LM_ERR("DB query error\n"); return(NULL); }
of course this kind of query matched the just inserted record for another pres_uri, which is wrong.
how should this be fixed? is it enough to always add etag value to the query or should also pres_uri be added just in case etags for different pres_uris would match? or perhaps the best fix would be to skip the query altogether in send_publish if etag is null?
has anyone else ever used pua module in db_mode=2?
-- juha
On 12/10/14 09:37, Juha Heinanen wrote:
while doing pua module tests in db_mode=2, i used pua_mi to send two publish requests. in both of them id and etag params had value '.'.
first generated publish request without sip-if-match header (as it should) and after 200 ok was received, record was properly inserted to pua table with etag value a.1413029856.5521.112.0 and empty pres_id.
the second for a different pres_uri however resulted in error in presence server:
Oct 12 10:06:42 siika /usr/bin/pres-serv[5522]: ERROR: presence [presentity.c:807]: update_presentity(): No E_Tag match a.1413029856.5521.112.0
after digging into it, i found with wireshark that the second publish indeed included sip-if-match header with that etag value even when it was initial publish (etag and id params '.') for another pres_uri.
then i took a look at pua source and found that if etag param is null it is not included in db query at all leaving only pres_id param to match against:
ua_pres_t *get_record_puadb(str pres_id, str *etag, ua_pres_t *result, db1_res_t **dbres) { int n_query_cols = 0, nr_rows; ... q_cols[n_query_cols] = &str_pres_id_col; q_vals[n_query_cols].type = DB1_STR; q_vals[n_query_cols].nul = 0; q_vals[n_query_cols].val.str_val = pres_id; n_query_cols++;
if (etag != NULL) { q_cols[n_query_cols] = &str_etag_col; q_vals[n_query_cols].type = DB1_STR; q_vals[n_query_cols].nul = 0; q_vals[n_query_cols].val.str_val.s = etag->s; q_vals[n_query_cols].val.str_val.len = etag->len; n_query_cols++; } ... if(query_fn(pua_db, q_cols, 0, q_vals, NULL,n_query_cols,0,0,&res) < 0) { LM_ERR("DB query error\n"); return(NULL); }
of course this kind of query matched the just inserted record for another pres_uri, which is wrong.
how should this be fixed? is it enough to always add etag value to the query or should also pres_uri be added just in case etags for different pres_uris would match? or perhaps the best fix would be to skip the query altogether in send_publish if etag is null?
I think that if it is an initial publish, then send out directly (I assume a new etag is going to be generated).
Then, iirc, etag is kind of unique globally per instance if generated by kamailio, so if it exists, then a query on it would be safe. But if it is generated by somone else, perhaps we cannot guarantee that, therefore I think the query has to be done on pres_uri as well to be in safe side always.
In other words, my impression is that both updates have to be done.
Cheers, Daniel
has anyone else ever used pua module in db_mode=2?
-- juha
sr-dev mailing list sr-dev@lists.sip-router.org http://lists.sip-router.org/cgi-bin/mailman/listinfo/sr-dev
Juha Heinanen writes:
how should this be fixed? is it enough to always add etag value to the query or should also pres_uri be added just in case etags for different pres_uris would match? or perhaps the best fix would be to skip the query altogether in send_publish if etag is null?
i tried the latter in pua/send_publish:
if (dbmode==PUA_DB_ONLY) { if (publ->etag) { memset(&dbpres, 0, sizeof(dbpres)); dbpres.pres_uri = &pres_uri; dbpres.watcher_uri = &watcher_uri; dbpres.extra_headers = &extra_headers; presentity = get_record_puadb(publ->id, publ->etag, &dbpres, &res); }
then also the initial publish for the second pres_uri was generated correctly without sip-if-match header. however, now when 200 ok with sip-etag header was received from presence server, pua did not insert second record to pua table, but updated etag of the existing record of the other pres_uri!
looks to me that pua module is pretty badly broken and i'm the first ever user who tries it in db_mode=2.
-- juha
On 12/10/14 09:56, Juha Heinanen wrote:
Juha Heinanen writes:
how should this be fixed? is it enough to always add etag value to the query or should also pres_uri be added just in case etags for different pres_uris would match? or perhaps the best fix would be to skip the query altogether in send_publish if etag is null?
i tried the latter in pua/send_publish:
if (dbmode==PUA_DB_ONLY) { if (publ->etag) { memset(&dbpres, 0, sizeof(dbpres)); dbpres.pres_uri = &pres_uri; dbpres.watcher_uri = &watcher_uri; dbpres.extra_headers = &extra_headers; presentity = get_record_puadb(publ->id, publ->etag, &dbpres, &res); }
then also the initial publish for the second pres_uri was generated correctly without sip-if-match header. however, now when 200 ok with sip-etag header was received from presence server, pua did not insert second record to pua table, but updated etag of the existing record of the other pres_uri!
looks to me that pua module is pretty badly broken and i'm the first ever user who tries it in db_mode=2.
Haven't used it in db_mode=2, but given that pua is an user-agent emulator, it is a matter of interaction from other modules as well. So far, the main usage in my side is for publishing dialog-info states, which works very well.
I had in mind that presence modules needed always to keep references in cache, but then I assume now that the db only mode lacks proper constraints in working with database, which should be easy to fix. Daniel
Daniel-Constantin Mierla writes:
Haven't used it in db_mode=2, but given that pua is an user-agent emulator, it is a matter of interaction from other modules as well. So far, the main usage in my side is for publishing dialog-info states, which works very well.
i have not detected any problems with presence module that pua is interacting with in my test.
I had in mind that presence modules needed always to keep references in cache, but then I assume now that the db only mode lacks proper constraints in working with database, which should be easy to fix.
there must be some bug in publ_cback_func() that causes update of wrong record instead of inserting a new one.
-- juha
On 12/10/14 10:23, Juha Heinanen wrote:
Daniel-Constantin Mierla writes:
Haven't used it in db_mode=2, but given that pua is an user-agent emulator, it is a matter of interaction from other modules as well. So far, the main usage in my side is for publishing dialog-info states, which works very well.
i have not detected any problems with presence module that pua is interacting with in my test.
I had in mind that presence modules needed always to keep references in cache, but then I assume now that the db only mode lacks proper constraints in working with database, which should be easy to fix.
there must be some bug in publ_cback_func() that causes update of wrong record instead of inserting a new one.
Looking quickly over the code, I think that queries on pua table must include the etag always. IIRC, SIP-ETag is returned in 200ok for PUBLISH and database table 'pua' has the definition with:
etag VARCHAR(64) NOT NULL,
Therefore, when the record is inserted, the etag must be known (since it is not null). Further updates, should be done using etag in the key.
Based on above, the function update_record_puadb() should return without doing the db update if pres->etag.s is not set. It should end up in an insert if pres->etag.s is not set.
Cheers, Daniel
Daniel-Constantin Mierla writes:
Based on above, the function update_record_puadb() should return without doing the db update if pres->etag.s is not set. It should end up in an insert if pres->etag.s is not set.
i'm not sure, since publ_cback_func() does make insert explicitly if record is not found. but it does not get that far, because for some reason, i see that this test succeeds:
if (pua_dbf.affected_rows != NULL || dbmode != PUA_DB_ONLY) { INFO("find_and_update_record\n"); if (find_and_update_record(hentity, hash_code, lexpire, &etag) > 0) goto done; }
what query is pua_dbf.affected_rows here referring to? i have not found in the function any db queries before the test.
-- juha
On 12/10/14 10:56, Juha Heinanen wrote:
Daniel-Constantin Mierla writes:
Based on above, the function update_record_puadb() should return without doing the db update if pres->etag.s is not set. It should end up in an insert if pres->etag.s is not set.
i'm not sure, since publ_cback_func() does make insert explicitly if record is not found. but it does not get that far, because for some reason, i see that this test succeeds:
if (pua_dbf.affected_rows != NULL || dbmode != PUA_DB_ONLY) { INFO("find_and_update_record\n"); if (find_and_update_record(hentity, hash_code, lexpire, &etag) > 0)
In the function of the above line is done an update with the etag not set and it shouldn't.
Either here the condition should be on hentity->etag.s being not null as well or the update_record_puadb() shoudl have its own safety check on etag (-- or even both for performance/safety).
goto done;}
what query is pua_dbf.affected_rows here referring to?
That is a check to see if that database connector module implements affected_rows function -- not a check of a result of affected rows.
Daniel
i have not found in the function any db queries before the test.
-- juha
Daniel-Constantin Mierla writes:
Either here the condition should be on hentity->etag.s being not null as well or the update_record_puadb() shoudl have its own safety check on etag (-- or even both for performance/safety).
i placed hentity->etag.s test before the update stuff and after that my tests worked ok:
if (hentity->etag.s) { if (pua_dbf.affected_rows != NULL || dbmode != PUA_DB_ONLY) { if (find_and_update_record(hentity, hash_code, lexpire, &etag) > 0) goto done; } else if ((db_presentity = get_record_puadb(hentity->id, &hentity->etag, &dbpres, &res)) != NULL) { update_record_puadb(hentity, lexpire, &etag); goto done; } }
-- juha
-
Daniel-Constantin Mierla -
Juha Heinanen