[Home]

Summary:ASTERISK-03564: [patch] Changes to devstate, message and subscribe
Reporter:Olle Johansson (oej)Labels:
Date Opened:2005-02-22 14:41:29.000-0600Date Closed:2008-01-15 15:46:06.000-0600
Priority:MajorRegression?No
Status:Closed/CompleteComponents:Channels/chan_sip/Interoperability
Versions:Frequency of
Occurrence
Related
Issues:
Environment:Attachments:( 0) ast_build_string_rev1.diff.txt
( 1) sipdiverse.txt
( 2) subscribe.txt
( 3) subscribe2.txt
( 4) subscribe-20050428.diff.txt
( 5) subscribe3.txt
( 6) subscribe-merged-with-3882.diff.txt
( 7) subscribe-merged-with-3882.diff.txt
( 8) polycom-sub.txt
( 9) polycom-sub-debug.txt
(10) sip3644-0828.txt
(11) sip3644-0829.txt
(12) sip3644-0829b.txt
(13) sip3644-0829c.txt
(14) sipsubscribe_polycom_support.txt
(15) sipsubscribe0610.txt
(16) sipsubscribe-20050622.rev768.txt
(17) sipsubscribe-20050623.rev770.txt
(18) sipsubscribe-20050627.rev771.txt
(19) sipsubscribe-20050706.rev774.txt
(20) sipsubscribe-20050708.rev775.txt
(21) sipsubscribe-20050715.rev779.txt
(22) sipsubscribe-20050718.rev781.txt
(23) sipsubscribe-20050720.rev783.txt
(24) sipsubscribe-20050725.rev787.txt
(25) sipsubscribe-20050725.rev789.txt
(26) sipsubscribe-20050726.rev792.txt
(27) sipsubscribe-20050726.rev792v2.txt
(28) sipsubscribe-20050726.rev796.txt
(29) sipsubscribe-20050729.rev797.txt
(30) sipsubscribe-20050801.rev797.txt
(31) sipsubscribe-20050803.rev801.txt
(32) sipsubscribe-20050812.rev806.txt
(33) sipsubscribe-20050812.rev806v2.txt
(34) sipsubscribe-20050823.rev813.txt
(35) sipsubscribe-20050824.rev814.txt
(36) sipsubscribe-20050824.rev814v2.txt
(37) sipsubscribe-20050825.rev820.txt
(38) sipsubscribe-20050825.rev820v2.txt
(39) sipsubscribe-20050829.rev822.txt
(40) subscribe0824.txt
(41) subscribe-20050531.diff.txt
Description:This patch
* Adds changes to devicestate
 Checking peer status based on incominglimit and qualify=
* Adds third XML format for SUBSCRIBE notifications
 for xten Eye-beam
* Improving SUBSCRIBE handling
 - Checking subscription Event: header
 - Generating error message if we do not support
   event package
 - Authentication as REGISTER (www-auth)
 - Fixes bugs in "sip show subscriptions"

Disclaimer on file

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

This patch does not change current behaviour, but improves handling of unknown SUBSCRIBE event packages (error message), adds a new event notification format and fixes small errors in the way we receive MESSAGEs.

Comments:By: Olle Johansson (oej) 2005-02-27 11:52:01.000-0600

Adds a change to notification so we will notify "closed" (or unavailable) if the hinted devices aren't registred.

Also, added a "subscribecontext" setting to set context for subscriptions. With that change, subscriptions suddenly work for me.

This works beautifully with Eye-beam now, SIP peer notification works. With the IAX2 devicestate addition (in separate bug report) I can watch the state of IAX peers also, from SIP clients.

By: Olle Johansson (oej) 2005-02-28 12:45:41.000-0600

The IAX2 devicestate addition is now included in CVS.

By: Mark Spencer (markster) 2005-03-02 00:14:11.000-0600

This XML stuff has got to go.

By: Olle Johansson (oej) 2005-03-02 12:21:14.000-0600

OK, will clean this up and retry within a week.

By: Olle Johansson (oej) 2005-03-09 01:06:49.000-0600

New cleaned-up version uploaded.

By: Olle Johansson (oej) 2005-03-13 09:26:39.000-0600

markster: Did you take a look at this?

By: Olle Johansson (oej) 2005-03-20 03:00:59.000-0600

Updated to current CVS

