[Home]

Summary:ASTERISK-03978: [patch] rtcache textual clearup attempt
Reporter:drmac (drmac)Labels:
Date Opened:2005-04-24 22:55:44Date Closed:2008-01-15 15:45:25.000-0600
Priority:TrivialRegression?No
Status:Closed/CompleteComponents:Core/General
Versions:Frequency of
Occurrence
Related
Issues:
Environment:Attachments:( 0) sip_clearups_take2.patch.txt
( 1) sip_clearups_take5.patch.txt
Description:The main reason for this patch was because the option "rtnoupdate" required a value of "no" in order to actually get updates. Gave me a 6th grade english-double-negative headache.

patch changes name to "rtupdate" so that the values of "yes" mean "do updates" and "no" means "don't do updates".

patch also clears up (hopefully) the comments surrounding the rtcache section of sip.conf

****** ADDITIONAL INFORMATION ******

Disclaimer on file
mboehm@cytelcom.com
Comments:By: Kevin P. Fleming (kpfleming) 2005-04-26 22:38:02

I think the idea of it being called 'rtnoupdate' was that it was an option to turn off the default behavior (which is to send updates). While I agree that the double-negative is not the best arrangement, it actually does describe what the admin is trying to do (disable updates, which are enabled by default).

Unless anyone else can jump in here and make atrong case for changing the sense of the option name, I'd prefer to just leave it as-is and update the documentation to make its usage clearer.
By: opsys (opsys) 2005-05-04 16:48:28

I support the patch. Double Negatives are confusing. Think non-english speaking persons. Also the documentation is already lacking and anything that can avoid that is welcome.
By: Christopher L. Wade (clwade) 2005-05-04 17:13:50

I agree that it should be changed to rtupdate.  Just change the documentation to note that 'rtupdate=yes' is the default and to turn off the default behavior requires you to say 'rtupdate=no'.  Hate to be a me too, but double negatives are bad things.
By: Mark Spencer (markster) 2005-05-04 23:27:30

Please note that the patch also reverses the default behavior which is a BAD THING(tm).

How about "rtomitupdate" or something
By: drmac (drmac) 2005-05-05 10:48:15

no it doesn't. (rather, it wasn't intended to)

the default is "send updates". my patch doesn't change that.

if you don't want updates, set rtupdate = no
By: drmac (drmac) 2005-05-23 09:52:46

any updates on this?
By: Russell Bryant (russell) 2005-05-23 18:33:01

This patch does indeed change the default behavior.

Before, if the 'rtnoupdate' option was not specified, the default value is 'no'.

Now, if the 'rtpupdate' option is not specified, the default is still 'no', and this changes the default behavior.

To avoid unnecessary compilcations in the code, the best thing to here, as previously stated by kpfleming and markster, is to either leave it alone or change it to another name that isn't as painful, such as 'rtomitupdate'.
By: drmac (drmac) 2005-05-24 09:18:19

can't i just change the patch to reflect the original behavior?
By: Clod Patry (junky) 2005-06-20 19:07:48

Patch must has no effect on backward compatibility (except for major cases, which is not the case here).
Like drumkilla said, your patch needs to dont change the default behavior, cause it's gonna break backward compatibility for a lot of people.
By: drmac (drmac) 2005-06-20 20:39:01

ok. i think i see the problem. i removed an '!' from the code and that changes behavior. the default behavior is to send updates because if the SIP_PAGE2_RTNOUPDATE flag is not present then the ast_test_flag returns false but the ! makes it true so the updates are sent.

this new patch should be better..
By: Michael Jerris (mikej) 2005-06-23 06:19:29

kevin- ready to review?
By: Olle Johansson (oej) 2005-07-20 12:43:22

Housekeeping reminder to everyone involved...

/O
By: Clod Patry (junky) 2005-08-22 22:09:53

sip_clearups_take2.patch.txt  breaks backward compatibility.
What if some users has rtnoupdate=foo in config?
By: drmac (drmac) 2005-08-23 08:08:57

you want a deprecated warning?
By: Kevin P. Fleming (kpfleming) 2005-08-23 11:10:14

