Bug 235236 - net/asterisk15 improvement: force setvar to override previous value in chan_sip, chan_iax2
Summary: net/asterisk15 improvement: force setvar to override previous value in chan_s...
Status: Closed Overcome By Events
Alias: None
Product: Ports & Packages
Classification: Unclassified
Component: Individual Port(s) (show other bugs)
Version: Latest
Hardware: Any Any
: --- Affects Only Me
Assignee: Guido Falsi
URL: https://gerrit.asterisk.org/10930
Keywords:
Depends on:
Blocks:
 
Reported: 2019-01-27 11:07 UTC by pfduch
Modified: 2019-10-04 19:02 UTC (History)
1 user (show)

See Also:
madpilot: maintainer-feedback+


Attachments
patch for chan_sip and chan_iax2 (3.51 KB, patch)
2019-01-27 11:07 UTC, pfduch
no flags Details | Diff
patch to add an option to Makefile (268 bytes, patch)
2019-01-27 11:09 UTC, pfduch
no flags Details | Diff
unified diff for Makefile (268 bytes, patch)
2019-01-27 11:14 UTC, pfduch
no flags Details | Diff
unified diff for Makefile (really) (1.03 KB, patch)
2019-01-27 11:16 UTC, pfduch
no flags Details | Diff
patch for chan_sip and chan_iax2 (v2) (3.51 KB, patch)
2019-01-28 18:26 UTC, pfduch
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description pfduch 2019-01-27 11:07:36 UTC
Created attachment 201436 [details]
patch for chan_sip and chan_iax2

In chan_sip and chan_iax2, variable set by setvar option cannot be overriden later. E.g.:

[tmpl](!)
setvar=foo=bar

[some-peer](tmpl)
setvar=foo=baz ; no chance: "${SIPPEER(some-peer,chanvar[foo])}" in dialplan still returns "bar"


Actually, second setvar appends an attribute-value pair to a list, but only the first one is always read.

A ticket in asterisk JIRA is still open for a long long time:
https://issues.asterisk.org/jira/browse/ASTERISK-23756

Unlike chan_sip and chan_iax2, chan_dahdi, for example, was already fixed for that.

I use the attached patch for a several years without any problems on versions 11.x, 13.x, 15.x, both i386 and amd64. It would be nice to include it to mainstream. Maybe it makes sense even to enable it by default.
Comment 1 pfduch 2019-01-27 11:09:15 UTC
Created attachment 201437 [details]
patch to add an option to Makefile

And a Makefile patch which adds an option to use a patch in subject
Comment 2 pfduch 2019-01-27 11:14:43 UTC
Created attachment 201439 [details]
unified diff for Makefile