By: Mark Spencer (markster) 2005-03-20 10:14:30.000-0600

The only really serious gotcha here is that snprintf does not behave the way you think it does (and the way that would make sense).  Here's a snippet from the snprintf man page:

 Return value
      Upon successful return, these functions return the number of characters printed (not including the trailing ’\0’
      used to end output to strings).  <i>The functions snprintf and vsnprintf do not write more than size bytes (includ-
      ing the trailing ’\0’).  If the output was truncated due to this limit then the return value is  the  number  of
      characters  (not  including the trailing ’\0’) which would have been written to the final string if enough space
      had been available</i>. Thus, a return value of size or more means that the output was truncated.  (See  also  below
      under NOTES.)  If an output error is encountered, a negative value is returned.

(italic attempted to be added to the important section). So, in other words, you actually have to call strlen on the result (or at least confirm the return size is not more than the size you permitted) in order to properly do this.  Thanks!

By: Kevin P. Fleming (kpfleming) 2005-03-20 10:42:09.000-0600

Since there are so many calls to snprintf here, it may make sense to just create a wrapper function that does the appropriate bits:

- check the return value and increment the pointer and remaining size appropriately (either the return value or the remaining size, whichever is smaller {no need for strlen})

- check for snprintf failure (return value less than zero) and handle as best it can be

To do this correctly, the wrapper function will need attribute((printf)) applied, just like ast_log and friends.

By: Olle Johansson (oej) 2005-03-20 11:10:05.000-0600

Hmm. Those are just copy'n'paste from existing code, nevertheless needs to be fixed. I'll look into it. Thank you for the information!

By: Olle Johansson (oej) 2005-03-20 11:19:05.000-0600

Whoa, the whole code is buggy - we're always sending the same maxsize to snprintf, even though we already used part of the allocation. Ugh. Needs serious redesign. Look what can happen when you start messing with old code...

By: Kevin P. Fleming (kpfleming) 2005-03-20 11:32:52.000-0600

No, that's not true... maxbytes is being decreased after each call to snprintf.

I'll post a patch to add an ast_build_string() function in a few minutes.

By: Kevin P. Fleming (kpfleming) 2005-03-20 11:55:52.000-0600

Small patch (disclaimer on file) to add ast_build_string() to API.

For uses where the potential error result from snprintf was already being ignored, the result from ast_build_string can also be ignored.

By: Olle Johansson (oej) 2005-03-21 00:32:21.000-0600

kpfleming: Thanks, it works great. I have some minor mods to do, then I'll upload again.

By: maik (maik) 2005-03-31 12:42:12.000-0600

I tried to merge my own patch (3882) with this one as you suggested. It looks like everything is still working as expected but I cannot guarantee that your changes are still working correctly because I don't have anything to test it with. But since my own changes are only minor I don't expect any problems.

You should the lines that add mfrom and mto to message because in the dialog-info part the wrong one was used. Maybe you have the same bug. I think nobody noticed this because at least the snom phones seem to ignore it. :)

By: maik (maik) 2005-03-31 18:57:37.000-0600

Fixed a small bug with the new config option.

By: Frank Sautter (xylome) 2005-04-21 10:38:39

feedback:
patch 2005-03-31 18:57 applied to cvs-head on 2005-04-19 (needed some manual patching).
several thousand calls since then with no problems - seems stable.
behaviour of AST_EXTENSION_UNAVAILABLE should be user configurable.... i would prefer 'terminated' instead of 'confirmed'.

edited on: 04-28-05 10:43

By: Frank Sautter (xylome) 2005-04-28 10:59:07

updated patch to cvshead 20050428
added Message-Account according to RFC3842 to the MWI NOTIFY message to make the "Retrieve"-Button on the snom phones work correctly.
if nothing other is specified 'asterisk@_mydomain.tld_' is used as Message-Account e.g.: Message-Account: sip:asterisk@levigo.de\r\n.
for each sip peer another dialplan extension could be used for accessing the voicemail e.g.: Message-Account: sip:franksvoicemail@levigo.de\r\n. to make this work 'mailboxexten=franksvoicemail' has to be set in sip.conf or realtime.
i'm not very happy with the parameter name mailboxexten but could not think of any other name for it. maybe someone has a better idea.

By: Olle Johansson (oej) 2005-04-28 14:35:40

