<p> Attention is currently required from: Michael Bradeen. </p>
<p>Patch set 2:<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/+/17644">View Change</a></p><p>13 comments:</p><ul style="list-style: none; padding: 0;"><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/d50d0bbe_9c4cb9a4">Patch Set #2, Line 296:</a> <code style="font-family:monospace,monospace"> * \retval -2 event was running and not rescheduled</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">I don't think changing this API call is a good idea, because other users of it may not be written to handle the return value. I think a separate API call should exist which returns this more descriptive information for use by the AST_SCHED_DEL_UNREF macro and perhaps other things, and ast_sched_del should instead just return -1 for this case.</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/66d6d011_22993ecf">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 style="white-space: pre-wrap; word-wrap: break-word;">This is no longer accurate.</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/68ede7f2_a56cd081">Patch Set #2, Line 658:</a> <code style="font-family:monospace,monospace">                     sched_release(con, s);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Add a comment that ast_sched_runq passes responsibility for release to us in this case.</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/3a7715bc_e1982a12">Patch Set #2, Line 807:</a> <code style="font-family:monospace,monospace">                        current->rescheduled = res ? 1 : 0;</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Add a comment explaining what has happened here (that another thread is waiting on this scheduled item, and thus will be responsible for destruction)</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/1b7ec7ed_91b9bf57">Patch Set #2, Line 366:</a> <code style="font-family:monospace,monospace">   usleep(3000);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Add a comment explaining why this sleep exists</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/ae96ca32_d42aea7c">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 style="white-space: pre-wrap; word-wrap: break-word;">This is actually just testing the scheduler</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/4f176e44_7c16c9fc">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 style="white-space: pre-wrap; word-wrap: break-word;">We generally put the obj = ao2_alloc on a separate line from the if.</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/fd521e8c_09f06bb6">Patch Set #2, Line 406:</a> <code style="font-family:monospace,monospace">             return AST_TEST_FAIL;</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">obj is leaked</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/806321e3_e358bc82">Patch Set #2, Line 409:</a> <code style="font-family:monospace,monospace">   if(ast_sched_start_thread(con)) {</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Space between if and (</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/69c7175a_4b5b0048">Patch Set #2, Line 412:</a> <code style="font-family:monospace,monospace">              return AST_TEST_FAIL;</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">obj is leaked</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/612a4d25_6954b9f8">Patch Set #2, Line 419:</a> <code style="font-family:monospace,monospace">           return AST_TEST_FAIL;</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">obj is leaked</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/02543e9c_39046ae0">Patch Set #2, Line 423:</a> <code style="font-family:monospace,monospace">   while(obj->scheduledCBstarted == 0) {</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Add a comment explaining what this is waiting on and why</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/8d57113d_42f64756">Patch Set #2, Line 444:</a> <code style="font-family:monospace,monospace"></code></p><p style="white-space: pre-wrap; word-wrap: break-word;">obj is leaked - the easiest way to destroy it is to probably loop until ao2_ref(obj, -1) returns 0.</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: 2 </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-Attention: Michael Bradeen <mbradeen@sangoma.com> </div>
<div style="display:none"> Gerrit-Comment-Date: Tue, 14 Dec 2021 10:31:04 +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>