Summary:	ASTERISK-07669: [patch] reload does not initialize configuration values
Reporter:	Tzafrir Cohen (tzafrir)	Labels:
Date Opened:	2006-09-04 04:48:15	Date Closed:	2007-01-18 19:01:49.000-0600
Priority:	Minor	Regression?	No
Status:	Closed/Complete	Components:	Channels/chan_zap
Versions:		Frequency of Occurrence
Related Issues:
Environment:		Attachments:	( 0) cha_zap_chan_conf_1.2.dpatch ( 1) chan_zap_cha_conf_5_1.4.diff ( 2) chan_zap_chan_conf_1.diff ( 3) chan_zap_chan_conf_2.diff ( 4) chan_zap_chan_conf_3.diff ( 5) chan_zap_chan_conf_4.diff ( 6) chan_zap_chan_conf_5_trunk.diff ( 7) chan_zap_chan_conf_6_trunk.diff ( 8) chan_zap_chan_conf_7_trunk.diff
Description:	chan_zap.c originally only read the configuration file on startup and not on reload. It fails to re-initialize configuration variables on reload. The following configuration file demonstrates how a channel will get different configuration on reload than on a normal startup. Consider the following zapata.conf for a system that has an FXS channel on zap channel 1: [channels] signalling=fxo_ks context=from-internal channel => 1 immediate=yes On initial load, the line immediate=yes will have no effect on the channel. However it will set the value of the internal variable "immediate" that is used when reading the configuration. Upon reload, the value of immediate will be "yes" and thus the channel will be set to the broken immediate=yes. ****** ADDITIONAL INFORMATION ****** Sorry for not testing this on a later version. But upon initial inspection, the relevant code in both 1.2 branch and trunk seems to be unchanged. Please try that simple test.
Comments:	By: Tzafrir Cohen (tzafrir) 2006-09-05 14:05:17 Uploaded an example patch vs. current trunk (r42034). It is totally untested, as I so far don't have a zaptel trunk on my system yet, though. Generally that patch creates a struct zt_chan_conf that is generated at the beginning of zap_setup and holds all the state variables that were listed as globals. It is passed (by value) to mkintf(). This patch only puts four of those variables there: 3 globals and one which was local to zap_setup . This has saved passing three separate parameters to mkintf(). Patch is disclaimed. While writing this text I noticed some problms with the posted patch. Uploading a second file with a version of the patch that is closer to relaity. By: Serge Vecher (serge-v) 2006-09-05 14:18:15 looks like there are some tabulation issues in the patch ... By: Stefano Brandimarte (stevens) 2006-09-05 18:39:10 tzafir: could You please look also at the "overlapdial" variable, that's valid just per span config. So if we've something like: overlapdial = yes channel => 1-10 overlapdial = no channel => 11-15,17-21 the latter will win and the whole span (channels 1 to 24 on a T1) will have the overlapdial set to 0 (no). I don't know why it isn't checked by mkintf, but it absolutely requires a fix. By: Tzafrir Cohen (tzafrir) 2006-09-05 23:03:07 This is a slightly different issue, but another note: overlapdial is defined as int in the global configuration and in struct zt_pri. However it is defined as an unsigned in in struct zt_pvt . What happens if the value -1 ever gets into that variable? There's a whole set of variables for which this applies. By: Tzafrir Cohen (tzafrir) 2006-09-06 00:41:18 Update to the suggested patch: * fixed the indentation issue serge-v mentioned. * I noticed that when generating the pseudo channel, some global variables get overriden. So I have added them to the struct and passed a new default config structure. It is possible to set some fields in it beforepassing it to mkintf(), should this be required. By: Tzafrir Cohen (tzafrir) 2006-09-06 00:42:54 I forgot to mention this in the previous note: I stress again that I have not yet event tested that patch to build. By: Stefano Brandimarte (stevens) 2006-09-06 04:37:48 tzafir: if You implement a correction setting to -1 the overlapdial variable (it's the same fix i would have implemented) i can arrange a check for that to adapt the code accordingly. See also http://bugs.digium.com/view.php?id=7511 and the comment ASTERISK-4951848. The zt_pvt unsigned int appears to be not used at all. By: Tzafrir Cohen (tzafrir) 2006-09-10 23:47:01 Updated patch to rev 42671. Could someone please set the details to "trunk/42671"? Other Changes: * Now struct zt_chan_conf also includes the default context and the caller ID (num/name). * Using AST_MAX_EXTENSION (80 chars) for caller ID num and name, as in the channel struct, and not 256 as originally. By: Mark Spencer (markster) 2006-09-11 07:02:46 Amazingly, there used to be code which initialized all these parameters, yet that code seems not only not to have been adjusted to cover all the parameters but appears to have actually been pulled out.  This patch is a step in the right direction, but it needs to include *all* the parameters in chan_conf, not just a subset of them.  Furthermore, this would solve an important issue for me in usersconf, because it would mean that I could end up with a channel_conf that is reset to what it was at the end of zapata.conf each time i "reload" the individual user channels. Also, please note that it is intentional that mkintf for the CHAN_PSEUDO *not* reset other parameters (save the group) so that it inherits any other qualifiers at the end of the file if it's not defined. By: Tzafrir Cohen (tzafrir) 2006-09-11 14:42:55 Some parameters, e.g. "language" get applied in zt_new at call creation time. This means that they are global and not per-channel. I'm leaving them be for the moment. By: Tzafrir Cohen (tzafrir) 2006-09-13 00:55:54 Uploaded patch vs. trunk rev 42878 (could somebody please update version info of the bug?) * Patch seems to compile. Not yet tested in a working system... * Now changes all static variables that are only set through mkintf() . * A separate struct for PRI related values that are set to the span, are they actually affect per-channel. Maybe the correct name would be "span" rather than "pri"? * I put the documentation near the defult values rather than near the declarations. Which would you prefer? * Added a number of small documentation comments, just for fun. * Some fields were not initialized. I added explicit initializations everywhere. I believe that this is OK to assume an implicit initialization to 0 for a statically-allocated struct (set to 0 by-definition). Not sure about struct allocated on the stack (as it is today) or dynamically (as it may be in the future. By: Tzafrir Cohen (tzafrir) 2006-10-04 16:42:41 Backport of the patch to 1.2 (still untested. Colliteds with a few patches I need :-(  ) By: jmls (jmls) 2006-11-04 02:17:46.000-0600 has anyone managed to test this in 1.2 and trunk ? Thanks By: gkloepfer (gkloepfer) 2006-11-06 13:18:56.000-0600 Note that issue in http://bugs.digium.com/view.php?id=8296 that tzafrir and I briefly discussed is indirectly related to this work (the patch mentioned affects more than just chan_zap, but its primary purpose is due to the way zapata.conf is parsed). By: Jason Parker (jparker) 2006-11-28 15:03:30.000-0600 tzafrir, I'm fairly certain you have a disclaimer, but you didn't mark it on this bug.  Can you verify that you have one on file? By: Tzafrir Cohen (tzafrir) 2006-11-28 20:30:46.000-0600 qwell: yes, all atachments here are disclaimed. By: Tzafrir Cohen (tzafrir) 2007-01-09 15:23:14.000-0600 Uploaded a new (and disclaimed) version of the patch. This one is for 1.4. A version for trunk is due shortly. Changes: 1. To aovid useless duplication, the configuration structure is now a struct that has three stadard chan_zap/zaptel structs (and one extra value). This means that almost all the translation is done at parsing time and need not repeat twice. The configuration variable no longer represents a channel config and renamed to "conf". 2. Not explicitly initializing things to 0, Again, saving much useless duplication. (TODO: do I need some explicit memset-s?) 3. The addition of zaptel setup through users.conf made things a bit more complex. The conf is defined in setup_zap and is now passed by reference to process_zap. It is still passed by-value to build_channels(). 4. process_zap does not call build_channels directly: I changed the meaning of "zapchan" to be "channels, that are defined in the end of the section". TODO: prevent different sections from affection each other (except the [channels] section of zapata.conf and the [general] section of users.conf). By: Tzafrir Cohen (tzafrir) 2007-01-16 14:55:02.000-0600 another note, which may or may not be worth a separate bug report: progzone is currently set globally, rather then per-channel By: Tzafrir Cohen (tzafrir) 2007-01-16 15:55:29.000-0600 Uploaded chan_zap_chan_conf_5_trunk.diff: a version for current trunk (51555) By: Tzafrir Cohen (tzafrir) 2007-01-17 09:52:33.000-0600 uploaded chan_zap_chan_conf_6_trunk.diff (vs. 51555) . Basically the same as chan_zap_chan_conf_5_trunk.diff, except that different zaptel sections of users.conf now don't affect each other. By: Tzafrir Cohen (tzafrir) 2007-01-17 23:54:54.000-0600 chan_zap_chan_conf_7_trunk.diff: A version implementing basically what we discussed on asterisk-dev . vs. svn trunk 51217 (two previous patches were vs. 51155, not 51555, sorry) The keyword "zapchan" has become an equivalent of "channel". skipchannels has been renamed to latechannels. When it is set, 'channel' (and 'crv') are only processed in the end of the process_zap loop. After processing users.conf (if it exists), all sections of zapata.conf (besides "channels", "trunkgroups" and "general" - a name I prefer to reserve) are processed in the same way. Bug: users.conf will not be processed if zapata.conf does not exist at all. An empty zapata.conf must exist. By: Dwayne Hubbard (dhubbard) 2007-01-18 19:01:47.000-0600 committed to branch 1.2 in revision 51271, branch 1.4 in revision 51272, and trunk in revision 51273.  If you have additional things to add that are not part of this bug, please open a new issue and provide a new patch.  Thanks! :-)