OK, I'm back on track and will take a look at this patch again after the changes made to it. Sorry for being off line for a while. A mean bug and a lot of travels has been in the way... :-)

By: Kevin P. Fleming (kpfleming) 2005-05-15 18:27:57

I have committed the ast_build_string() library function from this bug, but the rest is waiting on an update from the poster.

By: Olle Johansson (oej) 2005-05-16 00:52:51

Ok, will take a look at this again this week.

By: Frank Sautter (xylome) 2005-05-31 07:19:46

updated patch to cvshead 20050531

By: Olle Johansson (oej) 2005-06-10 15:04:41

xylome: Please check that my updated patch includes your code in a correct way. I've updated the code quite a bit to make it simpler. It works very nicely with Eye-beam now.

By: Frank Sautter (xylome) 2005-06-22 13:06:04

seems as you included my code correctly.
i updated the patch to the current cvs-head (what a pain) and fixed some minor discrepancies beetween older versions of this patch and your 20050610 patch (e.g. the configfile option 'notifyringing' was missing).
so far the code is working fine with my snom360.
i think this patch should make it (finally) to the cvs.

By: Kevin P. Fleming (kpfleming) 2005-06-22 17:50:48

First off, this looks like a good patch, even though it's large :-) I do have some code review notes, though:

1) 'mailboxexten' is definitely the wrong name... 'vmexten' might be better, although I think you should solicit opinions on asterisk-dev as well. Also, there needs to be a global setting for this as well, so it doesn't have to specified per-peer if it's the same for all of them.

2) Please create an enum for the p->subscribed values with proper names for each of the four possible values, and use a switch statement instead of chained ifs to process them when building the response packet.

3) This code is repeated three times:

t = tmp;
maxbytes = sizeof(tmp);

4) transmit_notify_with_mwi could use a single string buffer and ast_build_string(), instead of three buffers.

5) This seems confusing, to say the least:

+ /* Should we respond somehow? */
+ transmit_response(p, "202 Accepted", req);
+ /* We should respond 202 accepted, since we relay the message */

6) What is the reason for this change?

- peer = malloc(sizeof(*peer));
+ peer = malloc(sizeof(struct sip_peer));
if (!peer)
return NULL;

- memset(peer, 0, sizeof(*peer));
+ memset(peer, 0, sizeof(struct sip_peer));

7) sip.conf.sample will need to be updated to reflect the usage of these new configuration options.

By: Frank Sautter (xylome) 2005-06-23 05:42:36

* patch sipsubscribe-20050623.rev770.txt updated to todays rev 770 of chan_sip
* implemented all of kevin's advisories (exept 6)
* included the patches of the other files affected
* removed the rest of the strcpys
* minor whitepace cleanup (hope nobody kills me for this, but i stumbled over them and i could not resist)



By: Frank Sautter (xylome) 2005-06-23 05:47:46

i'm not shure why you changed '*peer' to 'struct sip_peer' - to my knowledge the result is the same.
maybe you could explain this to kevin.

i think it should remain as '*peer', as it would be less error prone

