[Asterisk-code-review] res_pjsip_geolocation: Initial commit (asterisk[development/16/geolocation])
Kevin Harwell
asteriskteam at digium.com
Thu Mar 10 13:40:16 CST 2022
Attention is currently required from: George Joseph.
Kevin Harwell has posted comments on this change. ( https://gerrit.asterisk.org/c/asterisk/+/18191 )
Change subject: res_pjsip_geolocation: Initial commit
......................................................................
Patch Set 3: Code-Review-1
(4 comments)
File include/asterisk/res_geolocation.h:
https://gerrit.asterisk.org/c/asterisk/+/18191/comment/30d37b06_b627e59f
PS3, Line 94: AST_OPTIONAL_API(int, ast_geoloc_is_loaded, (void), { return 0; });
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.
https://gerrit.asterisk.org/c/asterisk/+/18191/comment/f8cca13e_3cc83d71
PS3, Line 199: int ast_geoloc_datastore_add_eprofile(struct ast_datastore *ds,
: struct ast_geoloc_eprofile *eprofile);
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.
File res/res_geolocation/geoloc_datastore.c:
https://gerrit.asterisk.org/c/asterisk/+/18191/comment/872d0257_c4bee856
PS3, Line 96: rc = AST_VECTOR_APPEND(&eds->eprofiles, eprofile);
: if (rc != 0) {
: ast_log(LOG_ERROR, "Couldn't add eprofile '%s' to geoloc datastore '%s'\n", eprofile->id, eds->id);
: }
I'm personally not a fan of public API calls stealing refs, and unsure what our convention is if we even have one.
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.
Or maybe steal it from internal calls but not from external? I dunno maybe I'm overthinking it.
File res/res_pjsip_geolocation.c:
https://gerrit.asterisk.org/c/asterisk/+/18191/comment/5a68f0ac_7c665265
PS3, Line 148: if (config_profile->location_disposition == AST_GEOLOC_LOC_DISP_DISCARD) {
: ast_trace(4, "%s: Profile '%s' location_disposition is 'discard' so "
: "discarding Geolocation: " PJSTR_PRINTF_SPEC, session_name,
: ast_sorcery_object_get_id(config_profile),
: PJSTR_PRINTF_VAR(geoloc_hdr->hvalue));
:
: config_eprofile = ast_geoloc_eprofile_create_from_profile(config_profile);
: if (!config_eprofile) {
: SCOPE_EXIT_LOG_RTN_VALUE(0, LOG_WARNING, "%s: Unable to create eprofile from "
: "profile '%s'\n", session_name, ast_sorcery_object_get_id(config_profile));
: }
:
: rc = ast_geoloc_datastore_add_eprofile(ds, config_eprofile);
: if (rc != 0) {
: SCOPE_EXIT_LOG_RTN_VALUE(0, LOG_WARNING,
: "%s: Couldn't add eprofile '%s' to datastore\n", session_name,
: config_eprofile->id);
: }
:
: ast_channel_datastore_add(channel, ds);
: /*
: * We gave our eprofile reference to the datastore and the
: * datastore to the channel so don't let RAII_VAR clean them up.
: */
: config_eprofile = NULL;
: ds = NULL;
:
: SCOPE_EXIT_RTN_VALUE(0, "%s: Added geoloc datastore with 1 eprofile\n",
: session_name);
: } else if (config_profile->location_disposition == AST_GEOLOC_LOC_DISP_PREPEND) {
: ast_trace(4, "%s: Profile '%s' location_disposition is 'prepend' so "
: "adding to datastore first", session_name, ast_sorcery_object_get_id(config_profile));
:
: config_eprofile = ast_geoloc_eprofile_create_from_profile(config_profile);
: if (!config_eprofile) {
: SCOPE_EXIT_LOG_RTN_VALUE(0, LOG_WARNING, "%s: Unable to create eprofile from"
: " profile '%s'\n", session_name, ast_sorcery_object_get_id(config_profile));
: }
:
: rc = ast_geoloc_datastore_add_eprofile(ds, config_eprofile);
: if (rc != 0) {
: SCOPE_EXIT_LOG_RTN_VALUE(0, LOG_WARNING,
: "%s: Couldn't add eprofile '%s' to datastore\n", session_name,
: config_eprofile->id);
: }
: config_eprofile = NULL;
:
: if (!geoloc_hdr) {
: ast_channel_datastore_add(channel, ds);
: ds = NULL;
:
: SCOPE_EXIT_RTN_VALUE(0, "%s: No Geolocation header so just adding config profile "
: "'%s' to datastore\n", session_name, ast_sorcery_object_get_id(config_profile));
: }
: } else if (config_profile->location_disposition == AST_GEOLOC_LOC_DISP_REPLACE) {
: if (geoloc_hdr) {
: ast_trace(4, "%s: Profile '%s' location_disposition is 'replace' so "
: "we don't need to do anything with the configured profile", session_name,
: ast_sorcery_object_get_id(config_profile));
: } else {
: SCOPE_EXIT_LOG_RTN_VALUE(0, LOG_WARNING,
: "%s: Profile '%s' location_disposition is 'replace' but there's"
: "no Geolocation header and therefore no location info to replace"
: "it with\n", session_name, ast_sorcery_object_get_id(config_profile));
: }
: }
Aside from some logging params I wonder if this could all be moved internally into geoloc module or would that introduce some outside dependency?
Have it just pass in the call profile and return a datastore, which can then be added to the channel based on geoloc_hdr.
--
To view, visit https://gerrit.asterisk.org/c/asterisk/+/18191
To unsubscribe, or for help writing mail filters, visit https://gerrit.asterisk.org/settings
Gerrit-Project: asterisk
Gerrit-Branch: development/16/geolocation
Gerrit-Change-Id: I6ccb5108a5eec060efb8116502fbff3cc63d7554
Gerrit-Change-Number: 18191
Gerrit-PatchSet: 3
Gerrit-Owner: George Joseph <gjoseph at digium.com>
Gerrit-Reviewer: Friendly Automation
Gerrit-Reviewer: Joshua Colp <jcolp at sangoma.com>
Gerrit-Reviewer: Kevin Harwell <kharwell at digium.com>
Gerrit-Reviewer: Michael Bradeen <mbradeen at sangoma.com>
Gerrit-Attention: George Joseph <gjoseph at digium.com>
Gerrit-Comment-Date: Thu, 10 Mar 2022 19:40:16 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20220310/52afab68/attachment-0001.html>
More information about the asterisk-code-review
mailing list