[Asterisk-code-review] func_json: Adds JSON_DECODE function (asterisk[master])

[Joshua Colp]

Attention is currently required from: N A.
Joshua Colp has posted comments on this change. ( https://gerrit.asterisk.org/c/asterisk/+/16634 )

Change subject: func_json: Adds JSON_DECODE function
......................................................................


Patch Set 2: Code-Review-1

(8 comments)

File funcs/func_json.c:

https://gerrit.asterisk.org/c/asterisk/+/16634/comment/0b78f780_34f8c3ce 
PS2, Line 50: 			<para>The JSON_DECODE function parses a JSON string and returns a value by key.
This is not documented enough. It should state that it retrieves the value of the given variable name and parses it as JSON. As it is, a normal user could interpret this as directly taking JSON and not a variable name.


https://gerrit.asterisk.org/c/asterisk/+/16634/comment/7977d73b_ad2b3950 
PS2, Line 63: 	RAII_VAR(struct ast_json *, json, NULL, ast_json_free);
RAII_VAR isn't really needed because you can just call ast_json_free after use below, there isn't any additional return paths.


https://gerrit.asterisk.org/c/asterisk/+/16634/comment/e6ff52ac_8d66dd6e 
PS2, Line 64: 	RAII_VAR(struct ast_json *, jsonresult, NULL, ast_json_free);
This doesn't appear to be used anywhere


https://gerrit.asterisk.org/c/asterisk/+/16634/comment/5e01c3ac_9b560ed6 
PS2, Line 74: 	AST_STANDARD_APP_ARGS(args, data);
This doesn't handle if varname or key are empty.


https://gerrit.asterisk.org/c/asterisk/+/16634/comment/22d24f52_b26f2448 
PS2, Line 76: 	varsubst = ast_alloca(strlen(args.varname) + 4);
Document why the 4 with a comment, and this could fail.


https://gerrit.asterisk.org/c/asterisk/+/16634/comment/0378ad3c_318ddae9 
PS2, Line 77: 	sprintf(varsubst, "${%s}", args.varname);
Comment that this is safe because of the above allocation so if someone runs an automatic tool there's a chance they will more prominently see that it's fine.


https://gerrit.asterisk.org/c/asterisk/+/16634/comment/aafb2192_9076741b 
PS2, Line 80: 		ast_debug(1, "String was empty\n");
This should be more descriptive if at all possible - such as giving the channel name and the variable name.


https://gerrit.asterisk.org/c/asterisk/+/16634/comment/f7466025_5c4eff0c 
PS2, Line 83: 	jsonstring = ast_str_buffer(str);
Why is there a need for jsonstring at all if you can just use ast_str_buffer?



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

Gerrit-Project: asterisk
Gerrit-Branch: master
Gerrit-Change-Id: Iea60c49a7358dfdc2db60803cdc9a742f808ba2c
Gerrit-Change-Number: 16634
Gerrit-PatchSet: 2
Gerrit-Owner: N A <mail at interlinked.x10host.com>
Gerrit-Reviewer: Friendly Automation
Gerrit-Reviewer: Joshua Colp <jcolp at sangoma.com>
Gerrit-Attention: N A <mail at interlinked.x10host.com>
Gerrit-Comment-Date: Tue, 16 Nov 2021 11:55:02 +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/20211116/a16cb3b9/attachment.html>