<p>Patch set 1:<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/+/11217">View Change</a></p><p>11 comments:</p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.asterisk.org/#/c/11217/1/main/loader.c">File main/loader.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/11217/1/main/loader.c@162">Patch Set #1, Line 162:</a> <code style="font-family:monospace,monospace"> const char *name;</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">You could save an allocation by putting `char name[0];` last and including space for the string in the `info_list_obj` allocation. Then you wouldn't need `info_list_obj_dtor`.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/11217/1/main/loader.c@179">Patch Set #1, Line 179:</a> <code style="font-family:monospace,monospace"> new_entry = ao2_alloc(sizeof(*new_entry), info_list_obj_dtor);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Need to check for allocation failure.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/11217/1/main/loader.c@180">Patch Set #1, Line 180:</a> <code style="font-family:monospace,monospace"> new_entry->name = ast_strdup(name);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">If you do not take the `char name[0]` advice then you will need to verify that name was successfully allocated.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/11217/1/main/loader.c@285">Patch Set #1, Line 285:</a> <code style="font-family:monospace,monospace">#if defined(HAVE_PERMANENT_DLOPEN)</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Can this be merged into the first `#if defined(HAVE_PERMANENT_DLOPEN)`?</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/11217/1/main/loader.c@286">Patch Set #1, Line 286:</a> <code style="font-family:monospace,monospace">static int info_list_obj_cmp_fn(void *obj1, void *obj2, int flags)</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Please use `AO2_STRING_FIELD_CMP_FN(info_list_obj, name)` to construct this callback function.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/11217/1/main/loader.c@340">Patch Set #1, Line 340:</a> <code style="font-family:monospace,monospace"> if (obj_tmp) {</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">You need to `ao2_ref(obj_tmp, -1)` otherwise you leak a reference.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/11217/1/main/loader.c@366">Patch Set #1, Line 366:</a> <code style="font-family:monospace,monospace"> ast_module_unregister(obj_tmp->info);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">ao2_ref(obj_tmp, -1); after this line.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/11217/1/main/loader.c@722">Patch Set #1, Line 722:</a> <code style="font-family:monospace,monospace"> if (!info_list) {</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">This needs to be allocated near the start of load_modules, prevent startup if the allocation fails (by returning -1 from load_modules).</p><p style="white-space: pre-wrap; word-wrap: break-word;">This container will leak at shutdown, I think that's unavoidable so please comment near the allocation must not be cleaned at shutdown.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/11217/1/main/loader.c@728">Patch Set #1, Line 728:</a> <code style="font-family:monospace,monospace"> struct info_list_obj *obj_tmp = ao2_find(info_list, info->name,</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Need to clean reference to obj_tmp.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/11217/1/main/loader.c@732">Patch Set #1, Line 732:</a> <code style="font-family:monospace,monospace"> ao2_link(info_list, info_list_obj_alloc(info->name, info));</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Need to check for allocation failure from `info_list_obj_alloc`.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/11217/1/main/loader.c@736">Patch Set #1, Line 736:</a> <code style="font-family:monospace,monospace"> if (!info_list) {</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Please just delete this. Returning -1 from load_modules will be adequate / the right way to handle this situation. We don't want to use `exit` directly from here as it skips running mandatory shutdown functions.</p></li></ul></li></ul><p>To view, visit <a href="https://gerrit.asterisk.org/c/asterisk/+/11217">change 11217</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/+/11217"/><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: I86693a0ecf25d5ba81c73773a03df4abc3426875 </div>
<div style="display:none"> Gerrit-Change-Number: 11217 </div>
<div style="display:none"> Gerrit-PatchSet: 1 </div>
<div style="display:none"> Gerrit-Owner: Sebastian Kemper <sebastian_ml@gmx.net> </div>
<div style="display:none"> Gerrit-Reviewer: Corey Farrell <git@cfware.com> </div>
<div style="display:none"> Gerrit-Reviewer: Friendly Automation </div>
<div style="display:none"> Gerrit-Comment-Date: Wed, 03 Apr 2019 13:13:11 +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>