<p> Attention is currently required from: Sean Bright, Joshua Colp. </p>
<p><a href="https://gerrit.asterisk.org/c/asterisk/+/18829">View Change</a></p><p>18 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/577802df_c80baba1">Patch Set #12, Line 9:</a> <code style="font-family:monospace,monospace">Simplify, Simplify.</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">The commit message is what will go into Asterisk, and what will show up in the changelog and such. […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="null">Patchset:</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?tab=comments">Patch Set #15:</a> </p><p style="white-space: pre-wrap; word-wrap: break-word;">Hopefully I got everything, either sorted or at least answered.</p><p style="white-space: pre-wrap; word-wrap: break-word;">The reason for all of the function to have variadic was that to add a key value with spaces required it, otherwise the hiredis library returned a syntax error.</p><pre style="font-family: monospace,monospace; white-space: pre-wrap;">Following the hiredis example from https://github.com/redis/hiredis/blob/master/examples/example.c<br>    /* Set a key */<br>    reply = redisCommand(c,"SET %s %s", "foo", "hello world");<br>    printf("SET: %s\n", reply->str);<br>    freeReplyObject(reply);</pre></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/22c611ae_2bf8aa81">Patch Set #12, Line 137:</a> <code style="font-family:monospace,monospace">#define MAX_PREFIX 40</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Document what this is for, same for STR_CONF_SZ</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</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/2acc199d_874b6c53">Patch Set #12, Line 166:</a> <code style="font-family:monospace,monospace">  struct conf_global_options* global;</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">*global</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done.</p><p style="white-space: pre-wrap; word-wrap: break-word;">Reference code was res_statsd.c</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/b2971f0a_c4d47cb3">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><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">You can use ast_sockaddr_stringify_fmt to have the port automatically put in</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</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/128aaff6_627c96c9">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><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">I don't think this is useful</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Going forward not any more, but was in early days, as we had 100's of fd leaks.</p><p style="white-space: pre-wrap; word-wrap: break-word;">Removed.</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/b96a5019_9df3da0b">Patch Set #12, Line 243:</a> <code style="font-family:monospace,monospace">    memcpy(data, hiredis_context, sizeof(redisContext));</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">I don't understand how this works. […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">reworked hiredis_connect, new variable to indicate that the context is temporary.</p><p style="white-space: pre-wrap; word-wrap: break-word;">tmp_hiredis_context is allocated a new context when redisConnectWithTimeout<br>so has to be copied back to threadstorage, then free'd</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/3e787784_aed895c4">Patch Set #12, Line 244:</a> <code style="font-family:monospace,monospace"> ast_free(hiredis_context);</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Should this be using redisFree?</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">nope, after rework, hopefully this is clearer.</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/33985d25_d030244c">Patch Set #12, Line 249:</a> <code style="font-family:monospace,monospace">static void hiredis_disconnect(void* data)</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Document that this is a void pointer because the redisContext is stored directly using a threadstora […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</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/2ceaeb9b_8f0e324b">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><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">This feels more like a coding error, then something a user can really solve or should know about it  […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">removed log message.</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/bb0b6b18_ebbe9deb">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><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">I don't think this is useful.</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Was useful to debug fd leak that happened just by unloading and reloading module.</p><p style="white-space: pre-wrap; word-wrap: break-word;">Also see hiredis_shutdown comments.</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/4d2f5a2b_3f44c399">Patch Set #12, Line 289:</a> <code style="font-family:monospace,monospace">          if (attempt++ < 3) {</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">This retry logic should be documented, such as in the conf file and XML</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</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/7ab7979e_12ab37fe">Patch Set #12, Line 302:</a> <code style="font-family:monospace,monospace">          ast_log(AST_LOG_ERROR, "%s\n", reply->str);</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">I think the error message should include something like: "Received error 'blah' while executing redi […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">changed to:<br>ast_log(AST_LOG_ERROR, "Received error [%s] while executing Redis command [%s]\n", reply->str, cmd_string);</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/67787f9f_24ca9751">Patch Set #12, Line 539:</a> <code style="font-family:monospace,monospace">        pbx_builtin_setvar_helper(chan, "REDIS_RESULT", buf);</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">What is the reasoning behind also setting REDIS_RESULT? Dialplan functions don't also set dialplan v […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">The functions are a direct copy of func_db.c</p><p style="white-space: pre-wrap; word-wrap: break-word;">copy and rename from func_db:function_db_exists</p><p style="white-space: pre-wrap; word-wrap: break-word;">Now have removed all instances of REDIS_RESULT</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/899a3058_703f0259">Patch Set #12, Line 763:</a> <code style="font-family:monospace,monospace">                redisFree(hiredis_context);</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Would this result in a double free when the threadstorage gets destroyed?</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">The thread that calls unload_module never exited, thus never called hiredis_disconnect as defined below.</p><p style="white-space: pre-wrap; word-wrap: break-word;">AST_THREADSTORAGE_CUSTOM(hiredis_instance, hiredis_connect, hiredis_disconnect)</p><p style="white-space: pre-wrap; word-wrap: break-word;">So unloading and reloading the module caused an fd leak.</p><p style="white-space: pre-wrap; word-wrap: break-word;">This fixed the fd leak issue, but may cause a problem when the main thread exits.</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/502c3a39_0c3b7df5">Patch Set #12, Line 783:</a> <code style="font-family:monospace,monospace">       hiredis_shutdown();</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">This doesn't actually shut down all connections, because all other threads won't shut down theirs. […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">I presume when you say 'connections' you're referring to sockets connected to redis?</p><p style="white-space: pre-wrap; word-wrap: break-word;">There is only 1 long lasting connection and that is the deviceState one, the others only last the length of a redis command / reply.</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/78f1deec_9c0bba14">Patch Set #12, Line 850:</a> <code style="font-family:monospace,monospace">  if (hiredis_init()) {</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">This will just keep a connection open for no reason on the main Asterisk thread, or CLI thread.</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</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/31db3554_d47e73cb">Patch Set #13, Line 302:</a> <code style="font-family:monospace,monospace">      char cmd_string[256]; </code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Fixed redness</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: 15 </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: Joshua Colp <jcolp@sangoma.com> </div>
<div style="display:none"> Gerrit-Comment-Date: Fri, 29 Jul 2022 12:35:02 +0000 </div>
<div style="display:none"> Gerrit-HasComments: Yes </div>
<div style="display:none"> Gerrit-Has-Labels: No </div>
<div style="display:none"> Comment-In-Reply-To: Joshua Colp <jcolp@sangoma.com> </div>
<div style="display:none"> Gerrit-MessageType: comment </div>