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

[N A]

Attention is currently required from: Sean Bright.

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

Change subject: res_pjsip_stir_shaken: Fix JSON field ordering and disallowed TN characters.
......................................................................


Patch Set 6:

(6 comments)

Commit Message:

https://gerrit.asterisk.org/c/asterisk/+/19897/comment/7cefa605_40f8e15f 
PS2, Line 7: res_pjsip_stir_shaken: Fix JSON field ordering and canonicalization.
> "Canonicalization" in the context of RFC 8224 § 8. […]
Done


Patchset:

PS6: 
Not sure yet what's up with the json test failure, I can't reproduce that on any of my machines currently (tests all pass). In the meantime:


File include/asterisk/json.h:

https://gerrit.asterisk.org/c/asterisk/+/19897/comment/f4255b0d_961ecb29 
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_ […]
I think this makes sense. I'm not a huge fan of the flags being mutually exclusive internally (since the enum doesn't specify the values), but it's probably fine for these 3 options.


File main/json.c:

https://gerrit.asterisk.org/c/asterisk/+/19897/comment/83545d7d_2f4f7697 
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 d […]
Done


File res/res_pjsip_stir_shaken.c:

https://gerrit.asterisk.org/c/asterisk/+/19897/comment/ff56fa94_b864f88b 
PS2, Line 378: 	char *canonical_num;
> You don't need this, you can just use `dest_tn` directly. […]
Done


https://gerrit.asterisk.org/c/asterisk/+/19897/comment/9d076cac_d7028ac5 
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. […]
Done



-- 
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: 6
Gerrit-Owner: N A <asterisk at phreaknet.org>
Gerrit-Reviewer: Friendly Automation
Gerrit-Reviewer: Sean Bright <sean at seanbright.com>
Gerrit-Attention: Sean Bright <sean at seanbright.com>
Gerrit-Comment-Date: Tue, 28 Feb 2023 02:00:40 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Sean Bright <sean at seanbright.com>
Gerrit-MessageType: comment
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20230228/9e18fe62/attachment.html>