[Home]

Summary:ASTERISK-13203: [patch] chanvar-option for peers in sip.conf
Reporter:Marcus Hunger (fnordian)Labels:
Date Opened:2008-12-11 05:27:36.000-0600Date Closed:2009-07-07 16:10:16
Priority:MajorRegression?No
Status:Closed/CompleteComponents:Channels/chan_sip/General
Versions:Frequency of
Occurrence
Related
Issues:
Environment:Attachments:( 0) 20090116__bug14059.diff.txt
( 1) chan_sip_hfield.patch
Description:hi,
the attached patch introduces a new configuration-option for sip-peers called "hfield". if set, the value of this option is appended to an outgoing sip-invite.
by repeating the option it is possible to add multiple header-fields.

this enables per peer sip-header-fields e.g. for p-preferred-identity.



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

the patch does two things. it adds "hfield" to the build_peer loop to set __SIPADDHEADER%.2d channel-variables and it copies the peer->chanvars to the newly created dialog in create_addr_from_peer.
Comments:By: Marcus Hunger (fnordian) 2008-12-11 05:39:04.000-0600

the second part of the patch seems to copy functionality from http://bugs.digium.com/view.php?id=13565 (13565.diff).
By: Tilghman Lesher (tilghman) 2009-01-16 15:57:52.000-0600

Looks good, although I would just call the element "header" instead of "hfield".
By: Tilghman Lesher (tilghman) 2009-01-16 16:25:32.000-0600

Patch updated to current trunk, as well as adding the ability to embed semicolons into headers.
By: Olle Johansson (oej) 2009-01-23 08:53:13.000-0600

You can already do this with setvar=SIPADDHEADER90=header
By: Marcus Hunger (fnordian) 2009-01-23 09:04:48.000-0600

It should not work. Currently setvar works not for outgoing channels.
By: Olle Johansson (oej) 2009-01-23 09:07:11.000-0600

I see, you are right - my mistake.

Thanks for the feedback. Let's move this work forward then.
By: Tilghman Lesher (tilghman) 2009-01-26 17:43:05.000-0600

Actually, oej is right, though he had the syntax wrong:

setvar=__SIPADDHEADER90=X-Someheader: somethingelse

You don't even need to keep to 2 digits or use digits:

setvar=__SIPADDHEADERsuper=X-Someheader: somethingelse
setvar=__SIPADDHEADERduper=X-Foo: bar

setvar does exactly the same thing as this patch.
By: Marcus Hunger (fnordian) 2009-01-27 03:23:13.000-0600

the patch has two parts: one that does the same thing as setvar setting __SIPADDHEADERsomething and another one, that copies the channel-variables into an outgoing channel.
By: Olle Johansson (oej) 2009-01-27 03:41:30.000-0600

Takes time for an old man... :-)

You are right, the first parts of your patch makes sense. We only use the peer chanvars on inbound calls, not on outbound calls. I can accept that part of the patch, but don't really see the immediate need to add another configuration option. Better to teach people about setvar and use that.
By: Olle Johansson (oej) 2009-01-27 03:51:42.000-0600

Reassigning this issue to myself. I'm sure that Corydon won't mind one less issue in the tracker. If you do, please contact me or just re-assign it again :-)
By: Olle Johansson (oej) 2009-01-27 03:52:22.000-0600

Fnordian: Will think about the header= part a bit more before I make a final decision. If you want to convince me, find me on IRC...
By: Digium Subversion (svnbot) 2009-01-29 07:20:45.000-0600

Repository: asterisk
Revision: 172268

U   trunk/channels/chan_sip.c
U   trunk/include/asterisk/app.h
U   trunk/main/app.c

------------------------------------------------------------------------
r172268 | oej | 2009-01-29 07:20:44 -0600 (Thu, 29 Jan 2009) | 15 lines

- Make sure we set setvar= variables on outbound calls too, not only inbound calls.
- Also, change a function in app.c to return a userful value instead of always returning 0.

Patch by fnordian, changed by Corydon76 and myself.

This does not close the bug report, as fnordian had an additional change we're still discussing.

(related to issue ASTERISK-13203)
Reported by: fnordian
Patches:
     chan_sip_hfield.patch uploaded by fnordian (license 110)
     20090116__bug14059.diff.txt uploaded by Corydon76 (license 14)
Tested by: fnordian, Corydon76, oej


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

http://svn.digium.com/view/asterisk?view=rev&revision=172268
By: Leif Madsen (lmadsen) 2009-07-01 08:22:43

fnordian: the commit by oej says there is an additional change that you're still discussing -- has any work gone towards that?

Thanks!
By: Marcus Hunger (fnordian) 2009-07-01 08:46:08

The additional change is the "header"-config-option to have peer-specific sip-header-fields for outgoing calls. I am still very interested in that, but waited for the "final decision" from your side.

The proposed alternative for
header=X-Foo: bar
is
setvar=__SIPADDHEADERduper=X-Foo: bar

which in my opinion is not really an option but a config-hack because it brings an internal implementationstrategy of chan_sip to the config file.
By: Olle Johansson (oej) 2009-07-01 09:00:02

I think we should close this and open that as a feature request in another issue or discuss it on the asterisk-dev mailing list.

Thanks,
/O
By: Marcus Hunger (fnordian) 2009-07-01 09:01:15

oh. it's already a feature-request.
By: Leif Madsen (lmadsen) 2009-07-01 15:13:16

I agree -- it's already marked as a feature request. Why are we closing it again?

Perhaps it should be brought up on the mailing list though for further review. I'll let oej determine what he wants to do with this issue though -- if he feels it should be closed, he is welcome to do so.
By: Tilghman Lesher (tilghman) 2009-07-02 09:30:27

fnordian:  I think you're right that it's too hackish.

lmadsen: let's schedule that change for the next 1.4 release.

oej: if you want a different config option name, let's discuss that, but I think the overall concept of eliminating the hack has merit.


By: Digium Subversion (svnbot) 2009-07-07 16:10:15

Repository: asterisk
Revision: 205086

U   trunk/channels/chan_sip.c

------------------------------------------------------------------------
r205086 | tilghman | 2009-07-07 16:10:14 -0500 (Tue, 07 Jul 2009) | 4 lines

Permit setting custom headers from the peer definition.
(closes issue ASTERISK-13203)
Reported by: fnordian

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

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