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

[George Joseph]

> On Sept. 30, 2014, 1:37 p.m., Mark Michelson wrote:
> > branches/12/main/config.c, lines 751-753
> > <https://reviewboard.asterisk.org/r/4033/diff/2/?file=67696#file67696line751>
> >
> >     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))

That was the expected behavior.  I'll add some comments to explain.


> On Sept. 30, 2014, 1:37 p.m., Mark Michelson wrote:
> > branches/12/main/config.c, lines 795-808
> > <https://reviewboard.asterisk.org/r/4033/diff/2/?file=67696#file67696line795>
> >
> >     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?

Fixed the leak but the the match should remain case-sensitive.  Nothing prevents a user from creating multiple variable names or values differing only in case.  If the match was case-insensitive you could only ever hit the first one.


- George


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


On Sept. 29, 2014, 4: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, 4: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/5e0a58a2/attachment.html>