Hello
In cfg/cfg_ctx.c : 888 there is the definition of the function cfg_get_by_name which gets called in case of a "sercmd cfg.get" command
... if (var->def->on_change_cb) { /* The variable cannot be retrieved, because the fixup function may have changed it, and it is better to return an error than an incorrect value */ return 1; } ...
The "problem" is that if a use a fixup callback(or a on_change callback) , this function returns an error and the value cannot be read anymore via sercmd. What is the best way to proceed in this case? I am currently working on the modules_k/registrar module and if STATISTICS are used (default in kamailio build), I need to register some callbacks to be called when sercmd cfg.set_now_int (set_delayed_int) are called so I update kamailio's statistics framework with the new values passed from ser cfg framework. (for example min_expires, max_expires and others)
Is there a way for the cfg framework to specify that the callback doesn't change the value (like in the case above)?
Any ideas welcome.
Cheers Marius
On Feb 16, 2010 at 12:47, marius zbihlei marius.zbihlei@1and1.ro wrote:
Hello
In cfg/cfg_ctx.c : 888 there is the definition of the function cfg_get_by_name which gets called in case of a "sercmd cfg.get" command
... if (var->def->on_change_cb) { /* The variable cannot be retrieved, because the fixup function may have changed it, and it is better to return an error than an incorrect value */ return 1; } ...
The "problem" is that if a use a fixup callback(or a on_change callback) , this function returns an error and the value cannot be read anymore via sercmd. What is the best way to proceed in this case? I am currently working on the modules_k/registrar module and if STATISTICS are used (default in kamailio build), I need to register some callbacks to be called when sercmd cfg.set_now_int (set_delayed_int) are called so I update kamailio's statistics framework with the new values passed from ser cfg framework. (for example min_expires, max_expires and others)
You need this callback to be called the moment the value is set (1), in each process, before the updated config is used (2) or once, before the updated config is use the 1st time (by the first process that needs to access it) (3)?
If (2) is ok, then use the on_set_child_cb (next field in the cfg_def). If (3) is ok, then set CFG_CB_ONLY_ONCE among the flag and use the on_set_child_cb, like for (2).
Is there a way for the cfg framework to specify that the callback doesn't change the value (like in the case above)?
We could add a new flag (e.g. CFG_NOP_FIXUP or CFG_TRANSPARENT_FIXUP). So far I guess nobody had a fixup that didn't change the value :-)
Any ideas welcome.
In the long run we have to fix somehow the var. w/ fixup unreadable problem. Maybe we should add a cfg_on_display callback (but then we would have to update a lot of cfg_defs).
Andrei
Andrei Pelinescu-Onciul wrote:
On Feb 16, 2010 at 12:47, marius zbihlei marius.zbihlei@1and1.ro wrote:
You need this callback to be called the moment the value is set (1), in each process, before the updated config is used (2) or once, before the updated config is use the 1st time (by the first process that needs to access it) (3)?
If (2) is ok, then use the on_set_child_cb (next field in the cfg_def). If (3) is ok, then set CFG_CB_ONLY_ONCE among the flag and use the on_set_child_cb, like for (2).
Hello, What I really wanted was 1, when the cfg value was change the statistics are also updated. But I guess this can't be easy done (requires changes to cfg framework). As I only need the value changed once, and statistics aren't per process, I guess 3 is the better option.
I don't completely understand the whole "before the updated config is used". This means a cfg_update() is required or I don't have to do anything the update being done transparently from the user(myself) point of view. If I issue a sercmd cfg.set_now_int this will mean that the child_cb will be called at the time of the update(the value being changed now ?!) ?!
If I need to call cfg_update(), then I can call it from the "save" function export of the registrar module(and all other function exports called from ser cfg script). If this will make the child_cb to get called, then I can update the statistics from there. The window between the cfg value is changed and the changed is reflected to statistics is limited by the next call to a registrar function export when a sip messages is processed.
Another thing. Do I need to use cfg_update() periodically to ensure that values from cfg framework are consistent in the modules?! After looking at some examples in modules/tm I presumed that a call to cfg_get(...) is enough to get the updated value (Note that I don't have any child processes in the module - if there were children I would call cfg_update() from the infinite loop of the child).
So far I guess nobody had a fixup that didn't change the value :-)
I can think of two cases when the value is not changed/transformed
1. A change in cfg framework generates some internal module changes (like a reread of a database, update of statistics). I think this can be achieved via child_cb. 2. The fixup tests to see if the value is correct (like expecting a string from a already known lists of strings). No changes are possible and if the user gave smth wrong, sercmd will inform it at once . This can't be achieved via child_cb.
Thanks for the help
Marius
Any ideas welcome.
In the long run we have to fix somehow the var. w/ fixup unreadable problem. Maybe we should add a cfg_on_display callback (but then we would have to update a lot of cfg_defs).
Andrei
On Feb 16, 2010 at 14:41, marius zbihlei marius.zbihlei@1and1.ro wrote:
Andrei Pelinescu-Onciul wrote:
On Feb 16, 2010 at 12:47, marius zbihlei marius.zbihlei@1and1.ro wrote:
You need this callback to be called the moment the value is set (1), in each process, before the updated config is used (2) or once, before the updated config is use the 1st time (by the first process that needs to access it) (3)?
If (2) is ok, then use the on_set_child_cb (next field in the cfg_def). If (3) is ok, then set CFG_CB_ONLY_ONCE among the flag and use the on_set_child_cb, like for (2).
Hello, What I really wanted was 1, when the cfg value was change the statistics are also updated. But I guess this can't be easy done (requires changes to cfg framework). As I only need the value changed once, and statistics aren't per process, I guess 3 is the better option.
I don't completely understand the whole "before the updated config is used". This means a cfg_update() is required or I don't have to do anything the update being done transparently from the user(myself) point of view. If I issue a sercmd cfg.set_now_int this will mean that the child_cb will be called at the time of the update(the value being changed now ?!) ?!
You need to worry about cfg_update() only if you fork new processes and want to use the cfg framework from them. In general the cfg is updated for each "normal" process when a new message is read.
on_set_child_cb will be called by each process when its config needs updating (the global config pointer differs from the local one), so the answer to your question is "yes" (if no message comes, then no cfg_update() will happen for some time, so the callback might be called at a later time, at most TIMER_HANDLER_INTERVAL later).
Note also that if you want to use on_change_cb instead of on_set_child_cb, there is another issue. The cfg can be updated immediately, or on commit (e.g cfg.set_delayed_int and cfg.commit). However the cfg variable fixup (on_change_cb) is called immediately in both cases (because it's supposed to "fix" the value and not do some other stuff). So if you do something from on_change_cb, then it will probably break the cfg.set_delayed_int & cfg.commit case (the change will happen immediately and not at cfg.commit time). I had this bug in older versions for the sctp options. I used to do setsockopt()s from the fixup instead of the per child callback. Now they are done via the per child callback with the CFG_CB_ONLY_ONCE flag set (see sctp_option.c "autoclose" for an example)
If I need to call cfg_update(), then I can call it from the "save" function export of the registrar module(and all other function exports called from ser cfg script). If this will make the child_cb to get called, then I can update the statistics from there. The window between the cfg value is changed and the changed is reflected to statistics is limited by the next call to a registrar function export when a sip messages is processed.
cfg_update() is called each time a message is received and periodically by the timer. If you use (3) then you will have at most 1/16 s delay in the worst case. If this is not good enough for you, then I think we could add a call to cfg_update() at the end of cfg_commit() and cfg_set_now() (this would cause an update immediately after a commit or set_now and so it would trigger the immediate execution of the per child callbacks).
Another thing. Do I need to use cfg_update() periodically to ensure that values from cfg framework are consistent in the modules?! After looking at some examples in modules/tm I presumed that a call to cfg_get(...) is enough to get the updated value (Note that I don't have any child processes in the module - if there were children I would call cfg_update() from the infinite loop of the child).
Yes, you do have to call cfg_update() periodically if you want to make sure you have the latest config version for the non-atomic declared config vars. However cfg_update() is already called in all the processes (e.g.: each time a new message is received in the receiver processes and each timer tick in the timer process). In general it means that some of the new config values would be visible in a process only for future messages and not for the one that is currently being processed. This is a feature (it's a guarantee that any non-atomic-marked config var. would not suddenly change while you're processing a message) and updating more often will break several things (depending on the process, e.g. there are some tcp config variables that must not change while a message is processed).
So far I guess nobody had a fixup that didn't change the value :-)
I can think of two cases when the value is not changed/transformed
- A change in cfg framework generates some internal module changes
(like a reread of a database, update of statistics). I think this can be achieved via child_cb.
This one would also conflict with set_delays & commit if it would be done via the fixups.
- The fixup tests to see if the value is correct (like expecting a
string from a already known lists of strings). No changes are possible and if the user gave smth wrong, sercmd will inform it at once . This can't be achieved via child_cb.
Yes, this is a valid case. We will have to add another flag for this type of fixups.
Andrei
Hi,
On 02/16/2010 05:08 PM, Andrei Pelinescu-Onciul wrote:
On Feb 16, 2010 at 14:41, marius zbihlei marius.zbihlei@1and1.ro wrote:
Andrei Pelinescu-Onciul wrote:
On Feb 16, 2010 at 12:47, marius zbihlei marius.zbihlei@1and1.ro wrote:
You need this callback to be called the moment the value is set (1), in each process, before the updated config is used (2) or once, before the updated config is use the 1st time (by the first process that needs to access it) (3)?
If (2) is ok, then use the on_set_child_cb (next field in the cfg_def). If (3) is ok, then set CFG_CB_ONLY_ONCE among the flag and use the on_set_child_cb, like for (2).
Hello, What I really wanted was 1, when the cfg value was change the statistics are also updated. But I guess this can't be easy done (requires changes to cfg framework). As I only need the value changed once, and statistics aren't per process, I guess 3 is the better option.
I don't completely understand the whole "before the updated config is used". This means a cfg_update() is required or I don't have to do anything the update being done transparently from the user(myself) point of view. If I issue a sercmd cfg.set_now_int this will mean that the child_cb will be called at the time of the update(the value being changed now ?!) ?!
You need to worry about cfg_update() only if you fork new processes and want to use the cfg framework from them. In general the cfg is updated for each "normal" process when a new message is read.
on_set_child_cb will be called by each process when its config needs updating (the global config pointer differs from the local one), so the answer to your question is "yes" (if no message comes, then no cfg_update() will happen for some time, so the callback might be called at a later time, at most TIMER_HANDLER_INTERVAL later).
Note also that if you want to use on_change_cb instead of on_set_child_cb, there is another issue. The cfg can be updated immediately, or on commit (e.g cfg.set_delayed_int and cfg.commit). However the cfg variable fixup (on_change_cb) is called immediately in both cases (because it's supposed to "fix" the value and not do some other stuff). So if you do something from on_change_cb, then it will probably break the cfg.set_delayed_int & cfg.commit case (the change will happen immediately and not at cfg.commit time).
if the changes are committed at all. More changes can be prepared and committed at once, but they can also be revoked.
I have seen several times that the on_change_cb callback is confusing. Shall I rename it to something else, for example fixup_cb? So that the name would not indicate that the variable is changed.
I had this bug in older versions for the sctp options. I used to do setsockopt()s from the fixup instead of the per child callback. Now they are done via the per child callback with the CFG_CB_ONLY_ONCE flag set (see sctp_option.c "autoclose" for an example)
If I need to call cfg_update(), then I can call it from the "save" function export of the registrar module(and all other function exports called from ser cfg script). If this will make the child_cb to get called, then I can update the statistics from there. The window between the cfg value is changed and the changed is reflected to statistics is limited by the next call to a registrar function export when a sip messages is processed.
cfg_update() is called each time a message is received and periodically by the timer. If you use (3) then you will have at most 1/16 s delay in the worst case. If this is not good enough for you, then I think we could add a call to cfg_update() at the end of cfg_commit() and cfg_set_now() (this would cause an update immediately after a commit or set_now and so it would trigger the immediate execution of the per child callbacks).
Another thing. Do I need to use cfg_update() periodically to ensure that values from cfg framework are consistent in the modules?! After looking at some examples in modules/tm I presumed that a call to cfg_get(...) is enough to get the updated value (Note that I don't have any child processes in the module - if there were children I would call cfg_update() from the infinite loop of the child).
Yes, you do have to call cfg_update() periodically if you want to make sure you have the latest config version for the non-atomic declared config vars. However cfg_update() is already called in all the processes (e.g.: each time a new message is received in the receiver processes and each timer tick in the timer process). In general it means that some of the new config values would be visible in a process only for future messages and not for the one that is currently being processed. This is a feature (it's a guarantee that any non-atomic-marked config var. would not suddenly change while you're processing a message) and updating more often will break several things (depending on the process, e.g. there are some tcp config variables that must not change while a message is processed).
Indeed. So calling cfg_update() is necessary only if a new process is forked, otherwise do not call it.
So far I guess nobody had a fixup that didn't change the value :-)
I can think of two cases when the value is not changed/transformed
- A change in cfg framework generates some internal module changes
(like a reread of a database, update of statistics). I think this can be achieved via child_cb.
This one would also conflict with set_delays & commit if it would be done via the fixups.
- The fixup tests to see if the value is correct (like expecting a
string from a already known lists of strings). No changes are possible and if the user gave smth wrong, sercmd will inform it at once . This can't be achieved via child_cb.
Yes, this is a valid case. We will have to add another flag for this type of fixups.
The above two approaches can be used together: 1. Validate the value at the time it is fixed up (on_change_cb). 2. Apply any local changes from the on_set_child_cb.
In general, I see two problems with the cfg framework right now that could be improved, I appreciate any idea:
1) The problem Marius mentioned, i.e. those variables that have fixup function cannot be read via sercmd. The fixup function can perform quite simple tasks, for example data validation only, but it can also convert the values to some internal form that the framework is not aware of.
One possible solution would be to store also the original, not yet fixed up values and print them via sercmd. This is fine if the value was set via the framework, but the default values are not available in original form.
Another solution is to introduce a kind of reverse-fixup function that can convert the values back to human-readable form. This would require changes in the modules not only in the framework but the advantage is that really the value would be printed that is being used. For the simple cases we could have prepared reverse-fixups available that could re reused.
2) Some modules update local or even shared memory variables based on cfg variable changes. It would be nice to solve as many cases within the framework as possible. For example the modules could have write access to their variables during fixup, or more complicated structures could be stored not only int and str vars. If you miss any feature therefore you keep variables outside of the framework then please speak up!
Thanks, Miklos
Andrei
sr-dev mailing list sr-dev@lists.sip-router.org http://lists.sip-router.org/cgi-bin/mailman/listinfo/sr-dev
On Wednesday 17 February 2010, Miklos Tirpak wrote:
[..] 2) Some modules update local or even shared memory variables based on cfg variable changes. It would be nice to solve as many cases within the framework as possible. For example the modules could have write access to their variables during fixup, or more complicated structures could be stored not only int and str vars. If you miss any feature therefore you keep variables outside of the framework then please speak up!
Hi Miklos,
indeed this is another problem which we've also discussed during the planning for the cfg framework migration work, its present for example in the ratelimit and carrierroute modules.
Quite a few kamailio modules implemented also their own version of reload functionality with a FIFO command, e.g. "reload_fifo" in modules/carrierroute/cr_fifo.c. This is more or less a similar use case like the cfg framework represents (atomic reload of a variable), but with a a bit more complicated data structure (route_data_t).
So having a generic version for this in the cfg framework would be indeed nice in my opinion.
Cheers,
Henning
Henning Westerholt wrote:
On Wednesday 17 February 2010, Miklos Tirpak wrote:
[..] 2) Some modules update local or even shared memory variables based on cfg variable changes. It would be nice to solve as many cases within the framework as possible. For example the modules could have write access to their variables during fixup, or more complicated structures could be stored not only int and str vars. If you miss any feature therefore you keep variables outside of the framework then please speak up!
Hi Miklos,
indeed this is another problem which we've also discussed during the planning for the cfg framework migration work, its present for example in the ratelimit and carrierroute modules.
Quite a few kamailio modules implemented also their own version of reload functionality with a FIFO command, e.g. "reload_fifo" in modules/carrierroute/cr_fifo.c. This is more or less a similar use case like the cfg framework represents (atomic reload of a variable), but with a a bit more complicated data structure (route_data_t).
So having a generic version for this in the cfg framework would be indeed nice in my opinion.
Cheers,
Henning
Hello,
I have done some tests regarding the on_set_child_cb function (executed only once via CFG_CB_ONLY_ONCE or per each child process) and for what I want it does a good job. If the maximum delay is 1/16th of a second and above that, it will not create bugs like the one Andrei described in a previous mail, then I think this approach is good enough for the cases I identified.
Cheers, Marius
Hi Henning,
On 02/17/2010 11:22 AM, Henning Westerholt wrote:
On Wednesday 17 February 2010, Miklos Tirpak wrote:
[..] 2) Some modules update local or even shared memory variables based on cfg variable changes. It would be nice to solve as many cases within the framework as possible. For example the modules could have write access to their variables during fixup, or more complicated structures could be stored not only int and str vars. If you miss any feature therefore you keep variables outside of the framework then please speak up!
Hi Miklos,
indeed this is another problem which we've also discussed during the planning for the cfg framework migration work, its present for example in the ratelimit and carrierroute modules.
Quite a few kamailio modules implemented also their own version of reload functionality with a FIFO command, e.g. "reload_fifo" in modules/carrierroute/cr_fifo.c. This is more or less a similar use case like the cfg framework represents (atomic reload of a variable), but with a a bit more complicated data structure (route_data_t).
So having a generic version for this in the cfg framework would be indeed nice in my opinion.
the route_data_t structure seems to be the routing data similar to what a route block does in the script, I mean that it is not like a modparam. This is a bit far from the initial concept of the cfg framework but this would not be a problem.
In the framework, there are module and core parameters declared by the modules/core and there are cfg drivers that write the parameters. They can work on any backend, file, DB, srcmd... Hence, the drivers or the framework must know the structure of the configuration, i.e. what type of value is stored in which slot. The route_data_t structure is quite complex, the number of carriers and the number of domains is not known in advance, therefore the size of the structure is also unknown. The framework could hardly tell what is stored on a given memory address. The best we could do is to let the carrierroute module write its own configuration and do not touch it by other modules, I mean by the cfg drivers.
Actually the framework already supports the void* type that could be used for this purpose (with some enhancements). The module could load the config, allocate the necessary space in shm mem, and link it to the global configuration via the void* pointer. From this point the framework could offer:
- consistency: The config would remain the same during a SIP message processing. Even if there is a new config available, it would be used only for the next message routeing.
- locking: The carrierroute module should not care about locking the data structures during reading because it is the task of the framework. In fact, the framework supports lockless read because all modifications (except with the atomic flag) copy the entire config block first and modify the clone. This is done for performance reasons because the assumption is that the config rarely changes but it is read many times. This means that even the carrierroute module is not allowed to simply write its data structures. It should always construct a new structure (or clone the existing one) and link the new configuration to the global cfg structure via some interface.
So I do not see to much benefit of using the cfg framework in this case because the structure that needs to be stored does not have a fixed size, therefore it is hard to specify for the framework. In this case the carrierroute module would be the only one that can read/write the config which is nothing different from the current situation. Or do you think that the consistency and locking are worth the effort?
Regards, Miklos
Cheers,
Henning
On Monday 22 February 2010, Miklos Tirpak wrote:
[..] So having a generic version for this in the cfg framework would be indeed nice in my opinion.
Hello Miklos,
thanks for the reply.
the route_data_t structure seems to be the routing data similar to what a route block does in the script, I mean that it is not like a modparam. This is a bit far from the initial concept of the cfg framework but this would not be a problem.
Yes, its indeed a more complicated data structure.
In the framework, there are module and core parameters declared by the modules/core and there are cfg drivers that write the parameters. They can work on any backend, file, DB, srcmd... Hence, the drivers or the framework must know the structure of the configuration, i.e. what type of value is stored in which slot. The route_data_t structure is quite complex, the number of carriers and the number of domains is not known in advance, therefore the size of the structure is also unknown. The framework could hardly tell what is stored on a given memory address. The best we could do is to let the carrierroute module write its own configuration and do not touch it by other modules, I mean by the cfg drivers.
Yes, this is true. It would probably not make too much sense to let other modules tinker in this informations, a generic reload function would be more meaningful in this case.
Actually the framework already supports the void* type that could be used for this purpose (with some enhancements). The module could load the config, allocate the necessary space in shm mem, and link it to the global configuration via the void* pointer. From this point the framework could offer:
- consistency: The config would remain the same during a SIP message
processing. Even if there is a new config available, it would be used only for the next message routeing.
- locking: The carrierroute module should not care about locking the
data structures during reading because it is the task of the framework. In fact, the framework supports lockless read because all modifications (except with the atomic flag) copy the entire config block first and modify the clone. This is done for performance reasons because the assumption is that the config rarely changes but it is read many times. This means that even the carrierroute module is not allowed to simply write its data structures. It should always construct a new structure (or clone the existing one) and link the new configuration to the global cfg structure via some interface.
Ok, i understand. This is more or less the same way the module works at the moment.
So I do not see to much benefit of using the cfg framework in this case because the structure that needs to be stored does not have a fixed size, therefore it is hard to specify for the framework. In this case the carrierroute module would be the only one that can read/write the config which is nothing different from the current situation. Or do you think that the consistency and locking are worth the effort?
Good question. In cr we handle it in a way that even the module does not write into this datastructure, and there is only one place which updates it under a lock. (Writing to the route data during runtime would be a problem for the performance as the datastructure is somewhat bigger.) So if its not really fit into the (existing) framework and we don't really benefit from the added consistency at the moment, the only real reason to use a central functionality would be to have a unified interface for reloads across several modules.
Regards,
Henning
-
Andrei Pelinescu-Onciul -
Henning Westerholt -
marius zbihlei -
Miklos Tirpak