<blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><p style="white-space: pre-wrap; word-wrap: break-word;">Patch Set 1: Code-Review-1</p><p style="white-space: pre-wrap; word-wrap: break-word;">(4 comments)</p><p style="white-space: pre-wrap; word-wrap: break-word;">This is substantial enough that it also warrants a CHANGES entry[1].</p><p style="white-space: pre-wrap; word-wrap: break-word;">[1] https://wiki.asterisk.org/wiki/display/AST/CHANGES+and+UPGRADE.txt</p></blockquote><p><a href="https://gerrit.asterisk.org/c/asterisk/+/15763">View Change</a></p><p>3 comments:</p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0;"><p><a href="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:</a></p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/15763/1/res/res_pjsip_dialog_info_body_generator.c@156">Patch Set #1, Line 156:</a> <code style="font-family:monospace,monospace">         from_domain = endpoint ? (!ast_strlen_zero(endpoint->fromdomain) ? endpoint->fromdomain : invalid) : NULL;</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">This should have a comment stating that from_domain remains valid as the subscription holds a refere […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">The concern here was that 'endpoint' info may not contain 'fromdomain' info, so in order to prevent and issues we're verifying that information.  If it's NULL then return 'invalid' or NULL.<br>from_domain will not always return something, it will return NULL if endpoint is invalid.<br>Are you saying that this check is unnecessary, since endpoint cannot be NULL at this point in the logic, so we can just do this:<br>from_domain = !ast_strlen_zero(endpoint->fromdomain) ? endpoint->fromdomain : invalid;</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/15763/1/res/res_pjsip_dialog_info_body_generator.c@192">Patch Set #1, Line 192:</a> <code style="font-family:monospace,monospace">                    ast_debug(3, "From Domain remote URI domain '%s'\n", sanitized_remote);</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">If this debug message is to remain it should be extended with additional information about what this […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">This is really just for debug in development, and does not need to remain in the codebase.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/15763/1/res/res_pjsip_dialog_info_body_generator.c@207">Patch Set #1, Line 207:</a> <code style="font-family:monospace,monospace">                 need = strlen(connected_num) + (connected_num_restricted ? strlen(invalid) :</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">remote_target has a fixed maximum size, I don't see why this needs to be calculated and given to snp […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">I didn't write this line, I presume it's to prevent injection and buffer overflow.  remote_target max size is the limitation of PJSIP, but once we know the actual length of our data it would probably be better to set it first. Is there a reason to change this?</p></li></ul></li></ul><p>To view, visit <a href="https://gerrit.asterisk.org/c/asterisk/+/15763">change 15763</a>. To unsubscribe, or for help writing mail filters, visit <a href="https://gerrit.asterisk.org/settings">settings</a>.</p><div itemscope itemtype="http://schema.org/EmailMessage"><div itemscope itemprop="action" itemtype="http://schema.org/ViewAction"><link itemprop="url" href="https://gerrit.asterisk.org/c/asterisk/+/15763"/><meta itemprop="name" content="View Change"/></div></div>

<div style="display:none"> Gerrit-Project: asterisk </div>
<div style="display:none"> Gerrit-Branch: 16 </div>
<div style="display:none"> Gerrit-Change-Id: I20c5cf5b45f34d7179df6573c5abf863eb72964b </div>
<div style="display:none"> Gerrit-Change-Number: 15763 </div>
<div style="display:none"> Gerrit-PatchSet: 1 </div>
<div style="display:none"> Gerrit-Owner: Joe <ynadiv@corpit.xyz> </div>
<div style="display:none"> Gerrit-Reviewer: Joshua Colp <jcolp@sangoma.com> </div>
<div style="display:none"> Gerrit-CC: Friendly Automation </div>
<div style="display:none"> Gerrit-Comment-Date: Wed, 14 Apr 2021 17:33:15 +0000 </div>
<div style="display:none"> Gerrit-HasComments: Yes </div>
<div style="display:none"> Gerrit-Has-Labels: No </div>
<div style="display:none"> Comment-In-Reply-To: Joshua Colp <jcolp@sangoma.com> </div>
<div style="display:none"> Gerrit-MessageType: comment </div>