please review this patch so it could make it to cvs soon (i'm not very happy keeping such a big patch up to date :-/).



By: Frank Sautter (xylome) 2005-06-27 04:21:56

* patch sipsubscribe-20050627.rev771.txt updated to todays rev 771 of chan_sip
* i searched the source and found another occurence of a malloc of 'struct sip_peer' => i changed both occurrences now to 'peer = malloc(sizeof(*peer))' as i think it's less error proune.


By: Frank Sautter (xylome) 2005-07-07 06:35:14

updated to revision 774 of chan_sip.c.
is there a chance to have this commited to CVS soon? it's really time consuming to keep this big patch up to date.

By: Frank Sautter (xylome) 2005-07-08 03:32:37

updated to cvs head 2005-07-08 (revision 775 of chan_sip.c)
could you please review this code once again?

By: Frank Sautter (xylome) 2005-07-15 10:53:52

this patch gets really, really big now. this is due to the fact that i'm extending sip functionality more and more and there it would be very time expensive if possible at all to take this apart.
new things in this patch:
* updated to revision 779 of chan.sip
* enhanced notify message to make SIP call pickups work
* incorporated patch from bug 3710 => you may close 3710 and set a reference to this one
* INVITES with "Replaces" Header work now
* did a major whitespace code cleanup (i think some will love it and some will hate me for doing this)

in our company with serveral hundred calls per day to different sip hard- and softphones and a legacy pbx we could find no instability.

please - please - please:
- review this patch and make comments to get it committed to cvs.

By: dbruce (dbruce) 2005-07-16 16:23:27

Please... Review this issue and the attached patch for immediate inclusion in CVS.

The hard work of Xylome (and others) adds significant value to the functionality of the SIP driver. The patch also has little or no effect on the baseline functioning of the SIP driver and would not change it's default behaviour.

Also, inclusion in CVS would allow for more effort to be expended on improvements than on maintenance.

Thank you!



By: Olle Johansson (oej) 2005-07-18 03:12:56

xylome: I am back from holiday and a training in Denver. I think we have to separate things here, we can't add too much into one single patch. By adding so much, you are delaying the inclusion of my earlier patch. I'll look at this and see how we can make it manageable.

By: Frank Sautter (xylome) 2005-07-18 13:09:15

updated patch to revision 781 of chan_sip.c

oej gave me the hint, that it would be more likely to get to committed if the patch is reduced to a minimum.

* solved a bug which was introduced with devstate.c (state 'confirmed' was not published imediately to the subscribed phones)
* call pickup on established calls is no longer possible

By: Frank Sautter (xylome) 2005-07-20 07:23:03

just an update to revision 783 of chan_sip.c
i would be happy to cut this patch into smaller pieces if this one is to big to be committed in one piece (what i truly understand). but then there should be a short time of code freeze on chan_sip so i can take this monster patch apart and then one of you bugmarshals could submit them to cvs in short succession.

any comments?

By: Olle Johansson (oej) 2005-07-20 07:38:43

xylome: Remove the support of REPLACES, it does not belong in this patch. We need to discuss that off-line, since I have a lot of that code in progress as well... And if you do whitespace cleanup, it needs to be in a separate patch according to the guidelines :-)

Thanks for your hard work!

By: Olle Johansson (oej) 2005-07-25 08:50:44

Xylome: Do you have time to fix the patch or should we go back to the one before you added stuff?

By: Olle Johansson (oej) 2005-07-25 08:52:59

Also remove the whitespace and vmexten to separate patches, please.

By: Frank Sautter (xylome) 2005-07-25 10:08:44

oej: as you can see on my recent additions to the bugtracker, i'm currently taking this patch apart into different patches. i hope i will get the rest done tomorrow.
please help me getting all the other patches (ASTERISK-4672, ASTERISK-4673, ASTERISK-4674) i recently made to chan_sip, so we will get this all into 1.2

By: Kevin P. Fleming (kpfleming) 2005-07-25 13:47:12

For those who sent repeated reminders to me and others to review this bug... please do NOT do that. It is unnecessary, and if anything it will only serve to make me less interested in reviewing the bug. Getting a reminder is not going to make me drop some other project that is consuming my time and look at your patch; it will get looked at when time is available, regardless of whether a reminder is sent or not.

Now, on to the technical issues: Yes, I agree that there is far too much stuff combined into this patch. The other patches that are pending are a good start, let's get them taken care of and get this one cut down to its essence.

By: Frank Sautter (xylome) 2005-07-25 14:00:30

updated to revision 787 of chan_sip.c
reduced the patch to a minimum.
parts of the former patch are now accessible through bugs ASTERISK-4672, ASTERISK-4673 and ASTERISK-4674 (soon to follow more)



By: Michael Jerris (mikej) 2005-07-25 14:30:04

are ASTERISK-4672 and ASTERISK-4673 prereq's for this, or are they seperate issues?

By: Frank Sautter (xylome) 2005-07-25 15:25:10

updated to revision 789 of chan_sip.c

By: Frank Sautter (xylome) 2005-07-25 15:29:30

MikeJ: no. this patch has been taken apart into several smaller patches.
if you want this functionality now you have to revert to cvs-head as of 2005-07-20 and apply the corresponding patch.
the whole functionality will return when all pieces are committed to cvs (and there are not yet all separated into smaller patches)

By: Michael Jerris (mikej) 2005-07-25 15:39:00

my question was, do those other patches need to be in first to not cause problems if this one is commited.

By: Michael Jerris (mikej) 2005-07-25 21:33:47

