<p> Attention is currently required from: George Joseph. </p>
<p>Patch set 5:<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/+/18068">View Change</a></p><p>24 comments:</p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0;"><p><a href="null">File include/asterisk/res_geolocation.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/+/18068/comment/e43dcf66_8c52fbe9">Patch Set #5, Line 24:</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;">enum geoloc_pidf_section {<br> PIDF_SECTION_NONE = 0,<br> PIDF_SECTION_TUPLE,<br> PIDF_SECTION_DEVICE,<br> PIDF_SECTION_PERSON<br>};<br><br>enum geoloc_format {<br> GEOLOC_FORMAT_NONE = 0,<br> GEOLOC_FORMAT_CIVIC_ADDRESS,<br> GEOLOC_FORMAT_GML,<br> GEOLOC_FORMAT_URI,<br>};<br><br>enum geoloc_location_disposition {<br> GEOLOC_LOC_DISP_DISCARD = 0,<br> GEOLOC_LOC_DISP_APPEND,<br> GEOLOC_LOC_DISP_PREPEND,<br> GEOLOC_LOC_DISP_REPLACE,<br>};<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Should these enums start with "ast_" since they're public?</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/18068/comment/3bdce38c_fcdae5e8">Patch Set #5, Line 24:</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;">enum geoloc_pidf_section {<br> PIDF_SECTION_NONE = 0,<br> PIDF_SECTION_TUPLE,<br> PIDF_SECTION_DEVICE,<br> PIDF_SECTION_PERSON<br>};<br><br>enum geoloc_format {<br> GEOLOC_FORMAT_NONE = 0,<br> GEOLOC_FORMAT_CIVIC_ADDRESS,<br> GEOLOC_FORMAT_GML,<br> GEOLOC_FORMAT_URI,<br>};<br><br>enum geoloc_location_disposition {<br> GEOLOC_LOC_DISP_DISCARD = 0,<br> GEOLOC_LOC_DISP_APPEND,<br> GEOLOC_LOC_DISP_PREPEND,<br> GEOLOC_LOC_DISP_REPLACE,<br>};<br><br>struct ast_geoloc_location {<br> SORCERY_OBJECT(details);<br> enum geoloc_format format;<br> struct ast_variable *location_vars;<br>};<br><br>struct ast_geoloc_profile {<br> SORCERY_OBJECT(details);<br> AST_DECLARE_STRING_FIELDS(<br> AST_STRING_FIELD(location_reference);<br> );<br> enum geoloc_pidf_section pidf_section;<br> enum geoloc_location_disposition location_disposition;<br> int geolocation_routing;<br> int send_location;<br> struct ast_variable *location_refinement;<br> struct ast_variable *location_variables;<br> struct ast_variable *usage_rules_vars;<br>};<br><br>struct ast_geoloc_effective_profile {<br> AST_DECLARE_STRING_FIELDS(<br> AST_STRING_FIELD(id);<br> AST_STRING_FIELD(location_reference);<br> );<br> enum geoloc_pidf_section pidf_section;<br> enum geoloc_location_disposition location_disposition;<br> int geolocation_routing;<br> int send_location;<br> enum geoloc_format format;<br> struct ast_variable *location_vars;<br> struct ast_variable *location_refinement;<br> struct ast_variable *location_variables;<br> struct ast_variable *effective_location;<br> struct ast_variable *usage_rules_vars;<br>};<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">These enums and structs should be documented too, but if waiting to do that later that's fine.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/18068/comment/1068ec8a_198e83a8">Patch Set #5, Line 110:</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;">enum ast_geoloc_validate_result {<br> AST_GEOLOC_VALIDATE_SUCCESS = 0,<br> AST_GEOLOC_VALIDATE_MISSING_TYPE,<br> AST_GEOLOC_VALIDATE_INVALID_TYPE,<br> AST_GEOLOC_VALIDATE_INVALID_VARNAME,<br> AST_GEOLOC_VALIDATE_NOT_ENOUGH_VARNAMES,<br> AST_GEOLOC_VALIDATE_TOO_MANY_VARNAMES,<br> AST_GEOLOC_VALIDATE_INVALID_VALUE,<br>};<br><br>const char *ast_geoloc_validate_result_to_str(enum ast_geoloc_validate_result result);<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">more missing docs.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/18068/comment/a0aa70d6_bd140565">Patch Set #5, Line 130:</a> <code style="font-family:monospace,monospace">struct ast_variable *varlist</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">make "varlist" const since it's not being modified in the function.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/18068/comment/8ac0f952_db5316f4">Patch Set #5, Line 144:</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;">struct ast_datastore *ast_geoloc_datastore_create(const char *profile_name);<br><br>struct ast_geoloc_effective_profile *ast_geoloc_effective_profile_create(struct ast_geoloc_profile *profile);<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Docs needed for these functions too.</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="null">File res/res_geolocation.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/+/18068/comment/3cdafc01_d49e747a">Patch Set #5, Line 46:</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;"> res += geoloc_civicaddr_reload();<br> res += geoloc_gml_reload();<br> res += geoloc_config_reload();<br> res += geoloc_dialplan_reload();<br> res += geoloc_channel_reload();<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">If any of these fail the module declines. Perhaps "short-circuit" the calls so a subsequent one is not called if the previous failed?</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="null">File res/res_geolocation/geoloc_channel.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/+/18068/comment/1e4a42d1_740c57df">Patch Set #5, Line 45:</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 (!profile) {<br> ast_log(LOG_ERROR, "A profile with the name '%s' was not found", profile_name);<br> }<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Should this path free "ds" and return NULL here?</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/18068/comment/c702a2ef_c8c7205d">Patch Set #5, Line 49:</a> <code style="font-family:monospace,monospace"> eprofile = ast_geoloc_effective_profile_create(profile);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">As the code is now "profile" can be passed as NULL, which will cause things to crash inside ast_geoloc_effective_profile_create.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/18068/comment/6a0dac25_de5c8e9b">Patch Set #5, Line 50:</a> <code style="font-family:monospace,monospace"> ds->data = eprofile;</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">What if "eprofile" is NULL? Should processing discontinue here or be allowed to continue with a NULL eprofile as it's currently doing?</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="null">File res/res_geolocation/geoloc_config.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/+/18068/comment/f9497643_a06958e7">Patch Set #5, Line 59:</a> <code style="font-family:monospace,monospace">ast_sorcery_generic_alloc</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">ast_sorcery_generic_alloc can return NULL so location needs a NULL check.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/18068/comment/c2654e4d_4a8b245f">Patch Set #5, Line 82:</a> <code style="font-family:monospace,monospace"> struct ast_geoloc_profile *profile = ast_sorcery_generic_alloc(sizeof(*profile), geoloc_profile_destructor);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Same here, it's possible for ast_sorcery_generic_alloc to return NULL so "profile" needs to be checked before continuing.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/18068/comment/03fb74be_a8445cd0">Patch Set #5, Line 113:</a> <code style="font-family:monospace,monospace"> const char *profile_id = ast_sorcery_object_get_id(profile);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">So far there's at least one place profile can be passed in as NULL, so it needs a NULL check before accessing or things become crashy.</p><p style="white-space: pre-wrap; word-wrap: break-word;">Or perhaps since in the one place I've seen you're getting profile by id, and then here getting the id by profile perhaps pass in the id here instead, and retrieve the profile and check for NULL? That'd also remove one sorcery call.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/18068/comment/390aee15_8e11e787">Patch Set #5, Line 116:</a> <code style="font-family:monospace,monospace"> eprofile = geoloc_effective_profile_alloc(profile_id);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Do a NULL check on "eprofile" after allocation attempt.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/18068/comment/076cb14c_316004da">Patch Set #5, Line 163:</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;"> ast_log(LOG_ERROR, "Location '%s' must have a format\n", location_id);<br> return -1;<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">These lines can move back a tab.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/18068/comment/c7542b52_2bbfc98d">Patch Set #5, Line 165:</a> <code style="font-family:monospace,monospace"> break;</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">This line can be 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/+/18068/comment/9cf462ab_dfcb581e">Patch Set #5, Line 233:</a> <code style="font-family:monospace,monospace"> return -1;</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">"location" is ref leaked here.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/18068/comment/a0f8bde7_1acb349d">Patch Set #5, Line 243:</a> <code style="font-family:monospace,monospace"> return 0;</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">"location" is ref leaked here.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/18068/comment/01a244ae_ea5f0a43">Patch Set #5, Line 586:</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 (geoloc_sorcery) {<br> ast_sorcery_unref(geoloc_sorcery);<br> }<br> geoloc_sorcery = NULL;<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">I don't think is an actual problem (at least highly unlikely), but will mention just in case.</p><p style="white-space: pre-wrap; word-wrap: break-word;">Without locking on geoloc_sorcery, if multiple threads are in play it may be possible for something to call "geoloc_get_sorcery" between the unref and NULL here. If so then "geoloc_get_sorcery" potentially try to ref an invalid pointer.</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="null">File res/res_geolocation/geoloc_doc.xml:</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/+/18068/comment/b6b52ebd_d6257506">Patch Set #5, Line 10:</a> <code style="font-family:monospace,monospace">cffdffff</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Not sure what this is?</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/18068/comment/a744260d_7c10d394">Patch Set #5, Line 51:</a> <code style="font-family:monospace,monospace"> </configObject> </code></p><p style="white-space: pre-wrap; word-wrap: break-word;">extra space at eol.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/18068/comment/6b104a97_1c92b929">Patch Set #5, Line 55:</a> <code style="font-family:monospace,monospace">cffdffff</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">?</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/18068/comment/16fbb7e2_d0ae521a">Patch Set #5, Line 89:</a> <code style="font-family:monospace,monospace">xxxx</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Just acting as a placeholder for now?</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="null">File res/res_geolocation/geoloc_gml.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/+/18068/comment/041ebb2e_9b5ea6e8">Patch Set #5, Line 24:</a> <code style="font-family:monospace,monospace">#if 1 //not used yet.</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">If not used yet should this be "#if 0", or if now referenced the "#if" can be removed?</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="null">File res/res_geolocation/geoloc_private.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/+/18068/comment/595c585f_b4f228b5">Patch Set #5, Line 80:</a> <code style="font-family:monospace,monospace"> new_var = ast_variable_new(item_name, item_value, ""); \</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">This needs a NULL check.</p></li></ul></li></ul><p>To view, visit <a href="https://gerrit.asterisk.org/c/asterisk/+/18068">change 18068</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/+/18068"/><meta itemprop="name" content="View Change"/></div></div>
<div style="display:none"> Gerrit-Project: asterisk </div>
<div style="display:none"> Gerrit-Branch: development/16/geolocation </div>
<div style="display:none"> Gerrit-Change-Id: Ieb6e3640f31a676da42d8c144ebbb31ad795d849 </div>
<div style="display:none"> Gerrit-Change-Number: 18068 </div>
<div style="display:none"> Gerrit-PatchSet: 5 </div>
<div style="display:none"> Gerrit-Owner: George Joseph <gjoseph@digium.com> </div>
<div style="display:none"> Gerrit-Reviewer: Friendly Automation </div>
<div style="display:none"> Gerrit-Reviewer: Kevin Harwell <kharwell@digium.com> </div>
<div style="display:none"> Gerrit-Attention: George Joseph <gjoseph@digium.com> </div>
<div style="display:none"> Gerrit-Comment-Date: Tue, 22 Feb 2022 00:25:32 +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>