[Home]

Summary:ASTERISK-05910: [patch] various cli fixes (bugs and performance)
Reporter:Luigi Rizzo (rizzo)Labels:
Date Opened:2005-12-27 13:29:52.000-0600Date Closed:2011-06-07 14:09:59
Priority:MinorRegression?No
Status:Closed/CompleteComponents:Core/General
Versions:Frequency of
Occurrence
Related
Issues:
Environment:Attachments:( 0) cli-good.diff
Description:The attached patch fixes two issues:
1. [true bug]
  As noted in ASTERISK-5908, command completion does not always work correctly
  e.g. sometimes it requires you to type the initial character of an object
  to have it correctly expanded.
  The problem appears to be a mix of improper trailing-whitespace handling
  and errors in determining whether we are still in the initial part of
  the command (described in e->cmda[]), or not.
  The patch fixes this by removing the trailing whitespace in the string
  comparisons in __ast_cli_generator(), and figuring out correctly
  (in the same function) when we are done with e->cmda[] and need to look
  in e->generator().
2. [severe performance bug]
  One of the many performance bugs in the cli is that every single time
  a cli entry is used for completion, the full string is rebuilt from its
  components[NOTE 1]. It is simply not worth redoing the computation every
  time, especially given how often this happens [NOTE 2].
  The patch computes the full string at register time (or with a
  builtins_init() call for builtins) and stores it with the entry
  so it can be used afterwards.

Additionally, the patch provides the usual cleanup of related code,
namely (but i may have forgotten some bits, bear with me):
- remove functions join() and join2(), which are not used anymore;
- introduce a struct cli_iterator() and related function cli_next() to
 navigate through the list of builtins and helpers. This replaces some
 replicated blocks of code.
- in parse_args(), put a repeated block into a macro STARTNEW, and
 remove some useless parentheses.
 Also fix a real (but harmless) bug, "(quoted & whitespace)" was meant to
 be "(quoted && whitespace)"
- in cli.h, constify some fields in ast_cli_entry and add the new pointer
 _full_cmd;
- in utils.c, add a call to builtins_init(). If you have a better place,
 please move it where appropriate. See the patch on why the prototype
 is not in a global header.
 

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

[NOTE 1] The next step is to figure out whether we can just remove the
  description in cmda[] and replace it with the entire string (with proper
  post-processing to collapse whitespace).

[NOTE 2] As noted in ASTERISK-5908, there is still a big performance issue with
  the command completion code. For a field with N possible values,
  the generator function for the matching entry typically runs its
  inner loop N^2 times - and these are expensive operations, such as
  walking (and locking and unlocking) object lists.
  This needs to be simplified by extending the API for both
  __ast_cli_generator() and the individual functions.
Comments:By: Tilghman Lesher (tilghman) 2006-01-02 17:35:52.000-0600

builtins_init() needs to be renamed to ast_cli_init(), a prototype needs to be in include/asterisk/cli.h, and it needs to be called from asterisk.c:main().

While you're revamping this, you should probably fix the race condition of iterating over helpers without holding clilock.  Your cli_next() function should never be called without holding clilock.
By: Russell Bryant (russell) 2006-01-03 00:02:54.000-0600

All of the other builtin initialization functions' prototypes are in include/asterisk.h.  That would seem like the appropriate place for this one, as well.
By: Luigi Rizzo (rizzo) 2006-01-19 05:08:53.000-0600

[NOTE -- the correct file is cli-good.diff, the other one should be removed. ]

Updated to include the suggestions of corydon and russel.

The patch also includes a new command, ast_cli_complete(), which
greatly simplifies the writing of 'complete' when expanding into
fixed set of strings - see the documentation in cli.h
Previously, this code was replicated, in slightly different forms,
all over the place. In this way, it is as simple as this (taken from
app_meetme.c, not included yet in the patch):

       static char *cmds[] = {"lock", "unlock", "mute", "unmute",
                 "kick", "list", NULL};
       if (pos == 1) {         /* Command */
               return ast_cli_complete(word, cmds, state);
By: Russell Bryant (russell) 2006-01-26 13:06:42.000-0600

While testing your patch, I found a small bug with tab completion when the string for a command is a substring of another valid command.  Take the following case as an example:

*CLI> show chan<tab>
channel       channels      channeltypes  channeltype

The current CLI command is then completed correctly to "show channel."
 
*CLI> show channel<tab>
channels      channeltypes  channeltype

This is when the problem occurs.  It is no longer showing "channel" as a valid entry at this point.

*CLI> show channelty<tab>
channeltypes  channeltype  

This is correctly completed to "show channeltype".

*CLI> show channeltype<tab>

This incorrectly gets automatically completed to "show channeltypes", instead of providing both "channeltype" and "channeltypes" as options.
By: Luigi Rizzo (rizzo) 2006-01-26 15:17:51.000-0600

ok try the changes below in __ast_cli_generator(),
they seem to fix the problems here:
1) check for matchlen <= lc instead of strictly
less to include a matching token that is a prefix of other matches;
2) add a check for lc >0 to avoid infinite loops on empty strings;
and 3) preserve a trailing whitespace in the match string to enable
completion of a word for which we have no characters yet.
Maybe we should add a bit of comments to explain how things work
but later...

-       ast_join(matchstr, sizeof(matchstr)-1, argv);
+       ast_join(matchstr, sizeof(matchstr)-1, argv);
+       if (tws)
+               strcat(matchstr, " "); /* XXX */
       matchlen = strlen(matchstr);
       if (lock)
               ast_mutex_lock(&clilock);
       while( !ret && (e = cli_next(&i)) ) {
               int lc = strlen(e->_full_cmd);
-               if (e->_full_cmd[0] != '_' && matchlen < lc &&
+               if (e->_full_cmd[0] != '_' && lc > 0 && matchlen <= lc &&
By: Tilghman Lesher (tilghman) 2006-03-06 14:16:30.000-0600

This patch no longer cleanly applies.
By: Tilghman Lesher (tilghman) 2006-03-24 23:42:34.000-0600

Reporter seems to have lost interest.  Reopen when you have a new patch to upload.