Bug 235236
| Summary: | net/asterisk15 improvement: force setvar to override previous value in chan_sip, chan_iax2 | ||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | Ports & Packages | Reporter: | Alexander Pravkin <pfduch> | ||||||||||||
| Component: | Individual Port(s) | Assignee: | Guido Falsi <madpilot> | ||||||||||||
| Status: | Closed Overcome By Events | ||||||||||||||
| Severity: | Affects Only Me | CC: | rene | ||||||||||||
| Priority: | --- | Flags: | madpilot:
maintainer-feedback+
|
||||||||||||
| Version: | Latest | ||||||||||||||
| Hardware: | Any | ||||||||||||||
| OS: | Any | ||||||||||||||
| URL: | https://gerrit.asterisk.org/10930 | ||||||||||||||
| Attachments: |
|
||||||||||||||
|
Description
Alexander Pravkin
2019-01-27 11:07:36 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
Created attachment 201439 [details]
unified diff for Makefile
Sorry for diff format
Created attachment 201440 [details]
unified diff for Makefile (really)
(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. 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.
(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. (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. (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 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.
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. 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. I'm working with upstream to get this change included there: https://gerrit.asterisk.org/10930 They required some changes I'm (slowly) making. 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. 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. Expired port removed, switch to net/asterisk16 instead. |
