<p> Attention is currently required from: George Joseph. </p>
<p>Patch set 3:<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/+/18191">View Change</a></p><p>4 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/+/18191/comment/30d37b06_b627e59f">Patch Set #3, Line 94:</a> <code style="font-family:monospace,monospace">AST_OPTIONAL_API(int, ast_geoloc_is_loaded, (void), { return 0; });</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">I think if you wanted here you could call "ast_module_check" to see if the module is loaded vs having a custom function for it.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/18191/comment/f8cca13e_3cc83d71">Patch Set #3, Line 199:</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;">int ast_geoloc_datastore_add_eprofile(struct ast_datastore *ds,<br> struct ast_geoloc_eprofile *eprofile);<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">This currently steals the eprofile reference. If it's going to remain as such I think this needs a comment stating that the reference is being stolen.</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="null">File res/res_geolocation/geoloc_datastore.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/+/18191/comment/872d0257_c4bee856">Patch Set #3, Line 96:</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;"> rc = AST_VECTOR_APPEND(&eds->eprofiles, eprofile);<br> if (rc != 0) {<br> ast_log(LOG_ERROR, "Couldn't add eprofile '%s' to geoloc datastore '%s'\n", eprofile->id, eds->id);<br> }<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">I'm personally not a fan of public API calls stealing refs, and unsure what our convention is if we even have one.</p><p style="white-space: pre-wrap; word-wrap: break-word;">As such since the datastore is going to hold a reference I think you need to bump the reference on eprofile here (actually might want to do it after checking if it was added to save an unref if it wasn't) vs stealing it.</p><p style="white-space: pre-wrap; word-wrap: break-word;">Or maybe steal it from internal calls but not from external? I dunno maybe I'm overthinking it.</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="null">File res/res_pjsip_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/+/18191/comment/5a68f0ac_7c665265">Patch Set #3, Line 148:</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 (config_profile->location_disposition == AST_GEOLOC_LOC_DISP_DISCARD) {<br> ast_trace(4, "%s: Profile '%s' location_disposition is 'discard' so "<br> "discarding Geolocation: " PJSTR_PRINTF_SPEC, session_name,<br> ast_sorcery_object_get_id(config_profile),<br> PJSTR_PRINTF_VAR(geoloc_hdr->hvalue));<br><br> config_eprofile = ast_geoloc_eprofile_create_from_profile(config_profile);<br> if (!config_eprofile) {<br> SCOPE_EXIT_LOG_RTN_VALUE(0, LOG_WARNING, "%s: Unable to create eprofile from "<br> "profile '%s'\n", session_name, ast_sorcery_object_get_id(config_profile));<br> }<br><br> rc = ast_geoloc_datastore_add_eprofile(ds, config_eprofile);<br> if (rc != 0) {<br> SCOPE_EXIT_LOG_RTN_VALUE(0, LOG_WARNING,<br> "%s: Couldn't add eprofile '%s' to datastore\n", session_name,<br> config_eprofile->id);<br> }<br><br> ast_channel_datastore_add(channel, ds);<br> /*<br> * We gave our eprofile reference to the datastore and the<br> * datastore to the channel so don't let RAII_VAR clean them up.<br> */<br> config_eprofile = NULL;<br> ds = NULL;<br><br> SCOPE_EXIT_RTN_VALUE(0, "%s: Added geoloc datastore with 1 eprofile\n",<br> session_name);<br> } else if (config_profile->location_disposition == AST_GEOLOC_LOC_DISP_PREPEND) {<br> ast_trace(4, "%s: Profile '%s' location_disposition is 'prepend' so "<br> "adding to datastore first", session_name, ast_sorcery_object_get_id(config_profile));<br><br> config_eprofile = ast_geoloc_eprofile_create_from_profile(config_profile);<br> if (!config_eprofile) {<br> SCOPE_EXIT_LOG_RTN_VALUE(0, LOG_WARNING, "%s: Unable to create eprofile from"<br> " profile '%s'\n", session_name, ast_sorcery_object_get_id(config_profile));<br> }<br><br> rc = ast_geoloc_datastore_add_eprofile(ds, config_eprofile);<br> if (rc != 0) {<br> SCOPE_EXIT_LOG_RTN_VALUE(0, LOG_WARNING,<br> "%s: Couldn't add eprofile '%s' to datastore\n", session_name,<br> config_eprofile->id);<br> }<br> config_eprofile = NULL;<br><br> if (!geoloc_hdr) {<br> ast_channel_datastore_add(channel, ds);<br> ds = NULL;<br><br> SCOPE_EXIT_RTN_VALUE(0, "%s: No Geolocation header so just adding config profile "<br> "'%s' to datastore\n", session_name, ast_sorcery_object_get_id(config_profile));<br> }<br> } else if (config_profile->location_disposition == AST_GEOLOC_LOC_DISP_REPLACE) {<br> if (geoloc_hdr) {<br> ast_trace(4, "%s: Profile '%s' location_disposition is 'replace' so "<br> "we don't need to do anything with the configured profile", session_name,<br> ast_sorcery_object_get_id(config_profile));<br> } else {<br> SCOPE_EXIT_LOG_RTN_VALUE(0, LOG_WARNING,<br> "%s: Profile '%s' location_disposition is 'replace' but there's"<br> "no Geolocation header and therefore no location info to replace"<br> "it with\n", session_name, ast_sorcery_object_get_id(config_profile));<br> }<br> }<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Aside from some logging params I wonder if this could all be moved internally into geoloc module or would that introduce some outside dependency?</p><p style="white-space: pre-wrap; word-wrap: break-word;">Have it just pass in the call profile and return a datastore, which can then be added to the channel based on geoloc_hdr.</p></li></ul></li></ul><p>To view, visit <a href="https://gerrit.asterisk.org/c/asterisk/+/18191">change 18191</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/+/18191"/><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: I6ccb5108a5eec060efb8116502fbff3cc63d7554 </div>
<div style="display:none"> Gerrit-Change-Number: 18191 </div>
<div style="display:none"> Gerrit-PatchSet: 3 </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: Joshua Colp <jcolp@sangoma.com> </div>
<div style="display:none"> Gerrit-Reviewer: Kevin Harwell <kharwell@digium.com> </div>
<div style="display:none"> Gerrit-Reviewer: Michael Bradeen <mbradeen@sangoma.com> </div>
<div style="display:none"> Gerrit-Attention: George Joseph <gjoseph@digium.com> </div>
<div style="display:none"> Gerrit-Comment-Date: Thu, 10 Mar 2022 19:40:16 +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>