<p>Joshua Colp <strong>posted comments</strong> on this change.</p><p><a href="https://gerrit.asterisk.org/6158">View Change</a></p><p>Patch set 3:<span style="border-radius: 3px; display: inline-block; margin: 0 2px; padding: 4px;background-color: #ffd4d4;">Code-Review -1</span></p><p>(9 comments)</p><ul style="list-style: none; padding-left: 20px;"><li><p><a href="https://gerrit.asterisk.org/#/c/6158/3/configs/samples/xmpp.conf.sample">File configs/samples/xmpp.conf.sample:</a></p><ul style="list-style: none; padding-left: 20px;"><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/6158/3/configs/samples/xmpp.conf.sample@24">Patch Set #3, Line 24:</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;">;oauth_secret=OAUTH_SECRET_VALUE ; The application's secret to authorize using Google OAuth 2.0 protocol.<br> ; Create new one on https://console.cloud.google.com/apis/credentials/oauthclient<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">I think we need a more complete guide on how to actually use this for users.</p></li></ul></li><li><p><a href="https://gerrit.asterisk.org/#/c/6158/3/res/res_xmpp.c">File res/res_xmpp.c:</a></p><ul style="list-style: none; padding-left: 20px;"><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/6158/3/res/res_xmpp.c@3658">Patch Set #3, Line 3658:</a> <code style="font-family:monospace,monospace"> ast_log(LOG_NOTICE , "Connecting to client token: %s\n" , clientcfg->refresh_token);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">I don't think this should be a notice but instead a debug message, and it's also not completely useful in its current form. This should also include the client name.</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/6158/3/res/res_xmpp.c@3904">Patch Set #3, Line 3904:</a> <code style="font-family:monospace,monospace">static void fetch_access_token(struct ast_xmpp_client_config *cfg) {</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">The { should be on the next line.</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/6158/3/res/res_xmpp.c@3912">Patch Set #3, Line 3912:</a> <code style="font-family:monospace,monospace"> snprintf(cmd, sizeof(cmd) - 1 , "CURL(%s,client_id=%s&client_secret=%s&refresh_token=%s&grant_type=refresh_token)",</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">ast_asprintf can be used here to produce the cmd, so that a 4k buffer doesn't need to be used.</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/6158/3/res/res_xmpp.c@3915">Patch Set #3, Line 3915:</a> <code style="font-family:monospace,monospace"> ast_log(LOG_NOTICE, "Command %s\n" , cmd);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">This appears to be a debug message while developing this.</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/6158/3/res/res_xmpp.c@3918">Patch Set #3, Line 3918:</a> <code style="font-family:monospace,monospace"> ast_func_read(NULL, cmd, cBuf, sizeof(cBuf) - 1);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">This adds a dependency on CURL for this which isn't documented and should be better handled here - if the dependency isn't met then a message needs to be output so the user understands what is going on.</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/6158/3/res/res_xmpp.c@3919">Patch Set #3, Line 3919:</a> <code style="font-family:monospace,monospace"> ast_log(LOG_NOTICE, "Command status: %s\n", cBuf);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">This appears to be a debug message while developing this.</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/6158/3/res/res_xmpp.c@3925">Patch Set #3, Line 3925:</a> <code style="font-family:monospace,monospace"> ast_string_field_set(cfg, password, token);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Is the token guaranteed to be the same or less than the existing token? If not this can have a memory growth issue. String fields will only reuse the existing memory if the string is less than or equal to the length of the existing string that is there. If not then more memory is used, and the other allocation goes unused until the stringfields are released.</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/6158/3/res/res_xmpp.c@3926">Patch Set #3, Line 3926:</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;"> ast_log(LOG_NOTICE, "Access Token: %s\n", token);<br> } else {<br> ast_log(LOG_ERROR, "OAuth object is wrong\n");<br> }<br> } else {<br> ast_log(LOG_ERROR, "OAuth object is NULL\n");<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">These log messages aren't useful to users for when something goes wrong. It doesn't elaborate on what really went wrong or how to solve the problem.</p></li></ul></li></ul><p>To view, visit <a href="https://gerrit.asterisk.org/6158">change 6158</a>. To unsubscribe, 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/6158"/><meta itemprop="name" content="View Change"/></div></div>
<div style="display:none"> Gerrit-Project: asterisk </div>
<div style="display:none"> Gerrit-Branch: 13 </div>
<div style="display:none"> Gerrit-MessageType: comment </div>
<div style="display:none"> Gerrit-Change-Id: I07f7052a502457ab55010a4d3686653b60f4c8db </div>
<div style="display:none"> Gerrit-Change-Number: 6158 </div>
<div style="display:none"> Gerrit-PatchSet: 3 </div>
<div style="display:none"> Gerrit-Owner: Andrey <andr06@gmail.com> </div>
<div style="display:none"> Gerrit-Reviewer: Jenkins2 </div>
<div style="display:none"> Gerrit-Reviewer: Joshua Colp <jcolp@digium.com> </div>
<div style="display:none"> Gerrit-Comment-Date: Mon, 07 Aug 2017 11:13:46 +0000 </div>
<div style="display:none"> Gerrit-HasComments: Yes </div>