ok, ASTERISK-4672 and ASTERISK-4673 are in now.. is this one up to date with those commits, and did somone test the current slimmed down patch?  I would like to get this in as it resolves a crash too..

By: Frank Sautter (xylome) 2005-07-26 05:10:38

updated to revision 792 of chan_sip.c
MikeJ: you are welcome to test this patch in your environment.

By: Olle Johansson (oej) 2005-07-26 06:19:17

Seems like we're back to my original patch with some additions by maik and xylome.

Thank you, friends, for continuing this work and updating patches, separating patches and making life easier for the committers! This will be a great additional feature to Asterisk chan_sip.



By: Michael Jerris (mikej) 2005-07-26 06:32:33

is this up to date with kpflemings most recent code review, tested, and ready to go?

By: Olle Johansson (oej) 2005-07-26 07:04:33

With all the changes, I think this requires a new kpfleming review.

Guess this wasn't done:
"Please create an enum for the p->subscribed values with proper names for each of the four possible values, and use a switch statement instead of chained ifs to process them when building the response packet."

We need to change the #define's to an enum structure.

By: Frank Sautter (xylome) 2005-07-26 12:04:53

oej, kpflemming: the enum for sip_pvt.subscribed is in place (i missunderstood your recommendation in the first place and made an enum for 'localstate' and not for 'sip_pvt.subscribed').
added initialisation to 'sip_pvt.subscribed'.

By: Frank Sautter (xylome) 2005-07-27 04:18:55

kevin, olle: new patch for revision 796 created
do you think we can get this into cvs today, because some other patches rely on this one and i'm not able to provide them before this one is in place.



By: Mark Spencer (markster) 2005-07-28 14:21:48

Is it possible to subscribe the same call number to multiple events (e.g. a combination of DTMF, presense and message waiting)?  If so, we need the patch to be modified to support that.  I'm a little concerned about the RINGING+INUSE and RINGING states -- maybe there is a way to make them bitwise-or'd so that if you only need to know if there is a device tha is ringing or you need to know that there is one that is in use, you won't have to make multiple comparisons?

By: Olle Johansson (oej) 2005-07-29 01:30:48

maik: Any response on Mark's comments?

By: Frank Sautter (xylome) 2005-07-29 11:04:38

* updated to revision 797
* implemented mark's suggestion (bitwise oring)

By: xrobau (xrobau) 2005-07-30 22:58:38

Minor typo in the patch -
-               ast_cli(fd, "%d active SIP subscriptions(s)\n", numchans);
+               ast_cli(fd, "%d active SIP subscriptions%s\n", numchans, (numchans != 1) ? "s" : "");

Note - it should be 'subscription%s', otherwise with more than one you get 'subscriptionss'

By: xrobau (xrobau) 2005-08-01 06:31:55

Xylome, may I bring to you attention patch 0004865 which implements directed pickups. I had a look at your 7-15 revision which implemented call pickup when ringing, and after having a bit of a fiddle, it seems like it's the <local> and <remote> tags that tell the phone who to call. If app_directed_pickup is accepted, you could possibly use that instead of the original functionality?

By: Frank Sautter (xylome) 2005-08-01 10:35:36

corrected a spelling error (thanks to xrobau)

By: Frank Sautter (xylome) 2005-08-03 03:43:21

updated to revision 801.
is there anything that prevents inclusion into cvs head?

By: xrobau (xrobau) 2005-08-03 04:02:26

FWIW: I've been testing it here in a lab with 5 phones (3 x snom360, gxp2000 and a budgettone 101) and it's been working fine. The only difference in this patch is the 'subscriptions%s' change.

By: xrobau (xrobau) 2005-08-03 04:30:51

Of course, when I say 'working perfectly', I didn't actually bother _answering_ any of the calls. The light continues to flash when the call is answered.
306 dials 307 - NOTIFY with dialog-id=306, state=confirmed
307 starts to ring - NOTIFY with dialog-id=307;direction="recipient"
307 answers -- Nothing gets sent.

If you require any more information, you can find me on IRC as 'X-Rob'
(Note: This has just _stopped happening_), and is now working as expected, when running asterisk -vvvvr. I'll see if I can do some more debugging.



By: xrobau (xrobau) 2005-08-03 17:25:21

