[Asterisk-code-review] res_geolocation: Initial commit (asterisk[development/16/geolocation])
George Joseph
asteriskteam at digium.com
Tue Feb 22 13:49:56 CST 2022
Attention is currently required from: Kevin Harwell.
George Joseph has posted comments on this change. ( https://gerrit.asterisk.org/c/asterisk/+/18068 )
Change subject: res_geolocation: Initial commit
......................................................................
Patch Set 8:
(24 comments)
File include/asterisk/res_geolocation.h:
https://gerrit.asterisk.org/c/asterisk/+/18068/comment/24b072ad_041a8c56
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?
Done
https://gerrit.asterisk.org/c/asterisk/+/18068/comment/783cb950_b51dec5c
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.
Done
https://gerrit.asterisk.org/c/asterisk/+/18068/comment/ada8f2c5_b092c35f
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.
Done
https://gerrit.asterisk.org/c/asterisk/+/18068/comment/93e3b9ba_b92ea095
PS5, Line 130: struct ast_variable *varlist
> make "varlist" const since it's not being modified in the function.
Done
https://gerrit.asterisk.org/c/asterisk/+/18068/comment/6c34ca81_bb8b79f2
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.
Done
File res/res_geolocation.c:
https://gerrit.asterisk.org/c/asterisk/+/18068/comment/e497a6e3_08047722
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. […]
Done
File res/res_geolocation/geoloc_channel.c:
https://gerrit.asterisk.org/c/asterisk/+/18068/comment/e7e9dd96_d17d59b2
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?
Done
https://gerrit.asterisk.org/c/asterisk/+/18068/comment/1dd9aedc_94b457eb
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_geol […]
Done
https://gerrit.asterisk.org/c/asterisk/+/18068/comment/53f4077b_f541ae99
PS5, Line 50: ds->data = eprofile;
> What if "eprofile" is NULL? Should processing discontinue here or be allowed to continue with a NULL […]
Done
File res/res_geolocation/geoloc_config.c:
https://gerrit.asterisk.org/c/asterisk/+/18068/comment/4c81eb3d_29442004
PS5, Line 59: ast_sorcery_generic_alloc
> ast_sorcery_generic_alloc can return NULL so location needs a NULL check.
Done
https://gerrit.asterisk.org/c/asterisk/+/18068/comment/fb2c9ded_70a5dfab
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 check […]
Done
https://gerrit.asterisk.org/c/asterisk/+/18068/comment/0c8a3e0b_1f61a365
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 […]
Done
https://gerrit.asterisk.org/c/asterisk/+/18068/comment/93dcb112_5bb029f6
PS5, Line 116: eprofile = geoloc_effective_profile_alloc(profile_id);
> Do a NULL check on "eprofile" after allocation attempt.
Done
https://gerrit.asterisk.org/c/asterisk/+/18068/comment/91e01d6f_e1f845e4
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.
Done
https://gerrit.asterisk.org/c/asterisk/+/18068/comment/091e8302_b35bd105
PS5, Line 165: break;
> This line can be removed.
Done
https://gerrit.asterisk.org/c/asterisk/+/18068/comment/2d9e526e_ed98d38c
PS5, Line 233: return -1;
> "location" is ref leaked here.
Done
https://gerrit.asterisk.org/c/asterisk/+/18068/comment/fda80bb0_e24d2caf
PS5, Line 243: return 0;
> "location" is ref leaked here.
Done
https://gerrit.asterisk.org/c/asterisk/+/18068/comment/89912463_c2acf779
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.
This is how pjsip_configuration does it.
File res/res_geolocation/geoloc_doc.xml:
https://gerrit.asterisk.org/c/asterisk/+/18068/comment/6e925c71_b01ae17a
PS5, Line 10: cffdffff
> Not sure what this is?
Done
https://gerrit.asterisk.org/c/asterisk/+/18068/comment/98e61969_ff6f2c9f
PS5, Line 51: </configObject>
> extra space at eol.
Done
https://gerrit.asterisk.org/c/asterisk/+/18068/comment/e9095d77_3db850e9
PS5, Line 55: cffdffff
> ?
Done
https://gerrit.asterisk.org/c/asterisk/+/18068/comment/1973ba28_72b871dc
PS5, Line 89: xxxx
> Just acting as a placeholder for now?
Done
File res/res_geolocation/geoloc_gml.c:
https://gerrit.asterisk.org/c/asterisk/+/18068/comment/d2ffba69_146004f2
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?
Done
File res/res_geolocation/geoloc_private.h:
https://gerrit.asterisk.org/c/asterisk/+/18068/comment/08c2ef0d_c989f3f4
PS5, Line 80: new_var = ast_variable_new(item_name, item_value, ""); \
> This needs a NULL check.
Done
--
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: 8
Gerrit-Owner: George Joseph <gjoseph at digium.com>
Gerrit-Reviewer: Friendly Automation
Gerrit-Reviewer: Kevin Harwell <kharwell at digium.com>
Gerrit-Attention: Kevin Harwell <kharwell at digium.com>
Gerrit-Comment-Date: Tue, 22 Feb 2022 19:49:56 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Kevin Harwell <kharwell at digium.com>
Gerrit-MessageType: comment
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20220222/9a81952f/attachment-0001.html>
More information about the asterisk-code-review
mailing list