[Home]

Summary:ASTERISK-05908: [patch] fix for several bugs in pbx_config.c
Reporter:Luigi Rizzo (rizzo)Labels:
Date Opened:2005-12-27 09:00:46.000-0600Date Closed:2011-06-07 14:10:21
Priority:MinorRegression?No
Status:Closed/CompleteComponents:Core/General
Versions:Frequency of
Occurrence
Related
Issues:
Environment:Attachments:( 0) matches.diff
( 1) pbx_config.c
( 2) pbx_config.diff
Description:the code in pbx_config.c is full of inconsistencies and bugs
of various types, including but not limited to the list specified
in the 'Additional information' section below.

I tried to fix a few of these, and the result is attached both as
diff and entire file (more readable than the diffs).

Please review the code. It needs testing (as the original did,  
but did not receive).

I tried some trivial commands, completion etc, and they seem to
work as well or better than the original, but i am not generally
using these commands so i don't know how to perform an extensive
test.

Please don't complain now because there are still a few debugging
statements with C++ comments, they will go by the time a committer
wants to put this in.

Please _do_ complain if the code does not behave as expected,
preferably with a test case (e.g. "type 'include context t' followed
by 23 spaces") so i can try and reproduce them.

There are a few things unclear to me at the moment, namely:
+ do we need to lock individual contexts just to fetch their names,
 or ast_lock_contexts() is sufficient ? (i suspect it suffices);
+ do we need to lock individual contexts while we navigate their
 include or extension lists, or ast_lock_contexts() is sufficient ?
 (i suspect it does not suffice);
+ completion does not behave as i would like, e.g.

       include context <TAB>
               as expected returns the list of existing contexts
 but
       "include context local <TAB>
               does not seem to call the handler to provide the
               next keyword, 'in',
 and
       include context local in <TAB>
               does not seem to call the handler either to return a    
               list of candidates.
 I don't know if i did something wrong, or the behaviour is different
 because the ast_cli_entry structure has the first two keywords.
 Maybe this is a general problem ?

diffs and full file attached.


****** ADDITIONAL INFORMATION ******

Issues with existing pbx_config.c:
 
+ proper bugs, e.g. complete_context_add_include() completes the
 third word as 'in' or 'into', but the latter form is not accepted
 by handle_context_add_include();
 Anyways, the same complete_context_add_include() has all the 'pos'
 checking off by one so it doesn't work at all;

+ inconsistent locking (e.g. complete_context_remove_extension()
 does e = ast_walk_context_extensions(c, c)... without locking c;
 handle_save_dialplan() does the same;
 If locking the individual objects is unnecessary because we
 have ast_lock_contexts(), then it should be removed everywhere;
 otherwise it should be added where missing;

+ inefficient code, e.g.
       strlen(word) is computed one or more times in many loops,
       when the value is just a constant across the whole function;

+ convoluted coding, such as the sequence near line 555
     ret = malloc(strlen(ast_get_extension_name(e)) +
            strlen(ast_get_context_name(c)) + 2);
     if (ret)
       sprintf(ret, "%s@%s", ast_get_extension_name(e),
               ast_get_context_name(c));
 which is simply asprintf(...);
 Or this one at line 809:
       c = ast_walk_contexts(NULL);
       while (c && !context_existence) {
           if (!strcmp(context, ast_get_context_name(c))) {
               context_existence = 1;
               continue;
           }
           c = ast_walk_contexts(c);
       }

 which is simply spelled as follows:

       for (c = NULL; (c = ast_walk_contexts(c)); )
               if (!strcmp(context, ast_get_context_name(c)))
                       break;
       /* check c to determine if found or not */

 and so on;

+ useless casts (lots);

+ inconsistent formatting;

and large duplicated code sections when scanning include lists etc.
Comments:By: Russell Bryant (russell) 2005-12-27 09:10:19.000-0600

I do think there is a general problem with CLI tab completion of arguments beyond 1 after the number of arguments specified in the ast_cli_entry.  

I ran into this when playing with some code for ASTERISK-5887.  I implemented a CLI command, "codetest run".  The completion handler would run correcly when I did "codetest run <tab>" to give me the list of available files with registered tests.  However, when I did "codetest run chan_sip.c <tab>", expecting to be able to get a list of available tests for chan_sip.c, the completion handler was not being called.

I'm glad that you pointed this out, because I was not sure if this was something that I had done wrong.  However, since you are seeing the same thing, I think we can conclude that is a general case not accounted for in the CLI code.
By: Luigi Rizzo (rizzo) 2005-12-27 09:43:32.000-0600

btw another annoying thing is that the command completion code
seems to be called twice in asterisk.c::cli_complete() near
line 1676:

               nummatches = ast_cli_generatornummatches((char *)lf->buffer,ptr);
               matches = ast_cli_completion_matches((char *)lf->buffer,ptr);

Given that the list returned by  ast_cli_completion_matches() is NULL-terminated, the count could be determined by just using the
second call and a for loop on the array.
I tried this, it seems to work.

This would be a significant improvement, because command completion
is terribly expensive - it navigates through lists requiring locking,
allocation and whatnot.

Russel, can you test and commit the matches.diff patch  ?
By: Russell Bryant (russell) 2005-12-27 09:50:13.000-0600

I will test the patch.

I just had another thought about a further optimization to command completion functions.  I have gone through and made some of them only call strlen(word) one time, as you have done in these changes.

However, I just realized that we only need to recalculate the word length if it is the first time the function is begin called for the current word (state == 0).

So, would something like this work?

static int wordlen = 0;
...
if (!state)
    wordlen = strlen(word);
By: Luigi Rizzo (rizzo) 2005-12-27 10:21:14.000-0600

no, static variables would probably not work well because we are in
a multithreaded environment, so you don't know how many concurrent
threads are there.
In fact, static variables are almost always a bad idea unless used
for constants.

Anyways one strlen per function is negligible considering the cost
of all the rest.

E.g. one major performance bug (which deserves a separate bug report)
is the body of __ast_cli_generator() which is called n times in a row
by the ast_cli_completion_matches() and friends with the same
arguments but the last one.

Ideally one would want an API (e.g. passing state = -1) that lets the leaf functions (complete_*()) return all matches at once, instead of one at a
time. As a temporary workaround, still way way more efficient than what
we have now, all the above should apply to ast_cli_generator(), because
at the moment all the work to parse and reassemble the arguments and
find the correct hook is repeated again and again.
By: Russell Bryant (russell) 2005-12-27 10:25:04.000-0600

matches.diff has been merged into the trunk
By: Tilghman Lesher (tilghman) 2006-01-05 17:39:16.000-0600

Are there any more bits that have not already been committed that are clearly bugs and not performance optimizations?  If not, I'd like to move this into the feature arena and get a new patch for those changes alone.
By: Olle Johansson (oej) 2006-02-02 01:35:44.000-0600

rizzo: Corydon is waiting for a reply. Thanks.

/Housekeeping
By: Tilghman Lesher (tilghman) 2006-02-23 16:56:44.000-0600

Suspended due to lack of response from reporter.
By: Digium Subversion (svnbot) 2008-01-15 16:09:20.000-0600

Repository: asterisk
Revision: 7655

U   trunk/asterisk.c

------------------------------------------------------------------------
r7655 | russell | 2008-01-15 16:09:19 -0600 (Tue, 15 Jan 2008) | 3 lines

when doing tab completion, iterate the list of matches to count how many there
are instead of having the list of matches generated twice (issue ASTERISK-5908)

------------------------------------------------------------------------

http://svn.digium.com/view/asterisk?view=rev&revision=7655
By: Digium Subversion (svnbot) 2008-01-15 16:09:33.000-0600

Repository: asterisk
Revision: 7670

_U  team/crichter/0.3.0/
U   team/crichter/0.3.0/apps/app_chanspy.c
U   team/crichter/0.3.0/apps/app_dictate.c
U   team/crichter/0.3.0/apps/app_queue.c
U   team/crichter/0.3.0/apps/app_voicemail.c
U   team/crichter/0.3.0/asterisk.c
U   team/crichter/0.3.0/channels/chan_iax2.c
U   team/crichter/0.3.0/channels/chan_sip.c
U   team/crichter/0.3.0/channels/iax2-provision.c
U   team/crichter/0.3.0/cli.c
U   team/crichter/0.3.0/manager.c
U   team/crichter/0.3.0/pbx.c

------------------------------------------------------------------------
r7670 | crichter | 2008-01-15 16:09:33 -0600 (Tue, 15 Jan 2008) | 76 lines

Merged revisions 7655-7662,7664,7666-7668 via svnmerge from
https://origsvn.digium.com/svn/asterisk/trunk

................
r7655 | russell | 2005-12-27 18:24:54 +0100 (Di, 27 Dez 2005) | 3 lines

when doing tab completion, iterate the list of matches to count how many there
are instead of having the list of matches generated twice (issue ASTERISK-5908)

................
r7656 | tilghman | 2005-12-27 18:53:48 +0100 (Di, 27 Dez 2005) | 2 lines

Bug 5237 - Optional filename argument

................
r7657 | russell | 2005-12-27 19:18:41 +0100 (Di, 27 Dez 2005) | 3 lines

avoid repeated calls to strlen in command completion functions and normalize
some loops

................
r7658 | tilghman | 2005-12-27 20:13:13 +0100 (Di, 27 Dez 2005) | 2 lines

Bug 4880 - add priority label matching and dialplan function retrieval

................
r7659 | russell | 2005-12-27 20:48:44 +0100 (Di, 27 Dez 2005) | 2 lines

avoid unneeded calls to strlen in iax2 completion functions

................
r7660 | russell | 2005-12-27 20:59:09 +0100 (Di, 27 Dez 2005) | 3 lines

avoid duplicate strlen calls for the command completion functions for
'show application' and 'show applications'

................
r7661 | russell | 2005-12-27 21:03:07 +0100 (Di, 27 Dez 2005) | 2 lines

minor cleanups for another cli completion function ...

................
r7662 | russell | 2005-12-27 22:03:18 +0100 (Di, 27 Dez 2005) | 2 lines

fix permissions of created recordings (issue ASTERISK-5909)

................
r7664 | russell | 2005-12-28 18:31:12 +0100 (Mi, 28 Dez 2005) | 2 lines

restore alphabetical order for builtin cli commands (issue ASTERISK-5915)

................
r7666 | russell | 2005-12-28 18:37:35 +0100 (Mi, 28 Dez 2005) | 10 lines

Merged revisions 7665 via svnmerge from
https://origsvn.digium.com/svn/asterisk/branches/1.2

........
r7665 | russell | 2005-12-28 12:35:56 -0500 (Wed, 28 Dec 2005) | 2 lines

fix memory leak in build_rpid (issue ASTERISK-5912)

........

................
r7667 | russell | 2005-12-29 09:15:48 +0100 (Do, 29 Dez 2005) | 2 lines

avoid multiple strlen calls in complete_queue

................
r7668 | russell | 2005-12-29 09:25:06 +0100 (Do, 29 Dez 2005) | 2 lines

normalize a loop and avoid multiple strlen calls when completing voicemail users

................

------------------------------------------------------------------------

http://svn.digium.com/view/asterisk?view=rev&revision=7670