OK, xylome and I spent a significant amount of time on IRC diagnosing this, and whilst he found a work-around, I've got a feeling that this is just an indication of a deeper problem.  
What was happening was that if I answered the call 'quickly', eg, within one or two seconds of it starting to ring, asterisk had not changed the device state to AST_STATE_UP, which meant that the ast_device_state_changed() call wasn't doing anything. If I waited 4-5 seconds, and then answered the call, it worked fine. Xylome suggested a fix of doing an ast_setstate(p->owner, AST_STATE_UP); before the ast_device_state_changed (at around line 8627 in the patch), which immediately fixed the problem.
However, he was unable to duplicate this problem. This is probably because he is developing on a dual athlon 2.8, whilst my small test box is a Piii/500, where any race states or slowdowns are exacerbated. If someone could clue me up on where to look for more diagnosis of this, I'd love to help, but I'm pretty much out of my depth here 8-(

By: Olle Johansson (oej) 2005-08-04 04:42:04

Patience, please. The committers have been busy with other things for a few days.

By: Juraj Bednar (juraj) 2005-08-12 10:04:09

this patch is unnapliable to current cvs head, there were lots of changes, some things from this patch (from my point of view not much related) are already applied.

transmit_state_notify() has been changed quote a lot, there are lots of rejects. It is possible, that this has to be updated before entry into CVS.

Also, did you actually solve the problem with extension staying "on the phone"? Does the workaround you mentioned work?

By: Frank Sautter (xylome) 2005-08-12 10:26:31

patch updated to revision 806 of chan_sip.c

juraj: i don't know which patch you tried to apply, but there haven't been any addition from this patch to cvs except the ones i moved to other patches (see problem relations of this bug). i just checked cvs mailing list. as far as i can see, there was no change regarding transmit_state_notify() since 801?!?

as xrobrau wrote there seems to be an issue on slow machines, but i currently have not time to dig deeper. the bugfix i suggested for xrobrau was a dirty hack with potential segfault!

By: Juraj Bednar (juraj) 2005-08-14 12:37:19

Thanks, this patch was applied correctly.

Looking forward for the bugfix.

BTW: the bug happened to me on Intel(R) Xeon(TM) CPU 3.20GHz, 64-bit native Debian

By: Michael Jerris (mikej) 2005-08-14 19:04:03

as this issue is not just a slow machine issue, we definately need to figure out this race situation before this goes in.

By: Frank Sautter (xylome) 2005-08-16 05:43:21

i think i solved the problem occuring in some setups (yet i'm unable to reproduce it).
i added a 'ast_setstate(p->owner, AST_STATE_UP)' during the processing of the '200 OK' answer to our INVITE if the state isn't already in 'AST_STATE_UP' (very similar to the quick'n'dirty hack i sent to xrobau, but without a potential segfault).
please give feedback, if this fix solved your problem...

By: Juraj Bednar (juraj) 2005-08-16 06:49:47

thanks, I will try it.

BTW: if someone want just to try this new fix, here's the patch against chan_sip.c produced by sipsubscribe-20050812.rev806.txt.

--- chan_sip.c.orig      2005-08-16 13:45:35.802274960 +0200
+++ chan_sip.c  2005-08-16 13:45:58.958754640 +0200
@@ -8730,6 +8730,7 @@
                                               time(&p->ospstart);
#endif
                                               ast_queue_control(p->owner, AST_CONTROL_ANSWER);
+                                               ast_setstate(p->owner, AST_STATE_UP);
                                       } else {
                                               struct ast_frame af = { AST_FRAME_NULL, };
                                               ast_queue_frame(p->owner, &af);

By: xrobau (xrobau) 2005-08-16 09:45:13

I don't have any Snom's at the moment. I've got one on order, hopefully it'll be here within 48 hours of this reply - I'll do some testing when it arrives and update this patch then.

By: dbruce (dbruce) 2005-08-19 03:56:43

Somewhere along the line, the subscribe patch lost support for the Polycom soundpoint phones. The issue is that the Polycoms subscribe for "event: dialog" but does not include an "accept:" header.

Attached is a small patch that allows the subscription mechanism to work, by checking for a "dialog" event combined with "polycom" in the useragent header.

Disclaimer is on file.

By: Olle Johansson (oej) 2005-08-22 01:19:57

If you keep changing this patch, it will never ever go into cvs... Can we stop adding and changing and try to stabilize something. There are thousands of additions that could possibly be made to this, but let's do it later in the new cvs head. I opened this bug report in february and it's still not in the cvs.

Thank you.

By: Frank Sautter (xylome) 2005-08-23 11:43:51

updated to revision 813 of chan_sip.c

By: Olle Johansson (oej) 2005-08-24 03:56:15

Support for Eye-beam is broken by recent fixing. I will try to get this into shape again today.

By: Olle Johansson (oej) 2005-08-24 12:04:35

Uploaded new patch
- adds subscription type to "sip show subscriptions"
- adds setting of notifyringing to "sip show settings"
- fixes a case of empty contact header in failure responses
- works with eye-beam :-)

By: Frank Sautter (xylome) 2005-08-24 15:29:09

oej: thanks for your dedication
* i did some cleanup according to the coding guidelines
* added subscribecontext to sip.conf.sample
* removed 2 compiler warnings (did some ugly casting - maybe someone other looks after these warnings)

are you sure with this change (i have in mind that this was changed lately by mark):
- /* Go ahead and free RTP port */
- if (p->rtp && !p->owner) {
- ast_rtp_destroy(p->rtp);
- p->rtp = NULL;
- }
- if (p->vrtp && !p->owner) {
- ast_rtp_destroy(p->vrtp);
- p->vrtp = NULL;
- }



By: Frank Sautter (xylome) 2005-08-24 15:49:48

added output of subscribecontext to 'sip show peer'

By: Michael Jerris (mikej) 2005-08-24 15:52:14

we need to stop adding tweaks to this bug.  Can we please have this verified working and tested by xylome and oej, as well as others (please comment the bug) and get this in already.

By: Juraj Bednar (juraj) 2005-08-24 15:58:02

I use it since 20050801, no glitches, no problems for me, except of one, that is already fixed (with someone staying "On the phone")

To the future -- not meant to be fixed now. I believe it's not a good idea to have hardcoded strings of status here. Should be configurable and translateable, not everyone is speaking English(only). Should use translation configurable per user. Later in CVS HEAD.

Tested only with eyeBeam, used in production for three weeks...

By: Michael Jerris (mikej) 2005-08-24 22:59:46

xylome, in reference to that one block of code, that is a duplacte of a part of a patch moved to issue ASTERISK-4891 that has now been commited.

By: Olle Johansson (oej) 2005-08-25 00:42:21

Mikej: I remove the Subscribe block here, the register block in the other patch. This patch is about SUBSCRIBE...

Xylome: Thanks for removing those compiler warnings. Yes, I am sure that we do not allocate RTP for SUBSCRIBE any more, so we can remove those. We have been reworking the RTP allocation since Mark put those in. The SIP channel actually improves :-)

