<p>Andrey <strong>posted comments</strong> on this change.</p><p><a href="https://gerrit.asterisk.org/6158">View Change</a></p><p>Patch set 4:</p><p style="white-space: pre-wrap; word-wrap: break-word;">Fixed a lot of remarks.</p><p>(13 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;"> ; for more details.<br> ; For test reasons you can obtain one on the page<br></pre></blockquote></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">I think we need a more complete guide on how to actually use this for users</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</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@778">Patch Set #3, Line 778:</a> <code style="font-family:monospace,monospace">==</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Shouldn't this be '&&'?</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">No, it's XOR logic: if no password and no token -- it's an error; if password and token presents -- it's an error as well due to not clear what to use: token or password?</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/6158/3/res/res_xmpp.c@779">Patch Set #3, Line 779:</a> <code style="font-family:monospace,monospace">either</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">s/either/or/</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">No, _either_ is exactly what this means.</p></li><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_DEBUG, "Obtaining OAuth access token for client %s\n" , client->name);</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">I don't think this should be a notice but instead a debug message, and it's</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</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"></code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Should be on the following line</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</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><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">The { should be on the next line.</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/6158/3/res/res_xmpp.c@3905">Patch Set #3, Line 3905:</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;">{<br> RAII_VAR(char *,<br></pre></blockquote></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">I'd use ast_calloc or ast_asprintf to allocate these. 8K is a little big f</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Left cBuf[1024] only, suppose it's satisfied.</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"> ast_asprintf(&cmd, "CURL(%s,client_id=%s&client_secret=%s&refresh_token=%s&grant_type=refresh_token)",</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">ast_asprintf can be used here to produce the cmd, so that a 4k buffer doesn</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</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_DEBUG, "Command %s\n" , cmd);</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">This appears to be a debug message while developing this.</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</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"> if (!ast_func_read(NULL, cmd, cBuf, sizeof(cBuf) - 1)) {</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">This adds a dependency on CURL for this which isn't documented and should b</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</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_ERROR, "CURL is disabled. Add 'load = func_curl.so' to modules.conf\n", cBuf);</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">This appears to be a debug message while developing this.</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</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"> if (jobj != NULL) {</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Is the token guaranteed to be the same or less than the existing token? If </blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">From time to time tokens have approximately the same length. If you aware about the problem, let me know how to be in this case?</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;"> const char *token = ast_json_string_get(ast_json_object_get(jobj, "access_token"));<br> if (token != NULL) {<br> ast_string_field_set(cfg, password, token);<br> return;<br> }<br> }<br></pre></blockquote></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">These log messages aren't useful to users for when something goes wrong. It</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</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: 4 </div>
<div style="display:none"> Gerrit-Owner: Andrey <andr06@gmail.com> </div>
<div style="display:none"> Gerrit-Reviewer: Andrey <andr06@gmail.com> </div>
<div style="display:none"> Gerrit-Reviewer: George Joseph <gjoseph@digium.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-Reviewer: Sean Bright <sean.bright@gmail.com> </div>
<div style="display:none"> Gerrit-Comment-Date: Wed, 09 Aug 2017 20:02:43 +0000 </div>
<div style="display:none"> Gerrit-HasComments: Yes </div>