Module: sip-router Branch: master Commit: ed3902e9bd5a3af6ee5d5df08ce2564f028a0f24 URL: http://git.sip-router.org/cgi-bin/gitweb.cgi/sip-router/?a=commit;h=ed3902e9...
Author: Jan Janak jan@ryngle.com Committer: Jan Janak jan@ryngle.com Date: Sun Oct 18 00:39:31 2009 +0200
tm: Number of fixes in code and documentation for serial forking.
This patch improves the serial forking related code and documentation. The original code suffers from shortcommits, some of which were result of the migration to the new core.
The function that decodes destination sets encoded in AVPs has been improved, because the original version did not handle properly strings with missing elements. In such case the original implementation was likely to overwrite memory, because it did not check the return value of strchr properly. The new implementation tries to handle this situation. It continues parsing as long as it can, it only requires that the request-uri string is present, all other fields are made optional and their variables are properly initialized if their values cannot be found in the AVP.
There was a bug in the implementation of fr_inv_timer_next modparam in the original version, changes to the parameter value were ignored by the serial forking code. This was reported by Andrei and is now fixed by this commit. The parameter fr_inv_timer_next can now be configured at runtime with the configuration framework. Its value is in milliseconds and unlike fr_inv_timer, this timer cannot be configured separately for individual branches.
Obsolete definition of INV_FR_TIME_OUT_FIRST has been removed. That macro is not used anywhere in the code, thus it is not needed.
There were several places where LM_DBG printed a string and relied on the string being zero terminated, this may or may not be true in the future and this patch uses the macro STR_FMT where appropriate, which is safer.
Function t_next_contacts now checks if the function decode_branch_info really returned values for the dst_uri and the path vector. If not then it calls reset_dst_uri and reset_path_vector. Previous version of the code crashed sip-router, this is likely due to the merge and updates in the sip-router core.
We now use t_set_fr in t_next_contacts for setting the fr_inv_timer value to fr_inv_timer_next. This is much more efficient than creating AVPs with new timer values. Also the new value of the timer is now taken from a variable in the configuration framework, instead of just a regular global variable configured through modparam. This way we can adjust the value of the timer on the fly. Configuring it through modparam is, of course, possible too.
The value of of fr_inv_timer_next is now in milliseconds, instead of seconds. That's the only possibly incompatible change. However, this is consistent with all other timers in tm module, it is more efficient and it offers better granularity.
A missing call to destroy_avp has been added to t_next_contacts, in the code which is executed when no transaction exists. There, the avp should also be destroyed if all values have been exhausted and none of them had Q_FLAG set. This is a corner case which should not happen under normal circumstances, because that situation only happens if all branches have the same q value. Such AVP would not have been created by t_load_contacts and therefore t_next_contacts should not be called, but this bug may be triggered if someone uses t_next_contacts in an unexpected way and it is probably better to have it fixed.
Also the code which restores the value of fr_inv_timer at the end of t_next_contacts did not work properly. This patch fixes that. It first tries to retrieve a value configured with t_set_fr, but that is not guaranteed to succeed. After that it also tries the timer AVP and finally the configuration framework. The configuration framework always yields a value, so we can always restore the timer value, but we may fail to restore individual transaction timer values set by t_set_fr. If that fails then the global value from the configuration framework is used. This is documented as a shortcomming in the README and in the code.
In addition to code changes this patch also expands documentation on functions t_load_contacts and t_next_contacts, describing their operation in more detail. Also the format of the contacts AVP is now documented.
Finally, there is a whole new section in the README which describes how serial/parallel forking can be achieved with t_load_contacts and t_next_contacts and provides a number of examples.
---
modules/tm/README | 592 +++++++++++++++++++++++++++--------------- modules/tm/config.c | 7 +- modules/tm/config.h | 3 +- modules/tm/doc/functions.xml | 129 ++++++++-- modules/tm/doc/params.xml | 83 +++++-- modules/tm/doc/tm.xml | 218 ++++++++++++++++ modules/tm/t_serial.c | 208 ++++++++++------ modules/tm/tm.c | 2 +- 8 files changed, 900 insertions(+), 342 deletions(-)
Diff: http://git.sip-router.org/cgi-bin/gitweb.cgi/sip-router/?a=commitdiff;h=ed39...
Hello Juha,
This commit includes a number of changes I have done to the serial forking code in tm. The original version crashed sip-router and timers and other things did not work properly. I have tested the code and it seems to be working fine now, including all the things we discussed in other threads.
There is only one incompatible change. The value of fr_inv_timer_next is now in milliseconds. The previous version used seconds, but it did not work in sip-router anyway. All other tm timers use milliseconds, so we are now consistent.
I merged this commit also into sr_3.0 branch, because in addition to the new documentation this is all bugfixes. I think that this commit should also be merged into kamailio_3.0 branch, because the original code does not seem to work well with sip-router core.
I am also going to add support for serial forking to the all-inclusive config file in sip-router-oob.cfg. I have been waiting for q based serial forking for years, but never managed to pull myself together and actually implement it :-).
Please give it a try in your configuration if you have the time and let me know if something does not work.
-- Jan
On Sun, Oct 18, 2009 at 12:45 AM, Jan Janak jan@iptel.org wrote:
Module: sip-router Branch: master Commit: ed3902e9bd5a3af6ee5d5df08ce2564f028a0f24 URL: http://git.sip-router.org/cgi-bin/gitweb.cgi/sip-router/?a=commit;h=ed3902e9...
Author: Jan Janak jan@ryngle.com Committer: Jan Janak jan@ryngle.com Date: Sun Oct 18 00:39:31 2009 +0200
tm: Number of fixes in code and documentation for serial forking.
This patch improves the serial forking related code and documentation. The original code suffers from shortcommits, some of which were result of the migration to the new core.
The function that decodes destination sets encoded in AVPs has been improved, because the original version did not handle properly strings with missing elements. In such case the original implementation was likely to overwrite memory, because it did not check the return value of strchr properly. The new implementation tries to handle this situation. It continues parsing as long as it can, it only requires that the request-uri string is present, all other fields are made optional and their variables are properly initialized if their values cannot be found in the AVP.
There was a bug in the implementation of fr_inv_timer_next modparam in the original version, changes to the parameter value were ignored by the serial forking code. This was reported by Andrei and is now fixed by this commit. The parameter fr_inv_timer_next can now be configured at runtime with the configuration framework. Its value is in milliseconds and unlike fr_inv_timer, this timer cannot be configured separately for individual branches.
Obsolete definition of INV_FR_TIME_OUT_FIRST has been removed. That macro is not used anywhere in the code, thus it is not needed.
There were several places where LM_DBG printed a string and relied on the string being zero terminated, this may or may not be true in the future and this patch uses the macro STR_FMT where appropriate, which is safer.
Function t_next_contacts now checks if the function decode_branch_info really returned values for the dst_uri and the path vector. If not then it calls reset_dst_uri and reset_path_vector. Previous version of the code crashed sip-router, this is likely due to the merge and updates in the sip-router core.
We now use t_set_fr in t_next_contacts for setting the fr_inv_timer value to fr_inv_timer_next. This is much more efficient than creating AVPs with new timer values. Also the new value of the timer is now taken from a variable in the configuration framework, instead of just a regular global variable configured through modparam. This way we can adjust the value of the timer on the fly. Configuring it through modparam is, of course, possible too.
The value of of fr_inv_timer_next is now in milliseconds, instead of seconds. That's the only possibly incompatible change. However, this is consistent with all other timers in tm module, it is more efficient and it offers better granularity.
A missing call to destroy_avp has been added to t_next_contacts, in the code which is executed when no transaction exists. There, the avp should also be destroyed if all values have been exhausted and none of them had Q_FLAG set. This is a corner case which should not happen under normal circumstances, because that situation only happens if all branches have the same q value. Such AVP would not have been created by t_load_contacts and therefore t_next_contacts should not be called, but this bug may be triggered if someone uses t_next_contacts in an unexpected way and it is probably better to have it fixed.
Also the code which restores the value of fr_inv_timer at the end of t_next_contacts did not work properly. This patch fixes that. It first tries to retrieve a value configured with t_set_fr, but that is not guaranteed to succeed. After that it also tries the timer AVP and finally the configuration framework. The configuration framework always yields a value, so we can always restore the timer value, but we may fail to restore individual transaction timer values set by t_set_fr. If that fails then the global value from the configuration framework is used. This is documented as a shortcomming in the README and in the code.
In addition to code changes this patch also expands documentation on functions t_load_contacts and t_next_contacts, describing their operation in more detail. Also the format of the contacts AVP is now documented.
Finally, there is a whole new section in the README which describes how serial/parallel forking can be achieved with t_load_contacts and t_next_contacts and provides a number of examples.
modules/tm/README | 592 +++++++++++++++++++++++++++--------------- modules/tm/config.c | 7 +- modules/tm/config.h | 3 +- modules/tm/doc/functions.xml | 129 ++++++++-- modules/tm/doc/params.xml | 83 +++++-- modules/tm/doc/tm.xml | 218 ++++++++++++++++ modules/tm/t_serial.c | 208 ++++++++++------ modules/tm/tm.c | 2 +- 8 files changed, 900 insertions(+), 342 deletions(-)
Diff: http://git.sip-router.org/cgi-bin/gitweb.cgi/sip-router/?a=commitdiff;h=ed39...
sr-dev mailing list sr-dev@lists.sip-router.org http://lists.sip-router.org/cgi-bin/mailman/listinfo/sr-dev
Jan Janak writes:
This commit includes a number of changes I have done to the serial forking code in tm. The original version crashed sip-router and timers and other things did not work properly. I have tested the code and it seems to be working fine now, including all the things we discussed in other threads.
thanks jan for the very thorough work on t_serial.
There is only one incompatible change. The value of fr_inv_timer_next is now in milliseconds. The previous version used seconds, but it did not work in sip-router anyway. All other tm timers use milliseconds, so we are now consistent.
it is better that all timers use milliseconds, because it would be confusing if some would be in seconds and some in milliseconds.
I merged this commit also into sr_3.0 branch, because in addition to the new documentation this is all bugfixes. I think that this commit should also be merged into kamailio_3.0 branch, because the original code does not seem to work well with sip-router core.
let daniel do it because it was his idea to break the one core+tm model.
Please give it a try in your configuration if you have the time and let me know if something does not work.
i'll test it later today.
by the way, this encoding/decoding business in the code is ugly. if i remember correctly, AVPs can now have also structure values. if so, encoding/decoding could be avoided.
-- juha
On Sun, Oct 18, 2009 at 12:09 PM, Juha Heinanen jh@tutpro.com wrote:
[...]
by the way, this encoding/decoding business in the code is ugly. if i remember correctly, AVPs can now have also structure values. if so, encoding/decoding could be avoided.
Yes, there is certainly space for improvements. We could also improve the way how the serial forking code handles timers. Instead of configuring a static value with fr_inv_timer_next, we could compute the value of fr_inv_timer_next dynamically, based on the number of serial branches in the destination set and the value of max_inv_lifetime timer. Alternatively, we could also increase the value of max_inv_lifetime if serial forking is used in a transaction. The value of that timer is stored in the transaction, so it can be done selectively only for certain transactions. There are many possibilities how we can adjust timers automatically for serial forking...
-- Jan
Jan Janak writes:
Alternatively, we could also increase the value of max_inv_lifetime if serial forking is used in a transaction.
i tried that, but the problem is that caller's transaction times out anyway. so you must be able to try all branches before that happens.
-- juha
Jan Janak writes:
Please give it a try in your configuration if you have the time and let me know if something does not work.
jan,
after some bugs were fixed, i finally got so far that i was able to test serial forking. so far it has worked fine.
-- juha
-
Jan Janak -
Jan Janak -
jh＠tutpro.com