By: Frank Sautter (xylome) 2005-08-25 10:05:53

updated to revision 820 of chan_sip.c
* after oej yesterday added output of subscription state in 'sip show subscriptions', but used the 'ast_state2str' function which returns the devicesstate instead of the extensionstate, i added a new function to pbx.c that ast_extension_state2str.
* simplification of sub_type2str and find_sub_type
* some output beautification (alignment/truncation) of 'sip show channels'

By: Michael Jerris (mikej) 2005-08-25 10:13:06

I've said it before and I will say it again.  This bug needs to stop having functionality changes, or it CAN NOT go in to the tree.  We need people to test and verify functionality testing on this latest patch again now.  This bug is looking more and more to not be able to go into 1.2 becuase of the constant changing.

By: Olle Johansson (oej) 2005-08-25 10:16:22

Yes, this is final freeze. Please test now!

By: Frank Sautter (xylome) 2005-08-25 10:18:59

* was not happy with abbreviated function names regarding subscription types
* not happy with those functions returning NULL in case of wrong input (out of range of enum)



By: Frank Sautter (xylome) 2005-08-25 10:23:42

MikeJ, oej: sorry, but we have a moving target.... i just did some code review on oej's latest changes to make them pass the testing of our users and the bugmarshals.
speaking for myself: i will only keep this up to date and will not add any functionality to this any more.



By: Frank Sautter (xylome) 2005-08-25 10:49:53

MikeJ: regarding the tests: this patch is heavily tested several months now by many users (including us (ca.25 users, ca.150 calls a day) and some of our customers (biggest one ca.450 users, ca.2300 calls a day) ca.15 different hard and softphones) => no major problems (segfaults, deadlocks) and all reported issues have been removed!
this bug would not have this long record, if it had been accepted earlier.



