[Asterisk-code-review] ASTERISK-24601 Include LOCAL/REMOTE tags in BLF NOTIFY XML Patch Orig... (asterisk[16])

[Joshua Colp]

Joshua Colp has posted comments on this change. ( https://gerrit.asterisk.org/c/asterisk/+/15763 )

Change subject: ASTERISK-24601 Include LOCAL/REMOTE tags in BLF NOTIFY XML Patch Originally submitted by Joshua Elson Modified by Joseph Nadiv and Sean Bright
......................................................................


Patch Set 1: Code-Review-1

(4 comments)

This is substantial enough that it also warrants a CHANGES entry[1].

[1] https://wiki.asterisk.org/wiki/display/AST/CHANGES+and+UPGRADE.txt

https://gerrit.asterisk.org/c/asterisk/+/15763/1//COMMIT_MSG 
Commit Message:

https://gerrit.asterisk.org/c/asterisk/+/15763/1//COMMIT_MSG@7 
PS1, Line 7: ASTERISK-24601 Include LOCAL/REMOTE tags in BLF NOTIFY XML
Please follow the Commit Messages wiki page[1] for constructing this commit message.

[1] https://wiki.asterisk.org/wiki/display/AST/Commit+Messages


https://gerrit.asterisk.org/c/asterisk/+/15763/1/res/res_pjsip_dialog_info_body_generator.c 
File res/res_pjsip_dialog_info_body_generator.c:

https://gerrit.asterisk.org/c/asterisk/+/15763/1/res/res_pjsip_dialog_info_body_generator.c@156 
PS1, Line 156: 		from_domain = endpoint ? (!ast_strlen_zero(endpoint->fromdomain) ? endpoint->fromdomain : invalid) : NULL;
This should have a comment stating that from_domain remains valid as the subscription holds a reference to the endpoint, so as long as the subscription is valid then from_domain will be valid.


https://gerrit.asterisk.org/c/asterisk/+/15763/1/res/res_pjsip_dialog_info_body_generator.c@192 
PS1, Line 192: 			ast_debug(3, "From Domain remote URI domain '%s'\n", sanitized_remote);
If this debug message is to remain it should be extended with additional information about what this refers to, such as the endpoint involved and what is subscribed to.


https://gerrit.asterisk.org/c/asterisk/+/15763/1/res/res_pjsip_dialog_info_body_generator.c@207 
PS1, Line 207: 			need = strlen(connected_num) + (connected_num_restricted ? strlen(invalid) :
remote_target has a fixed maximum size, I don't see why this needs to be calculated and given to snprintf



-- 
To view, visit https://gerrit.asterisk.org/c/asterisk/+/15763
To unsubscribe, or for help writing mail filters, visit https://gerrit.asterisk.org/settings

Gerrit-Project: asterisk
Gerrit-Branch: 16
Gerrit-Change-Id: I20c5cf5b45f34d7179df6573c5abf863eb72964b
Gerrit-Change-Number: 15763
Gerrit-PatchSet: 1
Gerrit-Owner: Joseph Nadiv <ynadiv at corpit.xyz>
Gerrit-Reviewer: Joshua Colp <jcolp at sangoma.com>
Gerrit-CC: Friendly Automation
Gerrit-Comment-Date: Wed, 14 Apr 2021 15:02:56 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20210414/25339b31/attachment.html>