[asterisk-dev] [Code Review]: Tab completion for 'acl show'

[Matt Jordan]

> On Dec. 6, 2012, 8:13 a.m., Matt Jordan wrote:
> > /trunk/main/named_acl.c, lines 522-534
> > <https://reviewboard.asterisk.org/r/2230/diff/1/?file=32303#file32303line522>
> >
> >     This could actually crash.  ast_named_acl_init doesn't return failure if it fails to parse the configuration (it probably should...), which means you could have nothing stored in the globals ao2 container.  When you get the reference in the constructor call of the RAII_VAR macro, it could actually be NULL (nothing is in the container), which you dereference in ao2_iterator_init.
> >     
> >     I'd either check that you have a cfg object prior to doing the generation, or change it so that a failure to load the named ACLs completely bombs out.  This would make it so Asterisk won't load, which would be more intrusive, but there's a good chance it probably should completely fail to load if there's a configuration error in this component.
> 
> Russell Bryant wrote:
>     It looks like the file not existing is treated the same as a parse error in the file as far as the return value goes.  We don't want to block Asterisk from starting if the file doesn't exist, so for now I'll just fix up the tab completion code to do a non-NULL check.
>     
>     Somewhat related note ... while looking at aco_process_config(), I noticed some dead code.
>     
>                     if (!(cfg = ast_config_load(filename, cfg_flags))) {
>                             if (file->alias && strcmp(file->alias, filename)) {
>                                     filename = file->alias;
>                                     goto try_alias;
>                             }
>                             ast_log(LOG_ERROR, "Unable to load config file '%s'\n", file->filename);
>                             res = ACO_PROCESS_ERROR;
>                             break;
>                     } ...
>     
>                     } else if (cfg == CONFIG_STATUS_FILEMISSING) {
>                             if (file->alias && strcmp(file->alias, filename)) {
>                                     filename = file->alias;
>                                     goto try_alias;
>                             }
>                             ast_log(LOG_ERROR, "%s is missing! Cannot load %s\n", file->filename, info->module);
>                             res = ACO_PROCESS_ERROR;
>                             break;
>                     }
>     
>     The second block here will never be reached.  CONFIG_STATUS_FILEMISSING is 0, so the first block will run in that case.

Yeah, bailing because you chose to not provide a config file would be bad.

I know Terry wrote the file alias code after the first pass on the config parsing, so I'm not surprised there was some accidental duplication.  IMO, the file alias should probably be handled explicitly in the second block if possible - that's better than relying on the fact that CONFIG_STATUS_FILEMISSING == 0.


- Matt


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


On Dec. 6, 2012, 7:55 a.m., Russell Bryant wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/2230/
> -----------------------------------------------------------
> 
> (Updated Dec. 6, 2012, 7:55 a.m.)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Summary
> -------
> 
> Add tab completion for the 'acl show' CLI command.
> 
> 
> Diffs
> -----
> 
>   /trunk/main/named_acl.c 377308 
> 
> Diff: https://reviewboard.asterisk.org/r/2230/diff
> 
> 
> Testing
> -------
> 
> Did some mad tab complete action.
> 
> 
> Thanks,
> 
> Russell
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-dev/attachments/20121206/4ead7b01/attachment-0001.htm>