Sorry for diff format
Comment 3 pfduch 2019-01-27 11:16:50 UTC
Created attachment 201440 [details]
unified diff for Makefile (really)
Comment 4 Guido Falsi freebsd_committer 2019-01-27 11:39:43 UTC
(In reply to pfduch from comment #0)
[...]
> 
> I use the attached patch for a several years without any problems on
> versions 11.x, 13.x, 15.x, both i386 and amd64. It would be nice to include
> it to mainstream. Maybe it makes sense even to enable it by default.

Hi,

I understand your need, and understand the bug you're fighting with, but at the same time, this is an upstream bug, which is not specific to the FreeBSD port.

FreeBSD ports are, as the name implies, ports, and not forks of the upstream project.

I'm very cautious about importing patches which cause ports to diverge from upstream in any way. This is especially so for patches I'm unable to properly review myself, like this one. This code should be properly  reviewed and evaluated by the asterisk developers.

Asterisk ports already have too many options changing it's default behavior. Enabling by default this option causing a default behaviour different from the upstream project is something I'm very weary to do.

I'm not going to close this bug report right away, but my suggestion is to push harder for your patch to be included upstream.

Asterisk has a strict procedure ti get patches in their project, and that requires you to use their gerrit system to get a code review for the patch. Patches attached to bug report are much more difficult to get accepted.

Are you able to propose the patch in the asterisk gerrit system so it can get a better chance of being accepted there? 

If you're not confortable with their procedures, since I already got a few patches included upstream through their submission procedure, if you are ok with that, I could submit it to gerrit, obviously linking your bug report and specifying I'm only acting as a middleman.
Comment 5 Guido Falsi freebsd_committer 2019-01-27 12:17:45 UTC
In the while, looking at your code I have on question:

at line 30861 of chan_sip.c, you do:

tmpvar = NULL;

It looks redundant to me, since tmpvar is reassigned at the line below as a side effect of the if condition.


And, since I don't know the details of how asterisk manages channel variables internally, if I get correctly the meaning, whenever a new variable is set, your code goes through the variables linked list and removes any previous references to variables with the same name, is this correct?


As a final note, to get the code accepted upstream there a re a few style issues. The asterisk code uses spaces after commas and in control structures after the "if" and "for" and between the ")" and the "{". Also, the comments indentation looks wrong. Don't undervalue the impact such details can have on getting a patch integrated.

As a final note, this bug has been there for a long time, it is quite possible there are people out there with asterisk configuration that would brake if changing this behaviour. This can be a major roadblock to getting this patch included upstream.

Please let me know if you want me to help you with the asterisk code review process or want to make me submit it to gerrit on your behalf.
Comment 6 pfduch 2019-01-28 17:19:24 UTC
(In reply to Guido Falsi from comment #4)
> FreeBSD ports are, as the name implies, ports, and not forks of the upstream project.

Sometimes they may contain some fixes/features, not always FreeBSD-specific (e.g. mail/mutt).

> Asterisk ports already have too many options changing it's default behavior. Enabling by default this option causing a default behaviour different from the upstream project is something I'm very weary to do.

That is why I don't insist on enabling it by default. But, yes, a separate option seems to be an overkill too...

> Asterisk has a strict procedure ti get patches in their project, and that requires you to use their gerrit system to get a code review for the patch. Patches attached to bug report are much more difficult to get accepted.
> Are you able to propose the patch in the asterisk gerrit system so it can get a better chance of being accepted there?

I doubt it. I'm not familiar with neither gerrit nor asterisk policies for patches.

> If you're not confortable with their procedures, since I already got a few patches included upstream through their submission procedure, if you are ok with that, I could submit it to gerrit, obviously linking your bug report and specifying I'm only acting as a middleman.

I'd be very appreciative.
Comment 7 pfduch 2019-01-28 17:40:14 UTC
(In reply to Guido Falsi from comment #5)

Actually, that's not my patch. A long ago I just found it, inspected,
tried to test and then put into production.

> It looks redundant to me, since tmpvar is reassigned at the line below
> as a side effect of the if condition.
I suppose author wanted just to ensure tmpvar is unset as before added
block.

> ...whenever a new variable is set, your code goes through the
> variables linked list and removes any previous references to variables
> with the same name, is this correct?
Exactly.

> As a final note, to get the code accepted upstream there a re a few
> style issues.
No problem, I can reformat it.

> As a final note, this bug has been there for a long time, it is quite
> possible there are people out there with asterisk configuration that
> would brake if changing this behaviour.
I'm quite sure that noone uses several setvar's for the same variable
(except accidential) as it makes no sense with current behavour.

> Please let me know if you want me to help you with the asterisk code
> review process or want to make me submit it to gerrit on your behalf.
Please submit it.
Comment 8 pfduch 2019-01-28 17:45:03 UTC
(In reply to Guido Falsi from comment #5)

> As a final note, this bug has been there for a long time, it is quite
> possible there are people out there with asterisk configuration that
> would brake if changing this behaviour. This can be a major roadblock
> to getting this patch included upstream.
By the way, similar patch has been accepted for chan_dahdi:
https://issues.asterisk.org/jira/browse/ASTERISK-16197
Comment 9 pfduch 2019-01-28 18:26:57 UTC
Created attachment 201484 [details]
patch for chan_sip and chan_iax2 (v2)

Coding style formatting.

Also actualized line numbers for 15.7.1. Patch was being cleanly applied for different versions of asterisk, so I just didn't mind.
Comment 10 Guido Falsi freebsd_committer 2019-01-28 19:03:48 UTC
Thanks for your work, I'll have anyway to reapply the patch to the asterisk git repository head.

Please allow me time to work things out, I need to followup to the old bug report, prepare the patch and commit it to their gerrit code review system.
Comment 11 Guido Falsi freebsd_committer 2019-01-28 20:24:05 UTC
I submitted the patch almost as is but it's not conforming to the new asterisk code (it compiles and works in the FreeBSD port but is not up to standards anymore).

This is one of the reasons why I don't like accepting in ports patches which have not been integrated upstream. This patch as is has no chance of being accepted in the official asterisk sources.

I'll try to clean it up.
Comment 12 Guido Falsi freebsd_committer 2019-02-18 08:30:15 UTC
I'm working with upstream to get this change included there:

https://gerrit.asterisk.org/10930


They required some changes I'm (slowly) making.
Comment 13 Guido Falsi freebsd_committer 2019-04-03 09:38:21 UTC
Quick update:

upstream code review in progress, It's on track to be included in official releases, but still needs to be reviewed by more upstream developers.
Comment 14 Guido Falsi freebsd_committer 2019-09-13 14:01:42 UTC
The upstream bug has been closed and the change imported in upstream sources.

It will be available in the next asterisk 13 and asterisk 16 release.

Unluckily it will not be available in asterisk 15, because that version is nearing EOL and is in security patches only policy.

Anyway, since asterisk15 is EOL and the port will be also removed once that happens I suggest anyone using asterisk15 to migrate to asterisk 16, which is a Long Term Support release and will be supported upstream till 2023.
Comment 15 Rene Ladan freebsd_committer 2019-10-04 19:02:06 UTC
Expired port removed, switch to net/asterisk16 instead.