<p> Attention is currently required from: Sean Bright, Alec Davis, N A. </p>
<p>Patch set 13:<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/+/18829">View Change</a></p><p>17 comments:</p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0;"><p><a href="null">Commit Message:</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/+/18829/comment/c96d7d7d_0b1b432b">Patch Set #12, Line 9:</a> <code style="font-family:monospace,monospace">Simplify, Simplify.</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">The commit message is what will go into Asterisk, and what will show up in the changelog and such. It should reflect what is going into Asterisk and not the history of its development such as that it is now a single module.</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="null">File configs/samples/hiredis.conf.sample:</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/+/18829/comment/aa16388f_455017f3">Patch Set #12, Line 3:</a> <code style="font-family:monospace,monospace">;server = 127.0.0.1 ; server[:port] of Redis server to use, default=127.0.0.1.</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Does this support IPv6 as well? If so, it should be stated</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="null">File res/res_hiredis.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/+/18829/comment/51694e89_1fc2ffa4">Patch Set #12, Line 137:</a> <code style="font-family:monospace,monospace">#define MAX_PREFIX 40</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Document what this is for, same for STR_CONF_SZ</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/18829/comment/c0945bc0_d68d591d">Patch Set #12, Line 166:</a> <code style="font-family:monospace,monospace"> struct conf_global_options* global;</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">*global</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/18829/comment/1ecb399e_140e46a5">Patch Set #12, Line 204:</a> <code style="font-family:monospace,monospace"> ast_log(AST_LOG_ERROR, "Couldn't establish connection to %s:%d socket_fd[%d] Reason: %s\n", \</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">You can use ast_sockaddr_stringify_fmt to have the port automatically put in</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/18829/comment/33d79c4b_be8f9ad9">Patch Set #12, Line 213:</a> <code style="font-family:monospace,monospace"> ast_debug(6, "socket_fd[%d]\n", hiredis_context->fd);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">I don't think this is useful</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/18829/comment/de0d3523_67f80506">Patch Set #12, Line 243:</a> <code style="font-family:monospace,monospace"> memcpy(data, hiredis_context, sizeof(redisContext));</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">I don't understand how this works. You're copying the contents of hiredis_context over, but in the next line you seemingly free it?</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/18829/comment/5227417b_0c95fdc7">Patch Set #12, Line 244:</a> <code style="font-family:monospace,monospace"> ast_free(hiredis_context);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Should this be using redisFree?</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/18829/comment/a8c5faa4_834aefaf">Patch Set #12, Line 249:</a> <code style="font-family:monospace,monospace">static void hiredis_disconnect(void* data)</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Document that this is a void pointer because the redisContext is stored directly using a threadstorage, instead of wrapped in some way.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/18829/comment/aa119571_24c8c57b">Patch Set #12, Line 254:</a> <code style="font-family:monospace,monospace"> ast_log(AST_LOG_ERROR, "No hiredis_context. Reason: UNKNOWN\n");</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">This feels more like a coding error, then something a user can really solve or should know about it so I don't think the log message is really valuable as an error.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/18829/comment/bcf10b1e_7752410b">Patch Set #12, Line 258:</a> <code style="font-family:monospace,monospace"> ast_debug(6, "socket_fd[%d]\n", hiredis_context->fd);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">I don't think this is useful.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/18829/comment/e0bd533a_4d53fd0f">Patch Set #12, Line 289:</a> <code style="font-family:monospace,monospace"> if (attempt++ < 3) {</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">This retry logic should be documented, such as in the conf file and XML</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/18829/comment/1de64399_a6625bb1">Patch Set #12, Line 302:</a> <code style="font-family:monospace,monospace"> ast_log(AST_LOG_ERROR, "%s\n", reply->str);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">I think the error message should include something like: "Received error 'blah' while executing redis command"</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/18829/comment/f6fab9b3_3dbc7054">Patch Set #12, Line 539:</a> <code style="font-family:monospace,monospace"> pbx_builtin_setvar_helper(chan, "REDIS_RESULT", buf);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">What is the reasoning behind also setting REDIS_RESULT? Dialplan functions don't also set dialplan variables, they're meant to return the information themselves only.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/18829/comment/5259be5f_be590f3b">Patch Set #12, Line 763:</a> <code style="font-family:monospace,monospace"> redisFree(hiredis_context);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Would this result in a double free when the threadstorage gets destroyed?</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/18829/comment/a4a2107f_d2166f45">Patch Set #12, Line 783:</a> <code style="font-family:monospace,monospace"> hiredis_shutdown();</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">This doesn't actually shut down all connections, because all other threads won't shut down theirs. What state does this leave them in? I think either unload can't be allowed, or module reference counting has to be used so that if any thread is using a redis connection then it can't be unloaded.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/18829/comment/5e0e080d_11274302">Patch Set #12, Line 850:</a> <code style="font-family:monospace,monospace"> if (hiredis_init()) {</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">This will just keep a connection open for no reason on the main Asterisk thread, or CLI thread.</p></li></ul></li></ul><p>To view, visit <a href="https://gerrit.asterisk.org/c/asterisk/+/18829">change 18829</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/+/18829"/><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: Ifdf4d33d2a2a5fd044fac13b201c7437de34ba6a </div>
<div style="display:none"> Gerrit-Change-Number: 18829 </div>
<div style="display:none"> Gerrit-PatchSet: 13 </div>
<div style="display:none"> Gerrit-Owner: Alec Davis <alec@bdt.co.nz> </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-Reviewer: N A <mail@interlinked.x10host.com> </div>
<div style="display:none"> Gerrit-Reviewer: Sean Bright <sean@seanbright.com> </div>
<div style="display:none"> Gerrit-Attention: Sean Bright <sean@seanbright.com> </div>
<div style="display:none"> Gerrit-Attention: Alec Davis <alec@bdt.co.nz> </div>
<div style="display:none"> Gerrit-Attention: N A <mail@interlinked.x10host.com> </div>
<div style="display:none"> Gerrit-Comment-Date: Thu, 28 Jul 2022 09:30:08 +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>