<p> Attention is currently required from: Sean Bright, Joshua Colp, Kevin Harwell. </p>
<p><a href="https://gerrit.asterisk.org/c/asterisk/+/17644">View Change</a></p><p>30 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/+/17644/comment/f8dd33c8_f3e63860">Patch Set #3, Line 7:</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;">various: fix and test a double deref on a scheduled delete of an<br>executing call back.<br></pre></blockquote></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Try to shorten this line to 72 characters. Also use sched or sched. […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Thanks, also needed to be updated vs initial commit.</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/+/17644?tab=comments">Patch Set #8:</a> </p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Looks like you've addressed my feedback, but giving a -1 as it appears Sean's has still not been add […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Thanks, I had changed it earlier but didn't update on later amends which over-wrote the change. It has been reverted to reflect Sean's comments.</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="null">File include/asterisk/sched.h:</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/+/17644/comment/9530e4f8_4ec8b850">Patch Set #7, Line 75:</a> <code style="font-family:monospace,monospace"> * Only calls unref function if the delete succeeded.</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">I think this could use additional clarification: […]</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/+/17644/comment/73667d4f_df18fb56">Patch Set #7, Line 315:</a> <code style="font-family:monospace,monospace">* \retval -2 event was running and not rescheduled</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Reword this a bit to say something like "-2 event was running, but deleted via non rescheduling.</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 include/asterisk/sched.h:</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/+/17644/comment/daeab037_acc8be37">Patch Set #5, Line 304:</a> <code style="font-family:monospace,monospace"> * \brief Deletes a scheduled event provided the event is not running</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">I don't think this is descriptive enough, because the event may be running and then get deleted rega […]</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 include/asterisk/sched.h:</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/+/17644/comment/1438d198_e6ba71e9">Patch Set #2, Line 296:</a> <code style="font-family:monospace,monospace"> * \retval -2 event was running and not rescheduled</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">I don't think changing this API call is a good idea, because other users of it may not be written to […]</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 main/sched.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/+/17644/comment/290d4b3b_2e445fda">Patch Set #2, Line 651:</a> <code style="font-family:monospace,monospace"> /* Do not sched_release() here because ast_sched_runq() will do it */</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">This is no longer accurate.</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/+/17644/comment/0d5f338c_d8ec7ed6">Patch Set #2, Line 658:</a> <code style="font-family:monospace,monospace"> sched_release(con, s);</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">"s" gets freed in sched_release, and should be set to NULL after calling sched_release since "s" is […]</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/+/17644/comment/c96cdace_a737b737">Patch Set #2, Line 658:</a> <code style="font-family:monospace,monospace"> sched_release(con, s);</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Add a comment that ast_sched_runq passes responsibility for release to us in this case.</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/+/17644/comment/a8e927e4_34654fad">Patch Set #2, Line 807:</a> <code style="font-family:monospace,monospace"> current->rescheduled = res ? 1 : 0;</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Add a comment explaining what has happened here (that another thread is waiting on this scheduled it […]</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 main/sched.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/+/17644/comment/38c63089_d7bc00cc">Patch Set #6, Line 623:</a> <code style="font-family:monospace,monospace"> * schedule the relase.</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">release</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 main/sched.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/+/17644/comment/5b6dded2_9a32d92f">Patch Set #7, Line 666:</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;"> /* This was not rescheduled so the caller of ast_sched_del can not remove any<br> * references as they already were.<br> */<br></pre></blockquote></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">I think this comment can be deleted. […]</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/+/17644/comment/10be68c4_5619e8bf">Patch Set #7, Line 673:</a> <code style="font-family:monospace,monospace"> * it's destruction to us</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">s/it's/its</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/+/17644/comment/68c578f5_a8487e66">Patch Set #7, Line 688:</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;"> if (!s && *last_id != id) {<br> ast_debug(1, "Attempted to delete nonexistent schedule entry %d!\n", id);<br> /* Removing nonexistent schedule entry shouldn't trigger assert (it was enabled in DEV_MODE);<br> * because in many places entries is deleted without having valid id. */<br> *last_id = id;<br> return -1;<br> } else if (!s) {<br> return -1;<br> }<br><br> return res;<br></pre></blockquote></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">When you set "s" to NULL above this might need to be reworked as this function will now always retur […]</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 tests/test_sched.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/+/17644/comment/a54b6cd0_7dc147f3">Patch Set #2, Line 366:</a> <code style="font-family:monospace,monospace"> usleep(3000);</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Add a comment explaining why this sleep exists</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/+/17644/comment/066ea8bd_5c645431">Patch Set #2, Line 366:</a> <code style="font-family:monospace,monospace"> usleep(3000);</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">usleep can be interrupted, so this probably needs to be in a loop to ensure the full 3 seconds passe […]</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/+/17644/comment/55c53ed1_66fcafe0">Patch Set #2, Line 384:</a> <code style="font-family:monospace,monospace"> "This test forces a thread conflict issue on res mem sorcery.";</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">This is actually just testing the scheduler</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/+/17644/comment/093690bb_e9adc68a">Patch Set #2, Line 391:</a> <code style="font-family:monospace,monospace"> if (!(obj = ao2_alloc(sizeof(struct test_obj), test_obj_cleanup))) {</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">We generally put the obj = ao2_alloc on a separate line from the if.</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/+/17644/comment/2f243e63_611726df">Patch Set #2, Line 406:</a> <code style="font-family:monospace,monospace"> return AST_TEST_FAIL;</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">obj is leaked</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/+/17644/comment/10bec16c_119f00e4">Patch Set #2, Line 409:</a> <code style="font-family:monospace,monospace"> if(ast_sched_start_thread(con)) {</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Space between if and (</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/+/17644/comment/1c38eb22_299a39b1">Patch Set #2, Line 412:</a> <code style="font-family:monospace,monospace"> return AST_TEST_FAIL;</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">obj is leaked</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/+/17644/comment/29587423_60bd2954">Patch Set #2, Line 419:</a> <code style="font-family:monospace,monospace"> return AST_TEST_FAIL;</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">obj is leaked</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/+/17644/comment/4b7107f6_06d27773">Patch Set #2, Line 423:</a> <code style="font-family:monospace,monospace"> while(obj->scheduledCBstarted == 0) {</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Add a comment explaining what this is waiting on and why</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/+/17644/comment/a6b24cb5_31d8baf4">Patch Set #2, Line 439:</a> <code style="font-family:monospace,monospace"> ast_test_status_update(test, "Inorrect number of references '%d'\n",</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Inorrect is incorrect</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/+/17644/comment/ac68df8d_c231736e">Patch Set #2, Line 444:</a> <code style="font-family:monospace,monospace"></code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">obj is leaked - the easiest way to destroy it is to probably loop until ao2_ref(obj, -1) returns 0.</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 tests/test_sched.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/+/17644/comment/5e96835d_bc7f651d">Patch Set #5, Line 457:</a> <code style="font-family:monospace,monospace"> ao2_cleanup(obj);</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">This can still leak. […]</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 tests/test_sched.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/+/17644/comment/cb4ab88a_b50529ec">Patch Set #6, Line 426:</a> <code style="font-family:monospace,monospace"> while(!ao2_ref(obj, -1));</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">The ao2_ref function returns the current reference count before the operation, so this isn't correct […]</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 tests/test_sched.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/+/17644/comment/b6737288_e7b17e68">Patch Set #7, Line 405:</a> <code style="font-family:monospace,monospace"> ao2_ref(obj, +1);</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">If you move this to just before sched_add then you'll no longer leak "obj" in the below off nominal […]</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/+/17644/comment/336b7eab_3acd4aff">Patch Set #7, Line 423:</a> <code style="font-family:monospace,monospace"> ao2_bump(obj);</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">If you move the add ref above this can become ao2_ref(obj, +2); […]</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/+/17644/comment/4e4afa8a_15756383">Patch Set #7, Line 426:</a> <code style="font-family:monospace,monospace"> while(ao2_ref(obj, -1) > 1);</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">If this fails you know you have 3 additional refs currently, so ao2_ref(obj, -3) should be sufficien […]</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/c/asterisk/+/17644">change 17644</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/+/17644"/><meta itemprop="name" content="View Change"/></div></div>
<div style="display:none"> Gerrit-Project: asterisk </div>
<div style="display:none"> Gerrit-Branch: 16 </div>
<div style="display:none"> Gerrit-Change-Id: Icfb16b3acbc29cf5b4cef74183f7531caaefe21d </div>
<div style="display:none"> Gerrit-Change-Number: 17644 </div>
<div style="display:none"> Gerrit-PatchSet: 9 </div>
<div style="display:none"> Gerrit-Owner: Michael Bradeen <mbradeen@sangoma.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-Reviewer: Kevin Harwell <kharwell@digium.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-Attention: Kevin Harwell <kharwell@digium.com> </div>
<div style="display:none"> Gerrit-Comment-Date: Fri, 07 Jan 2022 17:26:29 +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: Sean Bright <sean@seanbright.com> </div>
<div style="display:none"> Comment-In-Reply-To: Joshua Colp <jcolp@sangoma.com> </div>
<div style="display:none"> Comment-In-Reply-To: Kevin Harwell <kharwell@digium.com> </div>
<div style="display:none"> Gerrit-MessageType: comment </div>