<p> Attention is currently required from: N A. </p>
<p>Patch set 2:<span style="border-radius: 3px; display: inline-block; margin: 0 2px; padding: 4px;background-color: #ffd4d4; color: #000000;">Code-Review -1</span></p><p><a href="https://gerrit.asterisk.org/c/asterisk/+/19897">View Change</a></p><p>5 comments:</p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0;"><p><a href="null">Commit Message:</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/+/19897/comment/6e175636_61b728ca">Patch Set #2, Line 7:</a> <code style="font-family:monospace,monospace">res_pjsip_stir_shaken: Fix JSON field ordering and canonicalization.</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">"Canonicalization" in the context of RFC 8224 § 8.3 encompasses more that just character filtering so we should use different terminology here to avoid confusing users. Maybe "partial canonicalization?" but that doesn't seem much clearer. I'll defer to you on this.</p><p style="white-space: pre-wrap; word-wrap: break-word;">This would apply to any place where we use any form of the word canonical either in the commit message or code itself.</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="null">File include/asterisk/json.h:</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/+/19897/comment/f4c9e0ea_40713c61">Patch Set #2, Line 816:</a> <code style="font-family:monospace,monospace">char *ast_json_dump_string_sorted(struct ast_json *root);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Rather than adding new API can we just add a new member to ast_json_encoding_format and update dump_flags() in json.c to handle adding the JSON_SORT_KEYS flag during serialization? So you would just call `ast_json_dump_string_format(... AST_JSON_SORT_KEYS)` instead of creating a dedicated function?</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="null">File main/json.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/+/19897/comment/352bdd08_cb93eb50">Patch Set #2, Line 460:</a> <code style="font-family:monospace,monospace"> JSON_INDENT(2) | JSON_PRESERVE_ORDER : JSON_COMPACT;</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">You can remove `JSON_PRESERVE_ORDER` entirely as part of updating `dump_flags()` since it has been deprecated since jansson 2.8 (it's the default) and we require 2.11.</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="null">File res/res_pjsip_stir_shaken.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/+/19897/comment/a7199cdf_c6619908">Patch Set #2, Line 378:</a> <code style="font-family:monospace,monospace"> char *canonical_num;</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">You don't need this, you can just use `dest_tn` directly. `dest_tn` also cleans itself up with `RAII_VAR()` so you won't need to manually free it later.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/19897/comment/030bb0a2_dc898ba4">Patch Set #2, Line 405:</a> </p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><pre style="font-family: monospace,monospace; white-space: pre-wrap;">/* Canonicalize the telephone number according to RFC 8224 (required by RFC 8225) */<br> canonical_num = ast_malloc(strlen(dest_tn) + 1);<br> if (!canonical_num) {<br> return -1;<br> } else {<br> const char *s = dest_tn;<br> char *new = canonical_num;<br> /* We're only removing characters, if anything, so the buffer is guaranteed to be large enough */<br> while (*s) {<br> if (isdigit(*s) || *s == '#' || *s == '*') { /* Only characters allowed */<br> *new++ = *s;<br> }<br> s++;<br> }<br> *new = '\0';<br> ast_debug(4, "Canonicalized telephone number %s -> %s\n", dest_tn, canonical_num);<br> }<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Do all of this with `dest_tn` above (at line 397) instead of doing another allocation. Doing it this way will also avoid the call to `strlen(…)`. So basically skip the call to `ast_copy_pj_str(…)` above and replace it with this code.</p><p style="white-space: pre-wrap; word-wrap: break-word;">Also see my previous comments about referring to this as "canonicalization."</p></li></ul></li></ul><p>To view, visit <a href="https://gerrit.asterisk.org/c/asterisk/+/19897">change 19897</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/+/19897"/><meta itemprop="name" content="View Change"/></div></div>
<div style="display:none"> Gerrit-Project: asterisk </div>
<div style="display:none"> Gerrit-Branch: master </div>
<div style="display:none"> Gerrit-Change-Id: Iab76d39447c4b8cf133de85657dba02fda07f9a2 </div>
<div style="display:none"> Gerrit-Change-Number: 19897 </div>
<div style="display:none"> Gerrit-PatchSet: 2 </div>
<div style="display:none"> Gerrit-Owner: N A <asterisk@phreaknet.org> </div>
<div style="display:none"> Gerrit-Reviewer: Friendly Automation </div>
<div style="display:none"> Gerrit-Reviewer: Sean Bright <sean@seanbright.com> </div>
<div style="display:none"> Gerrit-Attention: N A <asterisk@phreaknet.org> </div>
<div style="display:none"> Gerrit-Comment-Date: Thu, 23 Feb 2023 17:39:21 +0000 </div>
<div style="display:none"> Gerrit-HasComments: Yes </div>
<div style="display:none"> Gerrit-Has-Labels: Yes </div>
<div style="display:none"> Gerrit-MessageType: comment </div>