By: dwildes (dwildes) 2005-08-25 16:27:32

I've tested this patch on a small office for the day - around 15 SNOM 360 phones under light load of around 75 calls.  Everything ran great for them, no segfaults - hangups, business as usual.

By: xrobau (xrobau) 2005-08-25 18:45:33

I thought I should just point out that there are no problems with this patch - it works, doesn't cause crashes, and does as advertised. It should go in.

By: mochouinard (mochouinard) 2005-08-26 08:08:42

Everything seem to work as usual with this patch.  No issues

By: BJ Weschke (bweschke) 2005-08-26 11:56:06

This tested succesfully with a SNOM 360 I have here (firmware v4.0)

However, maybe this is in the documentation and missed it, but if not, the hint priority in extensions.conf must be above the action taken on that same extension. If not, the hook to send the state change to the subscriber is never taken.

Once I re-arranged the order, this worked fine.

By: Olle Johansson (oej) 2005-08-28 17:40:30

Updated to support yesterday's fix of Asterisk internal extension state subscription system.

By: paradise (paradise) 2005-08-29 01:10:56

nice patch, works fine for me.
why it's not added to CVS HEAD after 6 months???

By: Frank Sautter (xylome) 2005-08-29 05:28:44

oej's last patch missed the patches to sip.conf.sample

By: Olle Johansson (oej) 2005-08-29 09:55:00

Because it has been constantly changed, but we're close now, so have patience. it will go into CVS shortly.

By: wjchan (wjchan) 2005-08-29 12:29:01

polycom-sub.txt

I installed this patch but didn't change sip.conf.  When I enabled "presence" on a couple of my Polycom 600 phones and added a couple of "buddies", a lot of subscription messages go back and forth.  As you can see from the polycom-sub.txt file, the list of subscriptions keep growing.

By: Olle Johansson (oej) 2005-08-29 13:01:06

That polycom issue is weird... Need more debug than this, to see what happens, why the polycom re-issues a subscription.

By: Olle Johansson (oej) 2005-08-29 13:02:05

I have a separate patch for sip.conf.sample since there was a lot of errors in it... Sorry, did not explain that.

By: wjchan (wjchan) 2005-08-29 13:28:48

polycom-sub-debug.txt

Log starts at phone reboot

By: wjchan (wjchan) 2005-08-29 14:27:44

adding exten => XXX,hint,SIP/yyy fixed the problem

By: Olle Johansson (oej) 2005-08-29 14:33:41

Yes, I realized you had no hints. Have a patch coming up, since it was a bug. Could you please remove the hint and try with the new patch (will take a minute)?
I just want to confirm that I've fixed this issue.

By: wjchan (wjchan) 2005-08-29 14:58:57

That fixed the hint problem.

By: Olle Johansson (oej) 2005-08-29 15:12:43

Can you please try this patch as well? We now refuse the subscription if there's no hint and log an error message to the console.

By: wjchan (wjchan) 2005-08-29 15:39:17

couple of typos in the patch:

firstate -> firststate (2 occurences)

"Please add hint to...." missing closing quote.

Other than those, the patch is fine.

By: Kevin P. Fleming (kpfleming) 2005-08-29 19:52:26

Committed to CVS HEAD, thanks!

By: Digium Subversion (svnbot) 2008-01-15 15:34:50.000-0600

Repository: asterisk
Revision: 5684

U   trunk/include/asterisk/utils.h
U   trunk/utils.c

------------------------------------------------------------------------
r5684 | kpfleming | 2008-01-15 15:34:50 -0600 (Tue, 15 Jan 2008) | 2 lines

add ast_build_string library function (from bug ASTERISK-3564)

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

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

By: Digium Subversion (svnbot) 2008-01-15 15:46:06.000-0600

Repository: asterisk
Revision: 6446

U   trunk/channels/chan_sip.c
U   trunk/include/asterisk/pbx.h
U   trunk/pbx.c

------------------------------------------------------------------------
r6446 | kpfleming | 2008-01-15 15:46:06 -0600 (Tue, 15 Jan 2008) | 2 lines

massive upgrade to SUBSCRIBE, device state and messaging support (issue ASTERISK-3564)

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

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