[Asterisk-code-review] res_pjsip_stir_shaken: Fix JSON field ordering and canonicalization. (asterisk[master])

[Sean Bright]

Attention is currently required from: N A.

Sean Bright has posted comments on this change. ( https://gerrit.asterisk.org/c/asterisk/+/19897 )

Change subject: res_pjsip_stir_shaken: Fix JSON field ordering and canonicalization.
......................................................................


Patch Set 2: Code-Review-1

(5 comments)

Commit Message:

https://gerrit.asterisk.org/c/asterisk/+/19897/comment/6e175636_61b728ca 
PS2, Line 7: res_pjsip_stir_shaken: Fix JSON field ordering and canonicalization.
"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.

This would apply to any place where we use any form of the word canonical either in the commit message or code itself.


File include/asterisk/json.h:

https://gerrit.asterisk.org/c/asterisk/+/19897/comment/f4c9e0ea_40713c61 
PS2, Line 816: char *ast_json_dump_string_sorted(struct ast_json *root);
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?


File main/json.c:

https://gerrit.asterisk.org/c/asterisk/+/19897/comment/352bdd08_cb93eb50 
PS2, Line 460: 		JSON_INDENT(2) | JSON_PRESERVE_ORDER : JSON_COMPACT;
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.


File res/res_pjsip_stir_shaken.c:

https://gerrit.asterisk.org/c/asterisk/+/19897/comment/a7199cdf_c6619908 
PS2, Line 378: 	char *canonical_num;
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.


https://gerrit.asterisk.org/c/asterisk/+/19897/comment/030bb0a2_dc898ba4 
PS2, Line 405: /* Canonicalize the telephone number according to RFC 8224 (required by RFC 8225) */
             : 	canonical_num = ast_malloc(strlen(dest_tn) + 1);
             : 	if (!canonical_num) {
             : 		return -1;
             : 	} else {
             : 		const char *s = dest_tn;
             : 		char *new = canonical_num;
             : 		/* We're only removing characters, if anything, so the buffer is guaranteed to be large enough */
             : 		while (*s) {
             : 			if (isdigit(*s) || *s == '#' || *s == '*') { /* Only characters allowed */
             : 				*new++ = *s;
             : 			}
             : 			s++;
             : 		}
             : 		*new = '\0';
             : 		ast_debug(4, "Canonicalized telephone number %s -> %s\n", dest_tn, canonical_num);
             : 	}
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.

Also see my previous comments about referring to this as "canonicalization."



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

Gerrit-Project: asterisk
Gerrit-Branch: master
Gerrit-Change-Id: Iab76d39447c4b8cf133de85657dba02fda07f9a2
Gerrit-Change-Number: 19897
Gerrit-PatchSet: 2
Gerrit-Owner: N A <asterisk at phreaknet.org>
Gerrit-Reviewer: Friendly Automation
Gerrit-Reviewer: Sean Bright <sean at seanbright.com>
Gerrit-Attention: N A <asterisk at phreaknet.org>
Gerrit-Comment-Date: Thu, 23 Feb 2023 17:39:21 +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/20230223/38227953/attachment-0001.html>