[Asterisk-code-review] chan sip: 3PCC patch for AMI "SIPnotify" (asterisk[master])

[Corey Farrell]

Corey Farrell has posted comments on this change. ( https://gerrit.asterisk.org/7461 )

Change subject: chan_sip: 3PCC patch for AMI "SIPnotify"
......................................................................


Patch Set 3: Code-Review-1

(1 comment)

> Thanks for review.
 > For line 15608, used ast_variable_find_in_list() instead of
 > astman_get_header() because "Call-ID" should be in "Variable"
 > header.

Ah I see what you are doing now, but still stand behind my finding.  In this function the AMI "Variables" are being used to add data headers/content to the notify packet.  "Call-ID" is an argument and should be provided in a normal AMI header (read with astman_get_header).  This would also remove the need to skip "Call-ID" when adding variables to the SIP NOTIFY headers.  If you think I'm wrong please explain why it should be in "Variable".

I forgot to mention in the first review, you need to document the new optional parameter in the xml "DOCUMENTATION" comment (around line 583).

Finally the only comment I made which was not addressed is testing.  If you have not created a testsuite test can you please explain how you've tested it?  If you have not yet you should test with REF_DEBUG to ensure you are not leaking SIP dialogs (that would be a major leak).

https://gerrit.asterisk.org/#/c/7461/3//COMMIT_MSG
Commit Message:

https://gerrit.asterisk.org/#/c/7461/3//COMMIT_MSG@10
PS3, Line 10: with "SIPnotify" AMI action for latest (14.x and 15.x) asterisk.
Please remove " for latest (14.x and 15.x) asterisk".

Unless tests are added this feature will not be available for 15.  Asterisk 14 is in security fix only mode so no features/improvements are being accepted to 14.



-- 
To view, visit https://gerrit.asterisk.org/7461
To unsubscribe, visit https://gerrit.asterisk.org/settings

Gerrit-Project: asterisk
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I5797ded4752acd966db6b13971284db684cc5ab4
Gerrit-Change-Number: 7461
Gerrit-PatchSet: 3
Gerrit-Owner: Yasuhiko Kamata <yasuhiko.kamata at nxtg.co.jp>
Gerrit-Reviewer: Corey Farrell <git at cfware.com>
Gerrit-Reviewer: Jenkins2
Gerrit-Reviewer: Yasuhiko Kamata <yasuhiko.kamata at nxtg.co.jp>
Gerrit-Comment-Date: Fri, 08 Dec 2017 20:51:20 +0000
Gerrit-HasComments: Yes
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20171208/6192c9ed/attachment.html>