<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/+/16634">View Change</a></p><p>8 comments:</p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0;"><p><a href="null">File funcs/func_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/+/16634/comment/0b78f780_34f8c3ce">Patch Set #2, Line 50:</a> <code style="font-family:monospace,monospace">                        <para>The JSON_DECODE function parses a JSON string and returns a value by key.</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">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.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/16634/comment/7977d73b_ad2b3950">Patch Set #2, Line 63:</a> <code style="font-family:monospace,monospace">       RAII_VAR(struct ast_json *, json, NULL, ast_json_free);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">RAII_VAR isn't really needed because you can just call ast_json_free after use below, there isn't any additional return paths.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/16634/comment/e6ff52ac_8d66dd6e">Patch Set #2, Line 64:</a> <code style="font-family:monospace,monospace"> RAII_VAR(struct ast_json *, jsonresult, NULL, ast_json_free);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">This doesn't appear to be used anywhere</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/16634/comment/5e01c3ac_9b560ed6">Patch Set #2, Line 74:</a> <code style="font-family:monospace,monospace">      AST_STANDARD_APP_ARGS(args, data);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">This doesn't handle if varname or key are empty.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/16634/comment/22d24f52_b26f2448">Patch Set #2, Line 76:</a> <code style="font-family:monospace,monospace">        varsubst = ast_alloca(strlen(args.varname) + 4);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Document why the 4 with a comment, and this could fail.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/16634/comment/0378ad3c_318ddae9">Patch Set #2, Line 77:</a> <code style="font-family:monospace,monospace">       sprintf(varsubst, "${%s}", args.varname);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">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.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/16634/comment/aafb2192_9076741b">Patch Set #2, Line 80:</a> <code style="font-family:monospace,monospace">             ast_debug(1, "String was empty\n");</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">This should be more descriptive if at all possible - such as giving the channel name and the variable name.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/16634/comment/f7466025_5c4eff0c">Patch Set #2, Line 83:</a> <code style="font-family:monospace,monospace">      jsonstring = ast_str_buffer(str);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Why is there a need for jsonstring at all if you can just use ast_str_buffer?</p></li></ul></li></ul><p>To view, visit <a href="https://gerrit.asterisk.org/c/asterisk/+/16634">change 16634</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/+/16634"/><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: Iea60c49a7358dfdc2db60803cdc9a742f808ba2c </div>
<div style="display:none"> Gerrit-Change-Number: 16634 </div>
<div style="display:none"> Gerrit-PatchSet: 2 </div>
<div style="display:none"> Gerrit-Owner: N A <mail@interlinked.x10host.com> </div>
<div style="display:none"> Gerrit-Reviewer: Friendly Automation </div>
<div style="display:none"> Gerrit-Reviewer: Joshua Colp <jcolp@sangoma.com> </div>
<div style="display:none"> Gerrit-Attention: N A <mail@interlinked.x10host.com> </div>
<div style="display:none"> Gerrit-Comment-Date: Tue, 16 Nov 2021 11:55:02 +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>