No, there is no reason for a deprecation warning, this is a CVS HEAD only feature at this point. Users of CVS HEAD are expected to keep up to date with backwards-incompatible changes that occur during development, when we decide to commit one.

What is the status of this patch today?
By: Michael Jerris (mikej) 2005-08-23 11:14:51

logic looks good to me.  the second patch should be good to go.
By: Kevin P. Fleming (kpfleming) 2005-08-23 11:28:29

I don't see how this can be right... let me try to explain:

The current behavior defaults to sending updates, unless the SIP_PAGE2_RTNOUPDATE flag is set on the peer.

The new code is supposed to default to also sending updates, and not send them if the SIP_PAGE2_RTUPDATE flag is _NOT_ set on the peer. However:

1) The code currently checks to see if the RTUPDATE flag is set, and then inverts it to decide to do the update; this is backwards.

2) The RTUPDATE flag is not set on any peers by default, so unless someone puts rtupdate=yes in their config file, they won't get any updates.
By: drmac (drmac) 2005-08-23 12:09:55

The only difference in the code patch between version 1 and version 2 is the absence of "!". Version 1 has it, version 2 doesnt.

Re: Kevin's post

 1) mark stated above that if the "!" is present (ver 1) on the line in question, it changes default. Now you say that when it is absent (ver 2) it is backwards. Which of you is correct?

 2) If the default behavior of chan_sip is to send updates, then can't we simply uncomment the rtupdate=yes in the sample config file?
By: Michael Jerris (mikej) 2005-08-23 12:47:47

you need to do a little more than uncomment in the sample config, or else it will change for people who currently have config files.  you need to acutally set up a default value in code.
By: Kevin P. Fleming (kpfleming) 2005-08-23 13:18:31

The preceding comments about the logic were not necessarily correct, they talk about a lot of different things. Let's make it simple: in the absence of rtupdate=no being set on a peer, update_peer() needs to cause the update to happen. As Mike has commented, since people have existing configs (so changing the sample config will do no good for them). If the default behavior is for updates to happen, then the code will need to set RTUPDATE by default until (if) it is overwritten in the config. It also must set the flag again on reload, in case 'rtupdate=no' has been removed from the config file.

I do agree that the option name should be changed, I just don't want to break existing configs that were relying on default behavior. Existing configs that were explicitly setting rtnoupdate=yes can be broken for a short time until they update to rtupdate=no (and I will post an announcement to asterisk-dev when this patch is committed).
By: drmac (drmac) 2005-08-24 13:54:18

I'm looking thru the most recent chan_sip code and I don't see rtnoupdate as being a per-peer option. (ie. not in build_peer()) I only find the checking of its existance in the sip.conf "general" section.

Let me make sure I get this right before I start going again:

If rtupdate is not specified in the config, it should default to yes. (Meaning the flag must be copied to all peers.)

If rtupdate is specified with yes, do same as above.

If rtupdate is specified with no, don't set any flags.
By: drmac (drmac) 2005-08-24 14:30:40

OK. sip_clearups_take3.patch.txt should add SIP_RTUPDATE as a "true" global flag before the config is ever read. Then when the config is read, if there is no rtupdate present, the default is still there. If it is present and set to no, the flag is changed.
By: drmac (drmac) 2005-08-24 14:31:15

crap..delete take3..it didnt have the config.sample stuff in it. use take4
By: Michael Jerris (mikej) 2005-08-24 14:35:09

deleted take 3, ready for the file.
By: drmac (drmac) 2005-08-24 15:50:56

*sigh* forgot that flag was on page 2.

use take5
By: Kevin P. Fleming (kpfleming) 2005-08-24 22:25:53

Committed to CVS HEAD, thanks!
By: Digium Subversion (svnbot) 2008-01-15 15:45:25.000-0600

Repository: asterisk
Revision: 6401

U   trunk/channels/chan_sip.c
U   trunk/configs/sip.conf.sample

------------------------------------------------------------------------
r6401 | kpfleming | 2008-01-15 15:45:25 -0600 (Tue, 15 Jan 2008) | 2 lines

clean up rtcaching config options (issue ASTERISK-3978)

------------------------------------------------------------------------

http://svn.digium.com/view/asterisk?view=rev&revision=6401