[asterisk-dev] [Code Review] 4033: manager/config: Enhancements to support templates and non-unique category names via AMI

[Mark Michelson]

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/4033/#review13414
-----------------------------------------------------------


I made it about a third of the way through the review before I needed to stop to head out for the day. I'll pick it back up tomorrow.


branches/12/main/config.c
<https://reviewboard.asterisk.org/r/4033/#comment23933>

    s/Previoust/Prevoius/



branches/12/main/config.c
<https://reviewboard.asterisk.org/r/4033/#comment23936>

    Two nitpicks:
    
    1. You have some camelCase variable names here and Asterisk tends towards snake_case.
    
    2. regerr and rc can be declared inside the large while loop in this function since they are not referenced outside of it. Further, rc does not need to be initialized when declared.



branches/12/main/config.c
<https://reviewboard.asterisk.org/r/4033/#comment23934>

    This if allows for a zero-length category_name to result in a true return from does_category_match(). This is especially easy to do in the case that a NULL match parameter is passed in.
    
    I think the if should be rewritten as:
    
    if (ast_strlen_zero(category_name) || strcasecmp(cat->name, category_name))



branches/12/main/config.c
<https://reviewboard.asterisk.org/r/4033/#comment23937>

    You're currently leaking memory from the regcomp() calls. You need to call regfree() on &r_name and &r_value.
    
    Also, should REG_ICASE be one of the flags passed to regcomp?


- Mark Michelson


On Sept. 29, 2014, 10:11 p.m., George Joseph wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/4033/
> -----------------------------------------------------------
> 
> (Updated Sept. 29, 2014, 10:11 p.m.)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Repository: Asterisk
> 
> 
> Description
> -------
> 
> This patch provides the capability to manipulate templates and categories with non-unique names via AMI.
> 
> Summary of changes:
> 
> GetConfig and GetConfigJSON:
> Added "Filter" parameter:  A comma separated list of name_regex=value_regex expressions which will cause only categories whose variables match all expressions to be considered.  The special variable name TEMPLATES can be used to control whether templates are included.  Passing 'include' as the value will include templates along with normal categories. Passing 'restrict' as the value will restrict the operation to ONLY templates.  Not specifying a TEMPLATES expression results in the current default behavior which is to not include templates.
> 
> Examples:
> "GetConfig?Filename=pjsip.conf&Category=myitsp&Filter=type=aor" would return only 'myitsp' categories with a type of 'aor'.
> 
> "GetConfig?Filename=pjsip.conf&Category=itsp-template&Filter=TEMPLATES=restrict,type=aor" would return only 'itsp-template' categories that are actually templates with a type of 'aor'.
> 
> "GetConfig?Filename=pjsip.conf&Filter=type=registration,endpoint=myitsp" would return only registrations whose corresponding endpoint is 'myitsp'.
> 
> The output from GetConfig and GetConfigJSON also includes variables indicating if the returned category is a template and the template names a category inherits from if any.
> 
> UpdateConfig:
> NewCat now includes options for allowing duplicate category names, indicating if the category should be created as a template, and specifying templates the category should inherit from.  The rest of the actions now accept a filter string as defined above.  If there are non-unique category names, you can now update specific ones based on variable values.
> 
> To facilitate the new capabilities in manager, corresponding changes had to be made to config, most notably the addition of filter criteria to many of the APIs.  In some cases it was easy to change the references to use the new prototype but others would have required touching too many files for this patch so a wrapper with the original prototype was created.  Macros couldn't be used in this case because it would break binary compatibility with modules such as res_digium_phone that are linked to real symbols.
> 
> 
> Diffs
> -----
> 
>   branches/12/tests/test_sorcery_realtime.c 424097 
>   branches/12/tests/test_sorcery.c 424097 
>   branches/12/tests/test_config.c 424097 
>   branches/12/res/res_sorcery_realtime.c 424097 
>   branches/12/res/res_sorcery_config.c 424097 
>   branches/12/pbx/pbx_realtime.c 424097 
>   branches/12/main/manager.c 424097 
>   branches/12/main/config.c 424097 
>   branches/12/include/asterisk/config.h 424097 
>   branches/12/apps/app_voicemail.c 424097 
>   branches/12/apps/app_directory.c 424097 
> 
> Diff: https://reviewboard.asterisk.org/r/4033/diff/
> 
> 
> Testing
> -------
> 
> Testsuite before and after runs gave the same results.
> A set of unit tests for config was added to test_config.  They test basic, filtered and template operations.
> 
> 
> Thanks,
> 
> George Joseph
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-dev/attachments/20140930/6c23d25e/attachment-0001.html>