[Asterisk-code-review] res_geolocation: Initial commit (asterisk[development/16/geolocation])
Kevin Harwell
asteriskteam at digium.com
Mon Feb 21 18:25:32 CST 2022
Attention is currently required from: George Joseph.
Kevin Harwell has posted comments on this change. ( https://gerrit.asterisk.org/c/asterisk/+/18068 )
Change subject: res_geolocation: Initial commit
......................................................................
Patch Set 5: Code-Review-1
(24 comments)
File include/asterisk/res_geolocation.h:
https://gerrit.asterisk.org/c/asterisk/+/18068/comment/e43dcf66_8c52fbe9
PS5, Line 24: enum geoloc_pidf_section {
: PIDF_SECTION_NONE = 0,
: PIDF_SECTION_TUPLE,
: PIDF_SECTION_DEVICE,
: PIDF_SECTION_PERSON
: };
:
: enum geoloc_format {
: GEOLOC_FORMAT_NONE = 0,
: GEOLOC_FORMAT_CIVIC_ADDRESS,
: GEOLOC_FORMAT_GML,
: GEOLOC_FORMAT_URI,
: };
:
: enum geoloc_location_disposition {
: GEOLOC_LOC_DISP_DISCARD = 0,
: GEOLOC_LOC_DISP_APPEND,
: GEOLOC_LOC_DISP_PREPEND,
: GEOLOC_LOC_DISP_REPLACE,
: };
Should these enums start with "ast_" since they're public?
https://gerrit.asterisk.org/c/asterisk/+/18068/comment/3bdce38c_fcdae5e8
PS5, Line 24: enum geoloc_pidf_section {
: PIDF_SECTION_NONE = 0,
: PIDF_SECTION_TUPLE,
: PIDF_SECTION_DEVICE,
: PIDF_SECTION_PERSON
: };
:
: enum geoloc_format {
: GEOLOC_FORMAT_NONE = 0,
: GEOLOC_FORMAT_CIVIC_ADDRESS,
: GEOLOC_FORMAT_GML,
: GEOLOC_FORMAT_URI,
: };
:
: enum geoloc_location_disposition {
: GEOLOC_LOC_DISP_DISCARD = 0,
: GEOLOC_LOC_DISP_APPEND,
: GEOLOC_LOC_DISP_PREPEND,
: GEOLOC_LOC_DISP_REPLACE,
: };
:
: struct ast_geoloc_location {
: SORCERY_OBJECT(details);
: enum geoloc_format format;
: struct ast_variable *location_vars;
: };
:
: struct ast_geoloc_profile {
: SORCERY_OBJECT(details);
: AST_DECLARE_STRING_FIELDS(
: AST_STRING_FIELD(location_reference);
: );
: enum geoloc_pidf_section pidf_section;
: enum geoloc_location_disposition location_disposition;
: int geolocation_routing;
: int send_location;
: struct ast_variable *location_refinement;
: struct ast_variable *location_variables;
: struct ast_variable *usage_rules_vars;
: };
:
: struct ast_geoloc_effective_profile {
: AST_DECLARE_STRING_FIELDS(
: AST_STRING_FIELD(id);
: AST_STRING_FIELD(location_reference);
: );
: enum geoloc_pidf_section pidf_section;
: enum geoloc_location_disposition location_disposition;
: int geolocation_routing;
: int send_location;
: enum geoloc_format format;
: struct ast_variable *location_vars;
: struct ast_variable *location_refinement;
: struct ast_variable *location_variables;
: struct ast_variable *effective_location;
: struct ast_variable *usage_rules_vars;
: };
These enums and structs should be documented too, but if waiting to do that later that's fine.
https://gerrit.asterisk.org/c/asterisk/+/18068/comment/1068ec8a_198e83a8
PS5, Line 110: enum ast_geoloc_validate_result {
: AST_GEOLOC_VALIDATE_SUCCESS = 0,
: AST_GEOLOC_VALIDATE_MISSING_TYPE,
: AST_GEOLOC_VALIDATE_INVALID_TYPE,
: AST_GEOLOC_VALIDATE_INVALID_VARNAME,
: AST_GEOLOC_VALIDATE_NOT_ENOUGH_VARNAMES,
: AST_GEOLOC_VALIDATE_TOO_MANY_VARNAMES,
: AST_GEOLOC_VALIDATE_INVALID_VALUE,
: };
:
: const char *ast_geoloc_validate_result_to_str(enum ast_geoloc_validate_result result);
more missing docs.
https://gerrit.asterisk.org/c/asterisk/+/18068/comment/a0aa70d6_bd140565
PS5, Line 130: struct ast_variable *varlist
make "varlist" const since it's not being modified in the function.
https://gerrit.asterisk.org/c/asterisk/+/18068/comment/8ac0f952_db5316f4
PS5, Line 144: struct ast_datastore *ast_geoloc_datastore_create(const char *profile_name);
:
: struct ast_geoloc_effective_profile *ast_geoloc_effective_profile_create(struct ast_geoloc_profile *profile);
Docs needed for these functions too.
File res/res_geolocation.c:
https://gerrit.asterisk.org/c/asterisk/+/18068/comment/3cdafc01_d49e747a
PS5, Line 46: res += geoloc_civicaddr_reload();
: res += geoloc_gml_reload();
: res += geoloc_config_reload();
: res += geoloc_dialplan_reload();
: res += geoloc_channel_reload();
If any of these fail the module declines. Perhaps "short-circuit" the calls so a subsequent one is not called if the previous failed?
File res/res_geolocation/geoloc_channel.c:
https://gerrit.asterisk.org/c/asterisk/+/18068/comment/1e4a42d1_740c57df
PS5, Line 45: if (!profile) {
: ast_log(LOG_ERROR, "A profile with the name '%s' was not found", profile_name);
: }
Should this path free "ds" and return NULL here?
https://gerrit.asterisk.org/c/asterisk/+/18068/comment/c702a2ef_c8c7205d
PS5, Line 49: eprofile = ast_geoloc_effective_profile_create(profile);
As the code is now "profile" can be passed as NULL, which will cause things to crash inside ast_geoloc_effective_profile_create.
https://gerrit.asterisk.org/c/asterisk/+/18068/comment/6a0dac25_de5c8e9b
PS5, Line 50: ds->data = eprofile;
What if "eprofile" is NULL? Should processing discontinue here or be allowed to continue with a NULL eprofile as it's currently doing?
File res/res_geolocation/geoloc_config.c:
https://gerrit.asterisk.org/c/asterisk/+/18068/comment/f9497643_a06958e7
PS5, Line 59: ast_sorcery_generic_alloc
ast_sorcery_generic_alloc can return NULL so location needs a NULL check.
https://gerrit.asterisk.org/c/asterisk/+/18068/comment/c2654e4d_4a8b245f
PS5, Line 82: struct ast_geoloc_profile *profile = ast_sorcery_generic_alloc(sizeof(*profile), geoloc_profile_destructor);
Same here, it's possible for ast_sorcery_generic_alloc to return NULL so "profile" needs to be checked before continuing.
https://gerrit.asterisk.org/c/asterisk/+/18068/comment/03fb74be_a8445cd0
PS5, Line 113: const char *profile_id = ast_sorcery_object_get_id(profile);
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.
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.
https://gerrit.asterisk.org/c/asterisk/+/18068/comment/390aee15_8e11e787
PS5, Line 116: eprofile = geoloc_effective_profile_alloc(profile_id);
Do a NULL check on "eprofile" after allocation attempt.
https://gerrit.asterisk.org/c/asterisk/+/18068/comment/076cb14c_316004da
PS5, Line 163: ast_log(LOG_ERROR, "Location '%s' must have a format\n", location_id);
: return -1;
These lines can move back a tab.
https://gerrit.asterisk.org/c/asterisk/+/18068/comment/c7542b52_2bbfc98d
PS5, Line 165: break;
This line can be removed.
https://gerrit.asterisk.org/c/asterisk/+/18068/comment/9cf462ab_dfcb581e
PS5, Line 233: return -1;
"location" is ref leaked here.
https://gerrit.asterisk.org/c/asterisk/+/18068/comment/a0f8bde7_1acb349d
PS5, Line 243: return 0;
"location" is ref leaked here.
https://gerrit.asterisk.org/c/asterisk/+/18068/comment/01a244ae_ea5f0a43
PS5, Line 586: if (geoloc_sorcery) {
: ast_sorcery_unref(geoloc_sorcery);
: }
: geoloc_sorcery = NULL;
I don't think is an actual problem (at least highly unlikely), but will mention just in case.
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.
File res/res_geolocation/geoloc_doc.xml:
https://gerrit.asterisk.org/c/asterisk/+/18068/comment/b6b52ebd_d6257506
PS5, Line 10: cffdffff
Not sure what this is?
https://gerrit.asterisk.org/c/asterisk/+/18068/comment/a744260d_7c10d394
PS5, Line 51: </configObject>
extra space at eol.
https://gerrit.asterisk.org/c/asterisk/+/18068/comment/6b104a97_1c92b929
PS5, Line 55: cffdffff
?
https://gerrit.asterisk.org/c/asterisk/+/18068/comment/16fbb7e2_d0ae521a
PS5, Line 89: xxxx
Just acting as a placeholder for now?
File res/res_geolocation/geoloc_gml.c:
https://gerrit.asterisk.org/c/asterisk/+/18068/comment/041ebb2e_9b5ea6e8
PS5, Line 24: #if 1 //not used yet.
If not used yet should this be "#if 0", or if now referenced the "#if" can be removed?
File res/res_geolocation/geoloc_private.h:
https://gerrit.asterisk.org/c/asterisk/+/18068/comment/595c585f_b4f228b5
PS5, Line 80: new_var = ast_variable_new(item_name, item_value, ""); \
This needs a NULL check.
--
To view, visit https://gerrit.asterisk.org/c/asterisk/+/18068
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: Ieb6e3640f31a676da42d8c144ebbb31ad795d849
Gerrit-Change-Number: 18068
Gerrit-PatchSet: 5
Gerrit-Owner: George Joseph <gjoseph at digium.com>
Gerrit-Reviewer: Friendly Automation
Gerrit-Reviewer: Kevin Harwell <kharwell at digium.com>
Gerrit-Attention: George Joseph <gjoseph at digium.com>
Gerrit-Comment-Date: Tue, 22 Feb 2022 00:25:32 +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/20220221/c9ed929f/attachment-0001.html>
More information about the asterisk-code-review
mailing list